My case is Homescreen in firefox os, and the scroll property is overflow-y: visible, overflow-x: hidden. When dragging an item around the left/right side of the frame, we will do reflow thought the overflow-x is hidden. I think we could try to avoid the reflow.

Attachment is an example which overflow-x is hidden and has a draggable item.

In this patch, I add XY different property for frame. When set the child overflow area, update the change is X or Y. And update the information to it's parent in RestyleTracker::Flush(). Eventually, we can get the x,y change information at ScrollFrameHelper::UpdateOverflow(), and we can avoid the reflow for certain cases(like x has change but overflow-x: hidden). Another way is that we still calculate the xy change in child, but check these information at RestyleTracker::Flush(). When child changed, we check the nearest scrollable frame's overflow property to decide whether we need to pass the change to parent. This way can save more cpu power but I am not sure if it is safe.

(Note: The majority of this patch is actually just whitespace fixes, which were likely done by your editor or by an hg extension automatically, and then inadvertantly saved into your patch. This makes it harder for reviewers and future hg archeologists to see what's actually changing in your patch. You should probably revert those & [if you like] fix them in their own patch, and disable whatever editor / hg feature is triggering them, so they don't bulk up your future patches in the same way.)

Comment on attachment 8554443 [details] [diff] [review] bug_1125750.patch So it looks like this is setting a frame property on all frames, when you'd be better off setting a property or a member variable only on scrollframes, since that's where you need it. This is for two reasons: first, frame properties are for things that are rarely set; they take up more space than the equivalent member variable, but take up space only while present. Second, if you only need this for scrollframes, you shouldn't go through the trouble of allocating the property everywhere. (It's also possible it should be a member variable instead.) It's also not clear to me that the approach you're taking here accumulates multiple changes correctly, but I don't think that's worth examining closely until you do something that only applies to scrollframes. (I suspect it might make more sense to store the last overflow area used rather than booleans; that seems likely to handle multiple changes better.) Also, in general, I prefer avoiding std::pair. If you want a struct with two member variables, write your struct type, and give them useful names. And, as dholbert said, the whitespace changes get in the way of review.

To dbaron, Thanks for your feedback. I remove the properties and add a flag in scrollable frame. Child frame will update the flag when updating overflow region.

Comment on attachment 8555055 [details] [diff] [review] Part1 - Check the change of overflow region in scrollable frame Review of attachment 8555055 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +7144,5 @@ > nsOverflowAreas *overflow = > static_cast<nsOverflowAreas*>(Properties().Get(OverflowAreasProperty())); > bool changed = *overflow != aOverflowAreas; > + if (changed) { > + nsIScrollableFrame* scrollableFrame = do_QueryFrame(nsLayoutUtils::GetCrossDocParentFrame(this)); Are you sure parent frame is always scrollable frame? @@ +7201,5 @@ > + flag = nsIScrollableFrame::HORIZONTAL; > + } > + if (mOverflow.mVisualDeltas.mTop != oldDeltas.mTop || > + mOverflow.mVisualDeltas.mBottom != oldDeltas.mBottom) { > + flag = nsIScrollableFrame::VERTICAL; Should be |= @@ +7225,5 @@ > + uint32_t flag = (xChange ? nsIScrollableFrame::HORIZONTAL : 0) | > + (yChange ? nsIScrollableFrame::VERTICAL : 0); > + scrollableFrame->SetOverflowChange(flag); > + } > + } You should create a utility function to prevent code duplication. ::: layout/generic/nsGfxScrollFrame.cpp @@ +4649,5 @@ > + if (ss.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN && > + mOverflowChange & nsIScrollableFrame::HORIZONTAL) { > + needReflow = true; > + } > + if (ss.mHorizontal == NS_STYLE_OVERFLOW_HIDDEN && scrollPosition.x) { No need to check NS_STYLE_OVERFLOW_HIDDEN here. @@ +4656,5 @@ > + if (ss.mVertical != NS_STYLE_OVERFLOW_HIDDEN && > + mOverflowChange & nsIScrollableFrame::VERTICAL) { > + needReflow = true; > + } > + if (ss.mVertical == NS_STYLE_OVERFLOW_HIDDEN && scrollPosition.y) { Ditto.

Apply Morris's feedback. About the check of ScrollFrameHelper::UpdateOverflow(), original logic is always reflow when the position is not zero. In my patch, I add a check to make sure the direction of scroll position agrees with the direction of overflow change, though I am not sure if it is correct.

Fix typo.

Comment on attachment 8555153 [details] [diff] [review] Part1 - Check the change of overflow region in scrollable frame This seems more complicated than it needs to be. There should be only two places that change the overflow area of a scroll frame for each type of scroll frame: (1) nsHTMLScrollFrame::Reflow or something that it calls nsXULScrollFrame::Layout or something that it calls (2) ScrollFrameHelper::UpdateOverflow You're concerned about optimizing the path in (2), but you might need to know the previous state in addition to the current state, so you might (or might not) need to record that in both (1) and (2). But I don't understand why there are so many changes in this patch. Why can't you just do what you need in UpdateOverflow, plus some simple recording of what the previous state was?

Remove the child frame check and just check the overflow area in the ScrollFrameHelper.

Comment on attachment 8557849 [details] [diff] [review] Part1 - Check the change of overflow region in scrollable frame This looks much closer. Rather than operating on nsOverflowAreas objects when you only need the visual overflow, you should pass around and store (in the new member variable) only the visual overflow. You also need to set the new member variable in nsHTMLScrollFrame::Reflow and nsXULScrollFrame::Layout. GetOverflowChange needs to initialize flag to 0. It should also probably use |= for both HORIZONTAL and VERTICAL, for clarity. And it should probably be called result. (Or, if you don't like that, at least flags rather than flag.) >+ nsLayoutUtils::UnionChildOverflow(mOuter, overflowAreas); I'd have expected you to pass mScrolledFrame, not mOuter. Passing mOuter will give you the result that nothing ever changes. That said, you don't need to call UnionChildOverflow at all. You can just call GetScrolledRect(), which will also optimize away overflow changes on the side that we don't allow scrolling to (in LTR text, overflow to the top and the left). Could you also explain what you've done to test that this patch is correct? There should be automated tests that fail due to the above mistakes; if there aren't, you should add some.

I use GetScrolledRect to replace calculation of overflow region and apply other recommends in this patch. About the mOuter, actually I can get the change of overflow area in the last patch. I didn't send the last patch to tryserver but I did some local check before upload to bugzilla. The tryserver result of this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fdd61bddcba

(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #11) > Could you also explain what you've done to test that this patch is > correct? There should be automated tests that fail due to the above > mistakes; if there aren't, you should add some. (In reply to Ethan Lin[:Ethan] from comment #12) > I didn't send the last patch to > tryserver but I did some local check before upload to bugzilla. Were there existing tests that failed with the previous patch? If not, you should add some. (Catching mistakes like that in code review is a failure of the system; they should have been caught sooner.)

Comment on attachment 8558442 [details] [diff] [review] Part1 - Check the change of overflow region in scrollable frame >+GetOverflowChange(const nsRect& curRect, const nsRect& prevRect) Please call these aCurScrolledRect and aPrevScrolledRect to say what they actually are. (You'll need to break the lines after the || since they'll be longer, but that's fine.) >+ // Check the change of overflow area You're not checking the overflow are. But the comment should also say why it's doing what it's doing, so perhaps: // Reflow when the change in overflow leads to one of our scrollbars // changing or might require repositioning the scrolled content due to // reduced extents. r=dbaron with those changes, and with added tests that the previous patch fails if the previous patch didn't lead to any existing failures

(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #11) > Could you also explain what you've done to test that this patch is > correct? There should be automated tests that fail due to the above > mistakes; if there aren't, you should add some. In particular, I would expect tests to fail as a result of passing mOuter to ConsiderChildOverflow.

I apply the comment and rename variables in this patch.

Break the line that's too long. About the auto test, I'll check if there is any existed test.

Was there an existing test that fails? If not, do you need help writing a new one?

I tried to use mOuter to replace mScrolledFrame. There is no existing test error. The tryserver result is as follow: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8be1bf76a805 For now I am studying the difference between mOuter and mScrolledFrame. I will write a new test if needed.

I add a reftest which can detect the mOuter problem. When moving an element to the edge of a scrollable frame (overflow:auto), the scroll bar should appear. If using mOuter to calculate the overflow area, then the scroll bar will not appear.

Here is the try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b62fb4f42d5

Comment on attachment 8571758 [details] [diff] [review] Part2 - Add a reftest It's probably better to use transform: translateX() translateY() in the reference rather than using left/top, so that the test and reference match more closely other than regarding dynamic change vs. static setup. You could also compress the translation to translate(80px, 80px). r=dbaron with that

I apply dbaron's comments to this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/319d452a7049 https://hg.mozilla.org/integration/mozilla-inbound/rev/fbafae134491

https://hg.mozilla.org/mozilla-central/rev/319d452a7049 https://hg.mozilla.org/mozilla-central/rev/fbafae134491

Ethan, I would suggest to land this on b2g37. Any concern for it?

Peter, I have no concern to land this on b2g37.

Comment on attachment 8561166 [details] [diff] [review] Part1 - Check the change of overflow region in scrollable frame. r=dbaron NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): none User impact if declined: It's possible to get lower performance when moving an element in a scrollable frame Testing completed: Try is positive on master try Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: not available

Comment on attachment 8572311 [details] [diff] [review] Part2 - Add a reftest. r=dbaron NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): none User impact if declined: We may miss some possible problems of scroll frame. Testing completed: Try is positive on master try Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: not available

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/30a9e827a04e https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b0a59fd87635