Closed Bug 1159305 Opened 5 years ago Closed 5 years ago

provide logical accessors for nsStyleCoord values in nsStylePosition and nsStyleSides

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Layout code working with logical coordinates gets messy when it needs to consult various fields of nsStylePosition that are stored in physical terms. A set of logical accessors that take a writing mode and map to the appropriate physical fields will make for much cleaner, more maintainable code.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
This patch uses the new accessors to simplify some of the code we've already landed; they'll also be really helpful, I think, when converting things like absolute positioning code.
Attachment #8598730 - Flags: review?(smontagu)
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=eafa9979a022

Note that the reftest failures in this run are due to a bad patch from bug 1157951 that was in my tree at the time, not the patches here.
Comment on attachment 8598729 [details] [diff] [review]
patch 1 - Provide logical accessors for nsStylePosition and nsStyleSides fields

Maybe it's clearer to implement these:

>+inline nsStyleUnit nsStyleSides::GetUnit(mozilla::WritingMode aWM,
>+                                         mozilla::LogicalSide aSide) const


>+inline nsStyleCoord nsStyleSides::Get(mozilla::WritingMode aWM,
>+                                      mozilla::LogicalSide aSide) const

by calling the physical versions rather than by reimplementing the
physical versions?  The current thing seems like it could trip up
somebody making future modifications, especially given the header
file gymnastics.


For the nsStylePosition accessors, is there something better
we could do than passing aVertical as a bool?  Might it be cleaner
to just take a WritingMode?

Could you also add to the existing FIXME comment
above HeightDependsOnContainer, perhaps suggesting that the
path forward on that is to remove the physical versions and have
only the logical ones?

r=dbaron with that
Attachment #8598729 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⏰UTC+2 from comment #7)
> Comment on attachment 8598729 [details] [diff] [review]
> For the nsStylePosition accessors, is there something better
> we could do than passing aVertical as a bool?  Might it be cleaner
> to just take a WritingMode?

I passed it as a bool to avoid repeatedly doing the bit test for IsVertical(), in the (common) case where we want to access several of these things from the same caller. But perhaps it's not worth this micro-optimization (or perhaps the compiler does just as well by itself). I'll change it to a WritingMode if you prefer that for clarity? Please confirm which you'd like.
Flags: needinfo?(dbaron)
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> (In reply to David Baron [:dbaron] ⏰UTC+2 from comment #7)
> > Comment on attachment 8598729 [details] [diff] [review]
> > For the nsStylePosition accessors, is there something better
> > we could do than passing aVertical as a bool?  Might it be cleaner
> > to just take a WritingMode?
> 
> I passed it as a bool to avoid repeatedly doing the bit test for
> IsVertical(), in the (common) case where we want to access several of these
> things from the same caller. But perhaps it's not worth this
> micro-optimization (or perhaps the compiler does just as well by itself).
> I'll change it to a WritingMode if you prefer that for clarity? Please
> confirm which you'd like.

Oh, the other reason I didn't pass the WritingMode is that if we do that, we'll need to move their definitions to WritingModes.h (like I had to for the StyleSides ones). But I suppose if we're doing it for one of these structs, we may as well do the other too. I'll go ahead and do that, unless you dislike it due to the extra "header file gymnastics".
Flags: needinfo?(dbaron)
I think it's probably better; booleans make dangerous function parameters.

The header file stuff bothers me more when it's possible for an inline method to have its declaration included without its definition included, but that's not the case here, since a caller would have to have a WritingMode object, which means they have to have included WritingModes.h.  The remaining problem of the code being in odd places can be addressed by good comments.
er, an inline method **that is being called**
Just updating patch 1 per comments above; carrying over r=dbaron.
Attachment #8598729 - Attachment is obsolete: true
Updated patch 2 to match the changes in patch 1.
Attachment #8601359 - Flags: review?(smontagu)
Attachment #8598730 - Attachment is obsolete: true
Attachment #8598730 - Flags: review?(smontagu)
Comment on attachment 8601359 [details] [diff] [review]
patch 2 - Clean up layout code by using the new accessors

Review of attachment 8601359 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4167,5 @@
>      NS_ASSERTION(bSizeCoord.GetUnit() == eStyleUnit_Auto,
>                   "Unexpected block-size unit for viewport or canvas or page-content");
>      // For the viewport, canvas, and page-content kids, the percentage
>      // basis is just the parent height.
> +    h = f->GetLogicalSize(wm).BSize(wm);

h = f->BSize(wm);
Attachment #8601359 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/8406a2330ff2
https://hg.mozilla.org/mozilla-central/rev/3d9012207555
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.