960641 - Queue browser API calls before remote frame is shown

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

See the rationale in https://bugzilla.mozilla.org/show_bug.cgi?id=950266#c18

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

I will create gaia bug for this API.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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).

Comment 3

[[:fabrice] Fabrice Desré]

Tim, currently it's hard to create a queue because the methods *are not exposed yet* at all at this point...

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: expose browser api when inserting frame to DOM

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: queue API call if the process haven't been created

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Notify observers when a remote frame is inserted to DOM

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: expose browser API when frame is insert to DOM.

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

try: https://tbpl.mozilla.org/?tree=Try&rev=65a528aa7141

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://tbpl.mozilla.org/?tree=Try&rev=02c0bd9fb4d9

Mochitest-5 of emulator debug became green after rebase.

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Comment 13

[[:fabrice] Fabrice Desré]

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 14

[[:fabrice] Fabrice Desré]

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.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

The orange m-5 is still on emulator debug bug. It looks more like the failure is caused by this patch.

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch v2

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

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/6410e5487e43

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6410e5487e43

Comment 20

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

nominating since this blocks an 1.3 bug.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5fb1fa58180a

Comment 22

[Joe Cheng [:jcheng] (please needinfo)]

1.3T fixed. remove [tarako] whiteboard