926452 - The call to getComputedStyle in the onStateChange progress listener of BrowserElementChildPreload.js is expensive (~30ms)

Status: RESOLVED WONTFIX

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Expensive is relative here since it tooks a few ms, but this hurts page start when this stuff is only really useful for a case where the application is in landscape mode and takes time to load.

It worth trying to move it out of the startup path.

Comment 1

[[:fabrice] Fabrice Desré]

Yep, I timed that again on a Tarako. It's pretty terrible, ranging from a few ms to sometimes as long as 25/30ms

Comment 2

[Dietrich Ayala (:dietrich)]

Is there a way to eagerly check or pass in orientation to avoid doing this every time?

Comment 3

[[:fabrice] Fabrice Desré]

If I understand apps/system/js/window_manager.js:340 correctly we could delay that a bit with no harm (unless the user is rotating its phone like crazy...).

Comment 4

[Bobby Chien]

[Tracking Requested - why for this release]:

Comment 5

[Hema Koka [:hema]]

Based on Wilson's test results, he says this is costs around 30ms. Though it is not a huge win, small costs are adding up. 

Thinker/Bobby: could we get an assessment on what it would take to address this bug in 2.5? All small improvements will make a big difference. 

Profile info on bug 1164539

Comment 6

[Ting-Yu Chou [:ting] (away)]

If the style is always calculated before first paint, will this still be helpful for bug 1164539?

Will talk to Kanru after he's back from PTO.

Comment 7

[Wilson Page [:wilsonpage]]

Profile with c++ stack traces: https://people.mozilla.org/~bgirard/cleopatra/#report=5c719c423891c6a2e7c493947f631741fed25f64

Comment 8

[Wilson Page [:wilsonpage]]

Attachment: before.png

Comment 9

[Wilson Page [:wilsonpage]]

Attachment: after.png

Comment 10

[Wilson Page [:wilsonpage]]

Removing the call to getComputedStyle()[1] improved first postMessage() time by 38ms.

PROFILE: https://people.mozilla.org/~bgirard/cleopatra/#report=695a354d4c2580dca07f6212a6b0e77fa4fd0958

[1] https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#1596

Comment 11

[Wilson Page [:wilsonpage]]

AS mentioned earlier in this bug, getComputedStyle() is called on every app's launch path to determine the background-color. This background-color is then set on an element in the system app that contains the mozbrowser <iframe>.

This is done so that when devices are rotated and apps take some time to reflow we can show a better color than white in the empty space in the period before the app has reflowed to fill.

If this is the only use-case, it seems we could delay this work till after the launch path. I don't know enough about mozbrowser to suggest when would be a better time. I think Dale knows more about this. Dale any guidance here?

Comment 12

[Ting-Yu Chou [:ting] (away)]

(In reply to Wilson Page [:wilsonpage] from comment #7)
> Profile with c++ stack traces:
> https://people.mozilla.org/~bgirard/cleopatra/
> #report=5c719c423891c6a2e7c493947f631741fed25f64

It's GetOrCreateDOMReflector() of getComputedStyle() taking time. BTW, I don't see this bug in normal application launch.

Comment 13

[Wilson Page [:wilsonpage]]

IIRC Vivien said this code isn't actual required any more and could be stripped. NI to clarify and maybe patch ;)

Comment 14

[Ting-Yu Chou [:ting] (away)]

(In reply to Wilson Page [:wilsonpage] from comment #13)
> IIRC Vivien said this code isn't actual required any more and could be
> stripped. NI to clarify and maybe patch ;)

Remove the code will see white screen.

In bug 869903, there was a discussion which to set the default backgrdound color black, also :mwu tried to set the background color at mozbrowserfirstpaint but it failed some tests.

Comment 15

[Ting-Yu Chou [:ting] (away)]

My plan is to remove the code from bug 869903 and fix the white screen properly.

The background color of browser iframe was black, but then change to white [1], not sure about the reason. Revert it [2] back to #fff and remove the getComputedStyle() will not see bug 869903.

But we need some background for the code of bug 1047829 [3], Vivien?

[1] https://github.com/mozilla-b2g/gaia/commit/9e8e6d5318c31d75fc3fbeaaa0dab44297213fd8
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window.css#L66
[3] https://github.com/mozilla-b2g/gaia/commit/2e20dfb616a8ec8f01a7000dd6bdb32eb2b10dc1#diff-46b349b14289333755febebd351e16ccL35

Comment 16

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #15)
> The background color of browser iframe was black, but then change to white
> [1], not sure about the reason.
> [1]
> https://github.com/mozilla-b2g/gaia/commit/
> 9e8e6d5318c31d75fc3fbeaaa0dab44297213fd8

Ben, do you remember the reason of this change?

Comment 17

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #15)
> The background color of browser iframe was black, but then change to white
> [1], not sure about the reason. Revert it [2] back to #fff and remove the
> getComputedStyle() will not see bug 869903.

Correction: Revert it [2] back to #000.

Comment 18

[[:fabrice] Fabrice Desré]

iirc, we need a white background (and black text color) by default for pages that have no styling to display properly.

Comment 19

[Ting-Yu Chou [:ting] (away)]

(In reply to [:fabrice] Fabrice Desré from comment #18)
> iirc, we need a white background (and black text color) by default for pages
> that have no styling to display properly.

Then maybe some code has been changed, otherwise this should be broken once orientationchange is fired as it changes browser iframe's background color. I'll double check, thanks for the info :)

Comment 20

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #19)
> (In reply to [:fabrice] Fabrice Desré from comment #18)
> > iirc, we need a white background (and black text color) by default for pages
> > that have no styling to display properly.
> 
> Then maybe some code has been changed, otherwise this should be broken once
> orientationchange is fired as it changes browser iframe's background color.
> I'll double check, thanks for the info :)

I meant mozbrowserloadend.

Comment 21

[Ting-Yu Chou [:ting] (away)]

Double checked, Fabrice (comment 18) is right, set browser iframe background color to black will break the page without styling.

Comment 22

[Dale Harvey (:daleharvey)]

Clearing as it looks like this has ownership and details now, cheers

Comment 23

[Ting-Yu Chou [:ting] (away)]

From LayerScope, I found the RefLayerComposite of content has rotated orientation, but its reference layer (ContainerLayerComposite) is still unrotated.

Comment 24

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #23)
> From LayerScope, I found the RefLayerComposite of content has rotated
> orientation, but its reference layer (ContainerLayerComposite) is still
> unrotated.

I've talked to :mtseng whether in this case to fill remaining area black make sense or not, it sounds ok, but there're some other cases could have similar situation, e.g., 3d transform.

Comment 25

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #24)
> (In reply to Ting-Yu Chou [:ting] from comment #23)
> > From LayerScope, I found the RefLayerComposite of content has rotated
> > orientation, but its reference layer (ContainerLayerComposite) is still
> > unrotated.
> 
> I've talked to :mtseng whether in this case to fill remaining area black
> make sense or not, it sounds ok, but there're some other cases could have
> similar situation, e.g., 3d transform.

I think the code can be in RefLayerComposite::RenderLayer(), but I guess it's better to check a flag that they're in different orientation instead of comparing the rect/region everytime.

Bas, I am not familiar with the code, could you please suggest how should I proceed? Thank you.

Comment 26

[vnicolas]

Attachment: stop.sending.bgcolor.on.loadend.patch

Once support for background_color will land (in bug 1215077), the code that reads the background color of the page can just be removed. There will be a background-color derived from the theme_color, or from background_color if present and support is enabled.

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to vnicolas from comment #26)
> Created attachment 8674233 [details] [diff] [review]
> stop.sending.bgcolor.on.loadend.patch
> 
> Once support for background_color will land (in bug 1215077), the code that
> reads the background color of the page can just be removed. There will be a
> background-color derived from the theme_color, or from background_color if
> present and support is enabled.

What will be the consequences for pages that have no manifest to get a background color from?

Comment 28

[vnicolas]

(In reply to [:fabrice] Fabrice Desré from comment #27)
> (In reply to vnicolas from comment #26)
> > Created attachment 8674233 [details] [diff] [review]
> > stop.sending.bgcolor.on.loadend.patch
> > 
> > Once support for background_color will land (in bug 1215077), the code that
> > reads the background color of the page can just be removed. There will be a
> > background-color derived from the theme_color, or from background_color if
> > present and support is enabled.
> 
> What will be the consequences for pages that have no manifest to get a
> background color from?

They can always use a <meta name="theme-color" content="..."> instruction to control the background-color of this part of the UI. It is not perfect but more flexible than the one shot default background-color.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0359c684793

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b0359c684793

Comment 31

[Ting-Yu Chou [:ting] (away)]

Clear NI to bas as this is resolved through another route.

Comment 32

[David Flanagan [:djf]]

This bug caused bug 1213051. That bug is a 2.5+ blocker.  Vivien agrees that we should back this patch out until 1215077 lands. See https://bugzilla.mozilla.org/show_bug.cgi?id=1213051#c17

Comment 33

[Punam Dahiya [:pdahiya]]

Setting NI flag for Farbrice to help with backout. Thanks

Comment 34

[[:fabrice] Fabrice Desré]

backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/8a995569a4c0

Comment 35

[Thinker Li [:sinker]]

c

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Closing bugs with b2g-master=affected as it is likely to be out dated.