Closed Bug 1170644 Opened 5 years ago Closed 5 years ago

BrowserElementChild causes sync reflow on startup

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: wilsonpage, Assigned: cwiiis)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

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
Component: Gaia → DOM
Product: Firefox OS → Core
Flags: needinfo?(dale)
Attached file profile-2.json
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?
Flags: needinfo?(dale) → needinfo?(chrislord.net)
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.
Flags: needinfo?(chrislord.net)
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.
Blocks: 1164539
Jet/Daniel: Who can help with this? Please also see bug: 1164539 

Thanks
Hema
Flags: needinfo?(dholbert)
Flags: needinfo?(bugs)
(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)
Flags: needinfo?(dholbert) → needinfo?(chrislord.net)
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
Taking, the width/height members of the event work fine and match the values we query for.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(chrislord.net)
Use the width/height members of the event instead of getContentDimensions.
Attachment #8631049 - Flags: review?(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]
Flags: needinfo?(bugs)
Attachment #8631049 - Flags: review?(dholbert) → feedback+
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).
Attachment #8631049 - Flags: review?(bzbarsky)
Blocks: 757859
Comment on attachment 8631049 [details] [diff] [review]
0001-Bug-1170644-Fix-forced-reflow-in-BrowserElementChild.patch

r=me
Attachment #8631049 - Flags: review?(bzbarsky) → review+
HG-formatted patch, updated commit message. Carrying r+ and f+.
Attachment #8631049 - Attachment is obsolete: true
Attachment #8631173 - Flags: review+
Attachment #8631173 - Flags: feedback+
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 :)
Keywords: checkin-needed
Waddaya know, the tree reopened just in time. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/67f475daf729
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67f475daf729
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.