1170644 - BrowserElementChild causes sync reflow on startup

Status: RESOLVED FIXED

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

Description

[Wilson Page [:wilsonpage]]

Attachment: Screenshot 2015-06-02 18.03.30.png

Adds a performance hit to every app's startup. Not sure if there is a better time we could be doing this, perhaps once we know the app is laid out so not to force early reflow.

Caused by commit: https://github.com/mozilla/gecko-dev/commit/4736be98add3f1da09e7663b49987c2f24006b0e

Comment 1

[Wilson Page [:wilsonpage]]

Attachment: profile-2.json

Comment 2

[Dale Harvey (:daleharvey)]

The addition of the method should not cause the reflow, it being used however definitely will. I can take a look but Chris vaguelly remember you were using this?

Comment 3

[Chris Lord [:cwiiis]]

I guess the thing here is that the getContentDimensions call is forcing the reflow...

Is there an alternative, less expensive way than getContentDimensions that we could get the sizes we need, or could we add a method? Is is possible for the sizes to be lazily loaded with a property getter or some such?

If the answer to those is no, I wonder if we could add a delay before the event gets sent so that we can coalesce possible multiple firings/give apps a chance to do their layout? We do a similar thing for scroll events, for example.

Could probably do with someone from layout to comment on this.

Comment 4

[Chris Lord [:cwiiis]]

Just to note, I'd have suspected we could add a better method than getContentDimensions - if we've received the MozScrollAreaChanged signal, at some point a size has been calculated, and we're really only interested in that size. If the document is pending reflow and the size changes, presumably we'd get another MozScrollAreaChanged signal anyway, we don't want to force that reflow.

Comment 5

[Hema Koka [:hema]]

Jet/Daniel: Who can help with this? Please also see bug: 1164539 

Thanks
Hema

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Chris Lord [:cwiiis] from comment #3)
> I guess the thing here is that the getContentDimensions call is forcing the
> reflow...

That would do it.  I presume you're talking about this JS, ultimately:
  995   _getContentDimensions: function() {
  996     return {
  997       width: content.document.body.scrollWidth,
  998       height: content.document.body.scrollHeight
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#995

That will indeed force reflow (via inspecting scrollWidth/scrollHeight).

(In reply to Chris Lord [:cwiiis] from comment #4)
> Just to note, I'd have suspected we could add a better method than
> getContentDimensions - if we've received the MozScrollAreaChanged signal, at
> some point a size has been calculated, and we're really only interested in
> that size.

Indeed. Good news (maybe) -- it looks like that size may already be sent here. Looks like the event is created & dispatched here:
  5281   InternalScrollAreaEvent event(true, NS_SCROLLEDAREACHANGED, nullptr);
  5285   event.mArea = mScrolledFrame->GetScrollableOverflowRectRelativeToParent();
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=6069c09d8f46#5281

Is event.mArea exposed to JS here, and can we just use that instead of calling getContentDimensions? (directing this question at cwiiis; feel free to redirect if someone else is working on this on the JS side)

Comment 7

[Daniel Holbert [:dholbert]]

Actually, it looks like the event exposed to JS is probably a ScrollAreaEvent:
 http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ScrollAreaEvent.webidl
which has "width" and "height" accessors, which I suspect are all we really want here.

Those are backed by the "mClientArea" member-var:
http://mxr.mozilla.org/mozilla-central/source/dom/events/ScrollAreaEvent.h?rev=bd079aadd3f&mark=57-59,62-64#57
...which is initialized from the "mRect" of a InternalScrollAreaEvent like the one I mentioned in comment 6:
http://mxr.mozilla.org/mozilla-central/source/dom/events/ScrollAreaEvent.cpp?rev=bd079aadd3fe&mark=18-18,22-22#16

Comment 8

[Chris Lord [:cwiiis]]

Taking, the width/height members of the event work fine and match the values we query for.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: 0001-Bug-1170644-Fix-forced-reflow-in-BrowserElementChild.patch

Use the width/height members of the event instead of getContentDimensions.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8631049 [details] [diff] [review]
0001-Bug-1170644-Fix-forced-reflow-in-BrowserElementChild.patch

Patch looks fine to me, but I'm not a /dom peer and I've never really looked at this file before last night, so I'm not sure I'm OK reviewing changes here.

Setting feedback+; maybe get official review from someone who reviews JS code inside /dom?

[also clearing needinfo for jet, since I think we're good here]

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8631049 [details] [diff] [review]
0001-Bug-1170644-Fix-forced-reflow-in-BrowserElementChild.patch

Would've picked Ehsan (as he reviewed the original bug 757859), but his username says he's not doing reviews at the moment - hopefully this falls under bz's domain?

ftr, it's quite a small patch and it really only affects FirefoxOS and its dynamic toolbar (which I've verified seems fine and works as it did before).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631049 [details] [diff] [review]
0001-Bug-1170644-Fix-forced-reflow-in-BrowserElementChild.patch

r=me

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Fix forced reflow in BrowserElementChild

HG-formatted patch, updated commit message. Carrying r+ and f+.

Comment 14

[Chris Lord [:cwiiis]]

Inbound is closed and I don't get back to work until Monday. My work laptop exploded a couple of weeks ago and I haven't gotten a replacement yet, so I won't be able to push this when the tree reopens later - so adding checkin-needed and hoping some kind soul will get to it before then :)

Comment 15

[Chris Lord [:cwiiis]]

Waddaya know, the tree reopened just in time. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/67f475daf729

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67f475daf729