Last Comment Bug 894716 - Parameterize nsLayoutUtils::GetNearestScrollableFrame
: Parameterize nsLayoutUtils::GetNearestScrollableFrame
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Corey Ford [:coyotebush]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-16 17:40 PDT by Corey Ford [:coyotebush]
Modified: 2013-07-31 17:52 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Parameterize nsLayoutUtils::GetNearestScrollableFrame. (5.61 KB, patch)
2013-07-16 18:08 PDT, Corey Ford [:coyotebush]
dbaron: review-
Details | Diff | Splinter Review
Parameterize nsLayoutUtils::GetNearestScrollableFrame (v2, flags) (5.78 KB, patch)
2013-07-27 11:35 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Unify nsLayoutUtils::GetNearestScrollableFrame{,ForDirection} with extra parameters (15.85 KB, patch)
2013-07-29 11:28 PDT, Corey Ford [:coyotebush]
dbaron: feedback-
Details | Diff | Splinter Review
Parameterize nsLayoutUtils::GetNearestScrollableFrame. (5.83 KB, patch)
2013-07-30 21:02 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review

Description Corey Ford [:coyotebush] 2013-07-16 17:40:51 PDT
nsLayoutUtils::GetNearestScrollableFrame is used by nsPresShell.cpp and AnimationCommon.cpp. It "locates the first ancestor of aFrame (or aFrame itself) that is scrollable with overflow:scroll or overflow:auto in some direction. The search extends across document boundaries."

nsFrame::Handle{Press,Drag} each have a similar loop to find the nearest scrollable ancestor, except that they don't cross document boundaries and don't exclude overflow:hidden elements.

The latter behavior is probably what we want for bug 886646, and I think a good approach might be to add flag parameters to GetNearestScrollableFrame to toggle those two constraints.
Comment 1 Corey Ford [:coyotebush] 2013-07-16 18:08:29 PDT
Created attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

Something like this.

https://tbpl.mozilla.org/?tree=Try&rev=2522d6aa9275
Comment 2 Corey Ford [:coyotebush] 2013-07-22 18:28:28 PDT
Comment on attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

I might as well actually think about landing this. I'd consider inverting the sense of either of the booleans; it's a little weird that the two current use cases happen to be (true, false) and (false, true), and while they're both positively worded, one lets the search continue longer while the other lets it stop sooner.

And yes, it turned a bit messy to get the nsIFrame* version back out in HandlePress.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-25 18:08:54 PDT
Comment on attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

So rather than using sequences of boolean parameters (which made the calling code unreadable), we tend to use bitfields.  For an example, see nsLayoutUtils::PaintFrame.

I think I'd be inclined to invert the sense of the hidden bit -- make it a bit for skipping hidden.

I'd also prefer keeping the for() structure rather than switching to while() -- just use ?: inside the last part of the for statement.

And avoid default parameters unless there's a good reason for them.

Otherwise, I think this sounds good, though, but I'd like to see a patch with those changes, so marking as review-.
Comment 4 Corey Ford [:coyotebush] 2013-07-26 11:28:36 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> I think I'd be inclined to invert the sense of the hidden bit -- make it a
> bit for skipping hidden.

Sounds fine, although I realize that if we instead go with SAME_DOC and INCLUDE_HIDDEN flags, the default behavior is then identical to the original (and to GetNearestScrollableFrameForDirection).
Comment 5 Corey Ford [:coyotebush] 2013-07-27 11:35:46 PDT
Created attachment 782183 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame (v2, flags)

So if we were to use CROSS_DOC and SKIP_HIDDEN flags, I dislike the resulting asymmetry of the calls in nsIPresShell::GetFrameToScrollAsScrollable.

Another approach might be to unify this method with GetNearestScrollableFrameForDirection (whose only caller is nsIPresShell::GetFrameToScrollAsScrollable). The resulting logic would then resemble their ancestor GetNearestScrollingView (see bug 526394, part 5).
Comment 6 Corey Ford [:coyotebush] 2013-07-29 11:28:51 PDT
Created attachment 782677 [details] [diff] [review]
Unify nsLayoutUtils::GetNearestScrollableFrame{,ForDirection} with extra parameters

That approach would look like this. (still would need to figure out where best to declare/include the enum, and that enormous if condition could maybe be formulated better).

Feel free to just r+ the other patch, though, and we'll go with that...
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-30 18:23:26 PDT
Comment on attachment 782677 [details] [diff] [review]
Unify nsLayoutUtils::GetNearestScrollableFrame{,ForDirection} with extra parameters

I agree with you about the asymmetry in nsIPresShell::GetFrameToScrollAsScrollable, but I think a cleaner way to fix it, instead of adding an additional parameter to nsLayoutUtils::GetNearestScrollableFrame, would be to add two new flag bits to represent the two direction requirements in GetNearestScrollableFrameForDirection.  Except I also think the check it's doing is actually substantially different -- it's checking scrollbar visibility, which is something that's seems to me odd to check.  I'm inclined to suggest leaving the functions separate, and just going with cross-doc by default, then.  (Though I'm curious how much of the difference in terms of checking GetPerceivedScrollingDirections was really intentional.)
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-30 18:25:47 PDT
Comment on attachment 782183 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame (v2, flags)

>+    nsIPresShell::SetCapturingContent(((nsIFrame*)do_QueryFrame(scrollFrame))->
>+                                      GetContent(), CAPTURE_IGNOREALLOWED);

Put the nsIFrame* in a variable (no cast) rather than doing this cast (and a C-style cast, ugh) inline.

r=dbaron with that
Comment 9 Corey Ford [:coyotebush] 2013-07-30 18:26:43 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> (Though I'm curious how much of the difference
> in terms of checking GetPerceivedScrollingDirections was really intentional.)

Incidentally, GetNearestScrollingView had logic like
 (aDirection == eEither || totalHeight > visibleSize.height || margin.LeftRight()))
Comment 10 Corey Ford [:coyotebush] 2013-07-30 21:02:31 PDT
Created attachment 783523 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

Done. And realized I had removed an if statement from the logic of the original.
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-31 06:31:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/710c7f7c8e4c
Comment 12 Wes Kocher (:KWierso) 2013-07-31 17:52:12 PDT
https://hg.mozilla.org/mozilla-central/rev/710c7f7c8e4c

Note You need to log in before you can comment on or make changes to this bug.