1003848 - Don't use sync messaging during BrowserElementChild initialization

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js?rev=9bf6ffacf4f6&mark=55-55#55 shows up in the profiles since it causes child process to just wait for parent.
We could, I think, just send the name when creating the child.

Comment 1

[[:fabrice] Fabrice Desré]

We need both fullscreenAllowed very early to let apps go fullscreen or not too.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

We could just set name and fullscreenallowed when creating a mozbrowser.
Parent would send that info during creation.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, we may run BrowserElementChild.js in the same process as *Parent

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Boo, we have also the preload mechanism making this all more complicated.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: no-sync-hello.patch

It's mostly green on try (https://tbpl.mozilla.org/?tree=Try&rev=0b49b62a014d) but not fully yet...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

s/NS_LITERAL_STRING("")/EmptyString()/

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8507472 [details] [diff] [review]
no-sync-hello.patch

Don't you need to have some special case for FakeShow

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

But yes, the approach looks good to me.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Still one test failure (M5 emulator opt only :( ) where we get a visibilitychange event earlier than expected.

https://tbpl.mozilla.org/?tree=Try&rev=4422f4aa98c4

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Rebased. Try run is at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dec35a628939

There are 2 real failures (Gip f2 and N2 in emulator opt) that are both related to visibilitychange issues.

For the M2 failure (that I can't repro locally of course...) we get the visibilitychange event before the onload one in https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Inner.html?force=1

I'm not sure if that actually a bug or not. Olli, what do you think?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

load event doesn't have anything to do with visibility so, so yes, visibilitychange may easily happen before load.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: patch v4

Works fine, green run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ec645029389c

The test change is there to not expect anymore than we get onload before onvisibility change.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8529463 [details] [diff] [review]
patch v4

>+    mDocShell->SetFullscreenAllowed(
>+      mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::allowfullscreen) ||
>+      mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozallowfullscreen));
>+    bool isPrivate = mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozprivatebrowsing);
>+    if (isPrivate) {
>+      bool nonBlank;
>+      mDocShell->GetHasLoadedNonBlankURI(&nonBlank);
>+      if (nonBlank) {
>+        printf_stderr("We should not switch to Private Browsing after loading a document.\n");
this is a bit odd. 
Report to console?

>-  else {
>+  /*else {
>     // Pass the message up to our parent.
>     alert(e.detail.message);
>-  }
>+  }*/

I don't understand this. Please explain.
And either remove the code, or remove /* and */

>+struct ShowInfo
>+{
>+  nsString name;
>+  bool fullscreenAllowed;
>+  bool isPrivate;

a ctor would be good here.
It would initialize both bool variables to false.

>+    ShowInfo info(EmptyString(), true, false);
and then you could just have
ShowInfo info; here.
(I don't understand why you have true for fullscreenAllowed here. Looks wrong)

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: patch v5

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8529463 [details] [diff] [review]

> >+      if (nonBlank) {
> >+        printf_stderr("We should not switch to Private Browsing after loading a document.\n");
> this is a bit odd. 
> Report to console?

Done, using nsContentUtils::ReportToConsoleNonLocalized()

> >-  else {
> >+  /*else {
> >     // Pass the message up to our parent.
> >     alert(e.detail.message);
> >-  }
> >+  }*/
> 
> I don't understand this. Please explain.
> And either remove the code, or remove /* and */

This is to fix the issue explained in comment 11 : we get a visibility change event before onload on emulator, and that makes the test fail because we relay an unexpected event.

> >+struct ShowInfo
> >+{
> >+  nsString name;
> >+  bool fullscreenAllowed;
> >+  bool isPrivate;
> 
> a ctor would be good here.
> It would initialize both bool variables to false.

This is a ipdl struct, which is codegene'd. I didn't find a way to add a constructor by annotating the ipdl. Let me know if there's a way!

> (I don't understand why you have true for fullscreenAllowed here. Looks
> wrong)

Fixed, and I verified with fullscreen apps that everything works properly.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8530472 [details] [diff] [review]
patch v5

>+TabChild::ApplyShowInfo(const ShowInfo& info)
aInfo

ditto elsewhere for params.

Comment 17

[[:fabrice] Fabrice Desré]

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

Comment 18

[Carsten Book [:Tomcat]]

sorry backed out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=941014&repo=b2g-inbound

Comment 19

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/6e90fd9bf6e2

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e90fd9bf6e2