Last Comment Bug 893962 - Refactor the application of relative positioning
: Refactor the application of relative positioning
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Corey Ford [:coyotebush]
:
Mentors:
Depends on:
Blocks: 886646 896548 898794 898797
  Show dependency treegraph
 
Reported: 2013-07-15 12:04 PDT by Corey Ford [:coyotebush]
Modified: 2013-07-27 18:45 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactor the application of relative positioning (v2, static method) (11.79 KB, text/plain)
2013-07-15 12:04 PDT, Corey Ford [:coyotebush]
no flags Details
Refactor the application of relative positioning (v2, static method) (11.79 KB, patch)
2013-07-15 12:05 PDT, Corey Ford [:coyotebush]
dbaron: review-
Details | Diff | Splinter Review
Refactor the application of relative positioning. (10.19 KB, patch)
2013-07-15 16:08 PDT, Corey Ford [:coyotebush]
dbaron: review+
Details | Diff | Splinter Review
Refactor the application of relative positioning. (10.22 KB, patch)
2013-07-15 16:53 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review
Refactor the application of relative positioning. (10.22 KB, patch)
2013-07-15 17:28 PDT, Corey Ford [:coyotebush]
corey: review+
Details | Diff | Splinter Review

Description Corey Ford [:coyotebush] 2013-07-15 12:04:29 PDT
Created attachment 775840 [details]
Refactor the application of relative positioning (v2, static method)

(Previous discussion in bug 886646 comment 9 and following)

CSS relative positioning is applied separately in several places: nsBlockReflowContext, nsCanvasFrame, nsFlexContainerFrame, and nsLineLayout.

This should be factored out to a separate method, and sticky positioning can later be added there.

Implementing this in a non-static method on nsHTMLReflowState works well except for the case of nsLineLayout. Using a static method and carrying along necessary bits of state works, but seems a bit messy.
Comment 1 Corey Ford [:coyotebush] 2013-07-15 12:05:16 PDT
Created attachment 775841 [details] [diff] [review]
Refactor the application of relative positioning (v2, static method)
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-15 15:54:34 PDT
Comment on attachment 775841 [details] [diff] [review]
Refactor the application of relative positioning (v2, static method)

>-  nsContainerFrame::FinishReflowChild(mFrame, mPresContext, &aReflowState, mMetrics, x, y, 0);
>+  nsContainerFrame::FinishReflowChild(mFrame, mPresContext, &aReflowState, mMetrics, position.x, position.y, 0);

This line should get wrapped at slightly under 80 characters.

What I realized I should have suggested is that you could have two methods, a static method and a non-static method, where the non-static method is just an inline function that calls the static one with the right parameters.  That would be slightly preferable, if you don't mind -- it means changing a bit of stuff back to the way it was in the old patch.  Sorry I wasn't clear about that the last time around.

Also, in nsLineLayout, don't add an mStyleDisplay; instead, just use pfd->mFrame->StyleDisplay().
Comment 3 Corey Ford [:coyotebush] 2013-07-15 16:08:54 PDT
Created attachment 776014 [details] [diff] [review]
Refactor the application of relative positioning.

Yeah, that looks nicer.

try push: https://tbpl.mozilla.org/?tree=Try&rev=11b2cf5448fd
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-15 16:32:20 PDT
Comment on attachment 776014 [details] [diff] [review]
Refactor the application of relative positioning.

>+  nsContainerFrame::FinishReflowChild(mFrame, mPresContext, &aReflowState,
>+    mMetrics, position.x, position.y, 0);

Line up mMetrics with mFrame on the previous line.

>+  // If a relatively positioned element, adjust the position appropriately.
>+  static void ApplyRelativePositioning(nsPoint *aPosition,
>+                                       const nsStyleDisplay* aDisplay,
>+                                       const nsMargin& aComputedOffsets);

I might slightly prefer putting aPosition as the last parameter rather than the first, just because we have a tendency towards putting things that are output parameters (or in this case, in-out params) later in the parameter list (as a result of the XPCOM->C++ convention of having the return value, and thus out-param, as the last C++ argument).

It's ok to leave that as-is, though.

(Neither change should require a new try run; just check that it still compiles.)

r=dbaron
Comment 5 Corey Ford [:coyotebush] 2013-07-15 16:53:55 PDT
Created attachment 776043 [details] [diff] [review]
Refactor the application of relative positioning.

Done.
Comment 6 Corey Ford [:coyotebush] 2013-07-15 17:28:17 PDT
Created attachment 776061 [details] [diff] [review]
Refactor the application of relative positioning.
Comment 7 Corey Ford [:coyotebush] 2013-07-15 17:28:50 PDT
Comment on attachment 776061 [details] [diff] [review]
Refactor the application of relative positioning.

Fixed some pointer stars.
Comment 8 Corey Ford [:coyotebush] 2013-07-15 18:44:30 PDT
Tests are all green on try.
Comment 9 Matthew N. [:MattN] 2013-07-15 21:25:42 PDT
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/556a557c7276
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-07-16 13:35:08 PDT
https://hg.mozilla.org/mozilla-central/rev/556a557c7276
Comment 11 Corey Ford [:coyotebush] 2013-07-18 10:50:37 PDT
Hmm, we also make various use of the computed offsets via nsIFrame::GetRelativeOffset after storing them in ComputedOffsetProperty.

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