Merge lp:~abreu-alexandre/unity-webapps-qml/handle-delayed-content-handler-creation into lp:unity-webapps-qml

Proposed by Alexandre Abreu on 2016-04-27
Status: Merged
Approved by: Alberto Mardegan on 2016-04-28
Approved revision: 158
Merged at revision: 158
Proposed branch: lp:~abreu-alexandre/unity-webapps-qml/handle-delayed-content-handler-creation
Merge into: lp:unity-webapps-qml
Diff against target: 150 lines (+98/-7)
1 file modified
src/Ubuntu/UnityWebApps/bindings/content-hub/backend/content-hub.js (+98/-7)
To merge this branch: bzr merge lp:~abreu-alexandre/unity-webapps-qml/handle-delayed-content-handler-creation
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) 2016-04-27 Approve on 2016-04-28
PS Jenkins bot (community) continuous-integration Approve on 2016-04-27
Review via email: mp+293165@code.launchpad.net

Commit message

Handle delayed creation of content hub share/import/export handler that could miss transfer requests

Description of the change

Handle delayed creation of content hub share/import/export handler that could miss transfer requests

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
158. By Alexandre Abreu on 2016-04-27

tweak

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Alberto Mardegan (mardy) wrote :

Can you clarify a bit how this works? If I look at this code, it seems that the _#action#Requested() functions are only called after pushing a callback into the array, so it seems that the first branch of the if() can never be executed:

       _exportRequested: function(transfer) {
           if (this._exportRequestedCallbacks.length === 0) { // <- never true
               if (this._currentExportTransfer) {
                   this._currentExportTransfer.state = ContentTransfer.Aborted
               }
               this._currentExportTransfer = transfer
           } else {
               this._runHandlers(this._exportRequestedCallbacks, transfer)
           }

review: Needs Information
Alexandre Abreu (abreu-alexandre) wrote :

> Can you clarify a bit how this works? If I look at this code, it seems that
> the _#action#Requested() functions are only called after pushing a callback
> into the array, so it seems that the first branch of the if() can never be
> executed:
>
> _exportRequested: function(transfer) {
> if (this._exportRequestedCallbacks.length === 0) { // <- never true
> if (this._currentExportTransfer) {
> this._currentExportTransfer.state = ContentTransfer.Aborted
> }
> this._currentExportTransfer = transfer
> } else {
> this._runHandlers(this._exportRequestedCallbacks, transfer)
> }

actually the rationale behind this is this one:

at the moment (and I am thinking about possibly revamping it), the content hub bits found in the JS bindings (used by html5 and webapps) are triggered by the JS. Meaning that as the application is being started, it is not until the webview has been triggered, and the content loaded that the JS code scripts are being injected to potentially add e.g. share handler for the content hub.

This delay creates a clear race and the content hub sharing transfer is currently being dropped since at the time that all the QML app tree is complete, there are no share requested signal handlers.

When the JS finally gets executed and binds some share handler callbacks the transfer requested has been popped.

So the idea is for those signals that can wake the app, always define handlers, and acknowledge the fact that only until we have one callback defined the app could have been woken up by a missed share/import/export request.

In this context, the expression this._exportRequestedCallbacks.length evaluates to 0 when this happens (when the app is being woken up by a share request and the JS hasnt run yet).

Alberto Mardegan (mardy) wrote :

> So the idea is for those signals that can wake the app, always define
> handlers, and acknowledge the fact that only until we have one callback
> defined the app could have been woken up by a missed share/import/export
> request.

Now I got it, thanks! I missed the connections made in the setup() method; now it's clear.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Ubuntu/UnityWebApps/bindings/content-hub/backend/content-hub.js'
2--- src/Ubuntu/UnityWebApps/bindings/content-hub/backend/content-hub.js 2015-05-06 16:38:34 +0000
3+++ src/Ubuntu/UnityWebApps/bindings/content-hub/backend/content-hub.js 2016-04-27 20:15:04 +0000
4@@ -44,6 +44,97 @@
5
6 var _contenthub = ContentHubBridge.ContentHub;
7
8+ function ContentHubEventHandler() {
9+ this._contentHub = _contenthub;
10+
11+ this._shareRequestedCallbacks = []
12+ this._importRequestedCallbacks = []
13+ this._exportRequestedCallbacks = []
14+
15+ this._currentShareTransfer = null
16+ this._currentImportTranfer = null
17+ this._currentExportTransfer = null
18+
19+ this.setup()
20+ }
21+ ContentHubEventHandler.prototype = {
22+ setup: function() {
23+ this._contentHub.exportRequested.connect(
24+ this._exportRequested.bind(this))
25+ this._contentHub.importRequested.connect(
26+ this._importRequested.bind(this))
27+ this._contentHub.shareRequested.connect(
28+ this._shareRequested.bind(this))
29+ },
30+ _exportRequested: function(transfer) {
31+ if (this._exportRequestedCallbacks.length === 0) {
32+ if (this._currentExportTransfer) {
33+ this._currentExportTransfer.state = ContentTransfer.Aborted
34+ }
35+ this._currentExportTransfer = transfer
36+ } else {
37+ this._runHandlers(this._exportRequestedCallbacks, transfer)
38+ }
39+ },
40+ _importRequested: function(transfer) {
41+ if (this._importRequestedCallbacks.length === 0) {
42+ if (this._currentImportTransfer) {
43+ this._currentImportTransfer.state = ContentTransfer.Aborted
44+ }
45+ this._currentImportTransfer = transfer
46+ } else {
47+ this._runHandlers(this._importRequestedCallbacks, transfer)
48+ }
49+ },
50+ _shareRequested: function(transfer) {
51+ if (this._shareRequestedCallbacks.length === 0) {
52+ if (this._currentShareTransfer) {
53+ this._currentShareTransfer.state = ContentTransfer.Aborted
54+ }
55+ this._currentShareTransfer = transfer
56+ } else {
57+ this._runHandlers(this._shareRequestedCallbacks, transfer)
58+ }
59+ },
60+
61+ _runHandlers: function(handlers, transfer) {
62+ for (var i = 0;
63+ i < handlers.length;
64+ ++i) {
65+ handlers[i](transfer)
66+ }
67+ },
68+
69+ onExportRequested: function(callback) {
70+ this._exportRequestedCallbacks.push(callback)
71+ if (this._currentExportTransfer
72+ && this._currentExportTransfer.state !== ContentTransfer.Aborted
73+ && this._currentExportTransfer.state !== ContentTransfer.Finalized) {
74+ this._exportRequested(this._currentExportTransfer)
75+ this._currentExportTransfer = null
76+ }
77+ },
78+ onImportRequested: function(callback) {
79+ this._importRequestedCallbacks.push(callback)
80+ if (this._currentImportTransfer
81+ && this._currentImportTransfer.state !== ContentTransfer.Aborted
82+ && this._currentImportTransfer.state !== ContentTransfer.Finalized) {
83+ this._importRequested(this._currentImportTransfer)
84+ this._currentImportTransfer = null
85+ }
86+ },
87+ onShareRequested: function(callback) {
88+ this._shareRequestedCallbacks.push(callback)
89+ if (this._currentShareTransfer
90+ && this._currentShareTransfer.state !== ContentTransfer.Aborted
91+ && this._currentShareTransfer.state !== ContentTransfer.Finalized) {
92+ this._shareRequested(this._currentShareTransfer)
93+ this._currentShareTransfer = null
94+ }
95+ },
96+ };
97+ var contentHubHandler = new ContentHubEventHandler();
98+
99 // TODO find a better way
100 function _nameToContentType(name) {
101 var contentTypePerName = {
102@@ -345,8 +436,8 @@
103 item.object.name = items[i].name;
104 item.object.url = items[i].url;
105 if (items[i].text) {
106- item.object.text = items[i].text;
107- }
108+ item.object.text = items[i].text;
109+ }
110
111 contentItems.push(item.object);
112 }
113@@ -384,10 +475,10 @@
114 var items = [];
115 for (var i = 0; i < self.items.length; ++i) {
116 items.push({
117- name: self.items[i].name.toString(),
118+ name: self.items[i].name.toString(),
119 url: self.items[i].url.toString(),
120 text: self.items[i].text.toString()
121- });
122+ });
123 }
124 return items;
125 }
126@@ -807,21 +898,21 @@
127 },
128
129 onExportRequested: function(callback) {
130- _contenthub.exportRequested.connect(function(exportTransfer) {
131+ contentHubHandler.onExportRequested(function(exportTransfer) {
132 var wrapped = new ContentTransfer(exportTransfer);
133 callback(wrapped.serialize());
134 });
135 },
136
137 onImportRequested: function(callback) {
138- _contenthub.onImportRequested.connect(function(importTransfer) {
139+ contentHubHandler.onImportRequested(function(importTransfer) {
140 var wrapped = new ContentTransfer(importTransfer);
141 callback(wrapped.serialize());
142 });
143 },
144
145 onShareRequested: function(callback) {
146- _contenthub.shareRequested.connect(function(shareTransfer) {
147+ contentHubHandler.onShareRequested(function(shareTransfer) {
148 var wrapped = new ContentTransfer(shareTransfer);
149 callback(wrapped.serialize());
150 });

Subscribers

People subscribed via source and target branches

to all changes: