Closed Bug 960641 Opened 6 years ago Closed 6 years ago

Queue browser API calls before remote frame is shown

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: fabrice, Assigned: kk1fff)

References

Details

Attachments

(1 file, 5 obsolete files)

I will create gaia bug for this API.
I don't think we should introduce |mozbrowserready| event. The solution to work with these kind of delayed initialization is almost always queue-requests-until-ready, and it doesn't make sense for Gaia to manage that queue itself, given the fact all of the mozbrowser functions are already async.

Please create a queue in browserElementParent rather than having Alive to do so in Gaia (even through I know he have already written the patch).
Tim, currently it's hard to create a queue because the methods *are not exposed yet* at all at this point...
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #2)
> I don't think we should introduce |mozbrowserready| event. The solution to
> work with these kind of delayed initialization is almost always
> queue-requests-until-ready, and it doesn't make sense for Gaia to manage
> that queue itself, given the fact all of the mozbrowser functions are
> already async.
> 

Yes, that is what I am worried about, adding such patch would change browser API a lot. Sorry for proposing this bad idea. I think we should fix this inside Gecko.
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Tim, currently it's hard to create a queue because the methods *are not
> exposed yet* at all at this point...

I am trying to make preallocated process ready before Gaia attach the first iframe to DOM. If this cannot fix the problem, I think, at least, we can expose browser API before process is actually created, and queue all the API calls until the process is ready.
Whiteboard: [tarako]
https://tbpl.mozilla.org/?tree=Try&rev=02c0bd9fb4d9

Mochitest-5 of emulator debug became green after rebase.
Summary: Add a browserready event to the mozbrowser api → Queue browser API calls before remote frame is shown
Attached patch Proposed Patch (obsolete) — Splinter Review
Attachment #8361810 - Attachment is obsolete: true
Attachment #8361811 - Attachment is obsolete: true
Attachment #8362126 - Flags: review?(fabrice)
Comment on attachment 8362126 [details] [diff] [review]
Proposed Patch

Review of attachment 8362126 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixes. I assumed you checked that this doesn't break builds that don't have nuwa enabled?

::: content/base/src/nsFrameLoader.h
@@ +460,5 @@
>    // forwards some input events to out-of-process content.
>    uint32_t mEventMode;
> +
> +  // Indicate if we have sent 'remote-browser-frame-pending'.
> +  bool mPendingFrame;

s/mPendingFrame/mPendingFrameSent would be a bit clearer.

::: dom/browser-element/BrowserElementParent.jsm
@@ +134,5 @@
> +  let defineNoReturnMethod = function(name, fn) {
> +    XPCNativeWrapper.unwrap(self._frameElement)[name] = function method() {
> +      if (!self._mm) {
> +        // Remote browser haven't been created, we just queue the API call.
> +        var args = Array.slice(arguments);

nit: s/var/let

@@ +224,5 @@
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>                                           Ci.nsISupportsWeakReference]),
>  
> +  _runPendingAPICall: function() {

s/_runPendingAPICall/_runPendingAPICalls
Comment on attachment 8362126 [details] [diff] [review]
Proposed Patch

Review of attachment 8362126 [details] [diff] [review]:
-----------------------------------------------------------------

something else: We don't try currently to optimize any duplicate calls. (like, getting several getScreenShot calls). We may want to optimize a bit in a followup.
Attachment #8362126 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #13)
> Comment on attachment 8362126 [details] [diff] [review]
> Proposed Patch
> 
> Review of attachment 8362126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits fixes. I assumed you checked that this doesn't break builds
> that don't have nuwa enabled?

I had submitted a try: https://tbpl.mozilla.org/?tree=Try&rev=8cb9f3cf8fd4
I will land this after try pass.
The orange m-5 is still on emulator debug bug. It looks more like the failure is caused by this patch.
Delayed registerBrowserElementParentForApp to remote frame created. If we register before frame create, Webapp.jsm will throw an exception since it try to use the message manager in BEP. Requesting review for this change.

central: https://tbpl.mozilla.org/?rev=57317659a609
with this patch and nuwa enabled: https://tbpl.mozilla.org/?tree=Try&rev=970017d2788c
with this patch and nuwa disabled: https://tbpl.mozilla.org/?tree=Try&rev=c544e6fa5030
Attachment #8362126 - Attachment is obsolete: true
Attachment #8362541 - Flags: review?(fabrice)
Blocks: 961739
Attachment #8362541 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/6410e5487e43
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
nominating since this blocks an 1.3 bug.
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → 1.3+
1.3T fixed. remove [tarako] whiteboard
Whiteboard: [tarako]
See Also: → 1127189
You need to log in before you can comment on or make changes to this bug.