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.

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.

(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

(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.

(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".

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.

Updated patch 2 to match the changes in patch 1.

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);

https://hg.mozilla.org/integration/mozilla-inbound/rev/8406a2330ff2 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9012207555

https://hg.mozilla.org/mozilla-central/rev/8406a2330ff2 https://hg.mozilla.org/mozilla-central/rev/3d9012207555