Closed Bug 649142 Opened 13 years ago Closed 9 years ago

support logical box properties (-start/-end) without hidden longhand properties

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dbaron, Assigned: heycam)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, perf, regression, Whiteboard: [talos_regression])

Attachments

(11 files, 2 obsolete files)

3.24 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
27.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
32.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
74.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.43 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1014 bytes, patch
dbaron
: review+
Details | Diff | Splinter Review
12.49 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
25.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.45 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.41 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1022 bytes, patch
Unfocused
: review+
Details | Diff | Splinter Review
We should remove the *-value and *-source hidden longhand properties in our support for logical (-start/-end) box properties.  This depends on changing our declaration storage to preserve order.
Assignee: nobody → sjohnson
Priority: -- → P3
Assignee: jaywir3 → nobody
Assignee: nobody → cam
Here is the approach I'm going to use, using margin-* as an example:

1. Have real, longhand properties margin-{top,right,bottom,left,inline-start,inline-end}.
2. Remove the internal margin-{left,right,start,end}-value and
   margin-{left,right}-{ltr,rtl}-source properties.
3. Make -moz-margin-{start,end} be aliases for margin-inline-{start,end}.
4. Somehow, in nsCSSPropList.h, mark the logical properties as not needing storage in
   nsRuleData, and not contributing to the value returned by
   nsCSSProps::PropertyCountInStruct.
5. Add an argument to nsIStyleRule::MapRuleInfoInto for the computed value of direction
   (and later, writing-mode), which it would use (in conjunction with the order
   information we already store on a mozilla::css::Declaration) to select the right
   physical or logical properties to copy into the physical property slots of the
   nsRuleData.
6. nsRuleNode::ComputeMarginData would then not need to worry about logical
   properties at all.

I'll handle margin-block-{start,end} in a separate bug.

Does this sound about right David?
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
Yes, sounds right, modulo bug 649145 also being fixed to preserve order in declarations.
Flags: needinfo?(dbaron)
(In reply to Cameron McCormack (:heycam) from comment #1)
> 4. Somehow, in nsCSSPropList.h, mark the logical properties as not needing storage in
>    nsRuleData, and not contributing to the value returned by
>    nsCSSProps::PropertyCountInStruct.

Oh, and, in the worst case, "Somehow" could be with #ifdefs, if you can't find a better way.
We'll want to test that html.css with logical properties works as it does today, perhaps by running reftests with the logical html.css, with references that use the existing html.css.
Blocks: 1111440
Attachment #8539790 - Flags: review?(dbaron)
FYI, I'm going to put the new logical properties behind the vertical text pref.
Depends on: 1114400
Attached patch Part 8: Tests.Splinter Review
Attachment #8540528 - Flags: review?(dbaron)
Blocks: 1114875
Why do you need CSS_PROP_*_LOGICAL rather than just CSS_PROP_LOGICAL?

CSS_PROP_LOGICAL would need the explicit style struct parameter.

The only places that need CSS_PROP_struct are the *CheckCounter in nsCSSProps.cpp, and g*Flags in nsRuleNode.cpp.  I believe both of these should be excluding logical properties.

This would also have the advantage that you could implement CSS_PROP_LIST_EXCLUDE_LOGICAL by deciding whether to define CSS_PROP_LOGICAL to nothing or to CSS_PROP.  In fact, I'm actually inclined to say that maybe you should have a separate CSS_PROP_LIST_INCLUDE_LOGICAL for defining it to CSS_PROP, and thus require that exactly one of the three of CSS_PROP_LIST_INCLUDE_LOGICAL, CSS_PROP_LIST_EXCLUDE_LOGICAL, or CSS_PROP_LOGICAL is defined by the includer, which requires every includer to make a decision explicitly.
Flags: needinfo?(cam)
Comment on attachment 8540522 [details] [diff] [review]
Part 2: Convert logical properties to their physical equivalents during the cascade.

>+static bool
>+EnsurePhysicalProperty(nsCSSProperty& aProperty, nsRuleData* aRuleData)
>+{
>+  if (!nsCSSProps::PropHasFlags(aProperty, CSS_PROPERTY_LOGICAL)) {
>+    return false;
>+  }

I wonder if it's better for performance to have this check at the caller (and assert it inside)?

>+  uint8_t direction = aRuleData->mStyleContext->StyleVisibility()->mDirection;
>+  bool ltr = direction == NS_STYLE_DIRECTION_LTR;
>+
>+  switch (aProperty) {

I'm not crazy about this being a big switch statement; it seems like something that could be done from static data.  Probably ok for now, though.
Attachment #8540522 - Flags: review?(dbaron) → review+
Comment on attachment 8540523 [details] [diff] [review]
Part 3: Convert logical padding properties.

Per comment 16 and bug 1114400 comment 3, I think I want you to change
the macros, remove EnablingAlias, change the alias_for lines in
property_database.js, and perhaps a few other changes that follow from 
those.

>+  static const nsCSSProperty paddingProps[] = {

Use nsCSSProps::SubpropertyEntryFor(eCSSProperty_padding) instead (as
a |const nsCSSProperty* subprops|).

r=dbaron with that
Attachment #8540523 - Flags: review?(dbaron) → review+
Comment on attachment 8540524 [details] [diff] [review]
Part 4: Convert logical margin properties.

same as comment 18
Attachment #8540524 - Flags: review?(dbaron) → review+
Comment on attachment 8540525 [details] [diff] [review]
Part 5: Convert logical border properties.

Same as comment 18.

Also, the test_value_storage changes seem like they should be in a different (lower) patch.
Attachment #8540525 - Flags: review?(dbaron) → review+
Comment on attachment 8540526 [details] [diff] [review]
Part 6: Remove support for shorthand-implemented logical properties.

Please also remove CSSParserImpl::ParseDirectionalBorderSide.
Attachment #8540526 - Flags: review?(dbaron) → review+
Attachment #8540527 - Flags: review?(dbaron) → review+
Comment on attachment 8540521 [details] [diff] [review]
Part 1: Add macros and flags for defining logical properties and don't allocate storage for them in nsRuleData.

One other comment on patch 1 (in addition to comment 16):

>+// Check that all properties defined using CSS_PROP_*_LOGICAL macros use
>+// the CSS_PROPERTY_LOGICAL flag.
>+#define CSS_PROP(name_, id_, method_, flags_, pref_, parsevariant_,         \
>+                 kwtable_, stylestruct_, stylestructoffset_, animtype_)
>+#define CSS_PROP_LOGICAL(name_, id_, method_, flags_, pref_, parsevariant_, \
>+                         kwtable_, stylestruct_, stylestructoffset_,        \
>+                         animtype_)                                         \
>+  static_assert((flags_) & CSS_PROPERTY_LOGICAL,                            \
>+                "properties defined with CSS_PROP_*_LOGICAL must also use " \
>+                "the CSS_PROPERTY_LOGICAL flag");
>+#include "nsCSSPropList.h"
>+#undef CSS_PROP_LOGICAL
>+#undef CSS_PROP

You should assert the absence of the flag in the CSS_PROP here.
Attachment #8540528 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #20)
> Comment on attachment 8540525 [details] [diff] [review]
> Part 5: Convert logical border properties.
> 
> Same as comment 18.
> 
> Also, the test_value_storage changes seem like they should be in a different
> (lower) patch.

I'll drop that hunk as it isn't needed since making the -moz-padding-start etc. properties be the real properties and padding-inline-start etc. be aliases, per bug 1114400 comment 3.
Attachment #8540521 - Attachment is obsolete: true
Attachment #8540521 - Flags: review?(dbaron)
Flags: needinfo?(cam)
Attachment #8542135 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #17)
> Comment on attachment 8540522 [details] [diff] [review]
> Part 2: Convert logical properties to their physical equivalents during the
> cascade.
...
> I'm not crazy about this being a big switch statement; it seems like
> something that could be done from static data.  Probably ok for now, though.

Me neither.  I'll probably replace it with a table in bug 1114875.
(In reply to Cameron McCormack (:heycam) from comment #24)
> Created attachment 8542135 [details] [diff] [review]
> Part 1: Add macros and flags for defining logical properties and don't
> allocate storage for them in nsRuleData. (v2)

Oh, I guess I don't need the |#ifdef CSS_PROP_LIST_EXCLUDE_LOGICAL|s around the property definitions now that CSS_PROP_LOGICAL gets defined to nothing if CSS_PROP_LIST_EXCLUDE_LOGICAL was defined.
Comment on attachment 8542135 [details] [diff] [review]
Part 1: Add macros and flags for defining logical properties and don't allocate storage for them in nsRuleData. (v2)

> const nsStyleStructID nsCSSProps::kSIDTable[eCSSProperty_COUNT_no_shorthands] = {
>     // Note that this uses the special BackendOnly style struct ID
>     // (which does need to be valid for storing in the
>     // nsCSSCompressedDataBlock::mStyleBits bitfield).
>     #define CSS_PROP(name_, id_, method_, flags_, pref_, parsevariant_,     \
>                      kwtable_, stylestruct_, stylestructoffset_, animtype_) \
>         eStyleStruct_##stylestruct_,
>+    #define CSS_PROP_LIST_INCLUDE_LOGICAL
> 
>     #include "nsCSSPropList.h"
> 
>+    #undef CSS_PROP_LIST_EXCLUDE_LOGICAL
>     #undef CSS_PROP
> };

Your #undef should match your #define.

r=dbaron with that


And please do comment 28 (which applies to other patches).



Also, what happens with computed style for these properties?  I guess
there are two options:
 (1) report the empty string
 (2) report the computed value of the appropriate physical property
     based on direction and writing-mode


Sorry for not getting to this yesterday.
Attachment #8542135 - Flags: review?(dbaron) → review+
Comment on attachment 8542136 [details] [diff] [review]
Part 2.1: Test that logical properties in property_database.js are longhands.

r=dbaron... although maybe you want it to be ok to be a SHORTHAND_AND_LONGHAND if there's also an alias_for set?  (I'm not sure what you've done in the revisions of the higher patches, I guess.)
Attachment #8542136 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #29)
> Comment on attachment 8542135 [details] [diff] [review]
> Part 1: Add macros and flags for defining logical properties and don't
> allocate storage for them in nsRuleData. (v2)
...
> And please do comment 28 (which applies to other patches).

Done in my local patch revisions.

> Also, what happens with computed style for these properties?  I guess
> there are two options:
>  (1) report the empty string
>  (2) report the computed value of the appropriate physical property
>      based on direction and writing-mode

At the moment we do #1 for -moz-margin-start etc., by virtue of them not being in nsComputedDOMStylePropertyList.h, so I made the new properties behave the same way.  I like the idea of option #2, though.  I'll mail www-style about that, and handle it in a separate bug.

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #30)
> Comment on attachment 8542136 [details] [diff] [review]
> Part 2.1: Test that logical properties in property_database.js are longhands.
> 
> r=dbaron... although maybe you want it to be ok to be a
> SHORTHAND_AND_LONGHAND if there's also an alias_for set?  (I'm not sure what
> you've done in the revisions of the higher patches, I guess.)

I haven't stuck logical:true on the aliases so we don't need to do this.
Blocks: 1116638
see above backout note.

Also, could you file a followup blocking bug 1099032 to swap which ones are the primary properties (at the same time as landing bug 1099032).
Flags: needinfo?(cam)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #34)
> see above backout note.

Thanks, yeah, noticed that. :(  I'm waiting on service now to give me a new VMware licence key (since my old VMware broke after updating to OS X 10.10 over the break) to debug it...

> Also, could you file a followup blocking bug 1099032 to swap which ones are
> the primary properties (at the same time as landing bug 1099032).

Filed bug 1118103.
Attachment #8544392 - Flags: review?(dbaron) → review+
Depends on: 1120047
Maybe first try using try to bisect which changeset caused it?
So, some thoughts about the performance regression:

A simple increase in the number of properties is not a satisfactory explanation given that all of the increase was for this bug, and none was for the later bugs.

That makes me wonder if there are things that are looking for all of a set of properties to be filled in that are broken.  The most obvious, nsCSSProps::PropertyCountInStruct and its use in nsRuleNode::CheckSpecifiedProperties and other nsRuleNode functions, looks correct, though maybe worth double-checking.


Could having cases out-of-order in CSSParserImpl::ParsePropertyByFunction make a difference to code-generation?  (I noticed border-start.)


Perhaps more likely:  could the fact that we now fault out of the rule tree even when a logical property is *not* the winning declaration make a difference?  (Compare the added EnsurePhysicalProperty and its caller in part 2, versus the removed-in-part-6 AdjustLogicalBoxProp and its callers (removed in parts 3-5).  Then again, this is a little bit difficult to fix with more sides, since we need to check that the logical property is not the winning declaration for any writing-mode/direction.  (In the old code there were only 2 options; now there are more.)




One other thing I noticed looking at the patches:

in part 1, layout/style/test/ListCSSProperties.cpp has an incorrect #undef for the first #define/#undef pair.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #42)
> Perhaps more likely:  could the fact that we now fault out of the rule tree
> even when a logical property is *not* the winning declaration make a
> difference?  (Compare the added EnsurePhysicalProperty and its caller in
> part 2, versus the removed-in-part-6 AdjustLogicalBoxProp and its callers
> (removed in parts 3-5).  Then again, this is a little bit difficult to fix
> with more sides, since we need to check that the logical property is not the
> winning declaration for any writing-mode/direction.  (In the old code there
> were only 2 options; now there are more.)

It's actually not that hard given that EnsurePhysicalProperty is more table-driven as a result of the patches on top of this one.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #42)
> Could having cases out-of-order in CSSParserImpl::ParsePropertyByFunction
> make a difference to code-generation?  (I noticed border-start.)

This is unlikely to be a problem since there are plenty of existing cases out of order since the list of properties is a mix of shorthand and non-shorthand.
... which says that that's not the problem.
FWIW the performance regression comes in with part 5:

part 4 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=482644043a9a
part 5 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=dce333a3a905

which is the conversion of the border properties.
I wonder if it could be the switch statement in EnsurePhysicalProperty.  Although we turn into the table based lookup later on, we're still searching through it rather than doing direct lookups.
Didn't it get substantially larger after patch 5 in this bug (mostly in the other bugs)?  Would be hard to explain all of the regression being in patch 5.

(That said, if it's sorted, which I think it is, it could be a binary search.)
One possibility in patch 5 (and elsewhere):  nsRuleNode::HasAuthorSpecifiedRules should list in borderValues (etc.) all the properties that could lead to border being specified -- i.e., all the logical properties.  Has AuthorSpecifiedRules incorrectly returning false could lead to using native-theme drawing somewhere we weren't before, which would make painting more expensive.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #50)
> Didn't it get substantially larger after patch 5 in this bug (mostly in the
> other bugs)?  Would be hard to explain all of the regression being in patch
> 5.

Yeah, I suppose so.  I pushed this (only up to part 5 of this bug) and changed the switch statement to a direct array lookup, and it didn't help:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b227d9c9196
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #51)
> One possibility in patch 5 (and elsewhere): 
> nsRuleNode::HasAuthorSpecifiedRules should list in borderValues (etc.) all
> the properties that could lead to border being specified -- i.e., all the
> logical properties.  Has AuthorSpecifiedRules incorrectly returning false
> could lead to using native-theme drawing somewhere we weren't before, which
> would make painting more expensive.

Since we're looking over the nsRuleData's values to check for the border (and other) related properties after the MapRuleInfoInto calls (which will resolve the logical properties to physical ones; nsRuleData does not contain logical properties), I don't think need to look for the logical properties in here.
(In reply to Cameron McCormack (:heycam) from comment #54)
> Since we're looking over the nsRuleData's values to check for the border
> (and other) related properties after the MapRuleInfoInto calls (which will
> resolve the logical properties to physical ones; nsRuleData does not contain
> logical properties), I don't think need to look for the logical properties
> in here.

Ah, right.  And doing so gives fatal assertions.
I also tried going back to nsRuleData::ComputeBorderData having four calls to ValueForBorderLeftColor, ValueForBorderRightColor, etc., rather than looping over the subproperty table and using ValueFor, to no avail.
I was pointed towards https://wiki.mozilla.org/Buildbot/Talos/Profiling so I'll see if I can get any useful information out of that.  If not, then I'm not sure where to look next.  (Although I'll point out that these are only on Windows non-PGO builds, so nothing we'd ship to users.  On the other hand it means then we are relying on PGO doing its thing, and if later we somehow tickle PGO to behave differently, we might lose the gains we needed for these patches to be safe.)
Well, now I just received a regression email for Windows PGO:

Regression: Mozilla-Inbound - Tab Animation Test - WINNT 6.1 (ix) - 12.7% increase
----------------------------------------------------------------------------------
    Previous: avg 4.898 stddev 0.077 of 12 runs up to revision 63680efe6d55
    New     : avg 5.520 stddev 0.092 of 12 runs since revision cf403ce5f085
    Change  : +0.622 (12.7% / z=8.108)
    Graph   : http://mzl.la/1uhIQjQ

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=63680efe6d55&tochange=cf403ce5f085
thanks for jumping on this and finding the patch which caused the problem.  For the record this is on windows 7 and as you have noticed on pgo and non pgo.  There is a smaller windows 8 pgo only regression on TART, but I haven't determined if it is related or not.  As a heads up, that will probably require some additional pgo builds which will mean more data points and a few random talos emails as it fills in missing holes.
Blocks: 1122690
Keywords: perf, regression
Whiteboard: [talos_regression]
I couldn't get useful information out of profiles on try.  I tried running TART locally, on Linux -- not the platform with the regressions, but I thought it might still show some differences -- but I got similar looking times.
you can do this on try server with:
try: -b o -p win32 -u none -t svgr mozharness: --spsProfile
Thanks, unfortunately I got oranges with this; mstange tried helping me with some patches to get it to work but we ran into issues with the profiles getting too large.
I've discovered that this bug fixes bug 621351, and so now some transitions that weren't possible before are now.  So I'm wondering now whether we're running some transitions (or animations?) on some border properties.
Should be testable with some printfs of what we're animating/transitioning.
Yeah, trying that now (though I only am logging transitions): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e184a0238440

As well as changing these properties to be unanimatable (with and without this bug's patches):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e268f88cb9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6879d68764
(In reply to Cameron McCormack (:heycam) from comment #66)
> Yeah, trying that now (though I only am logging transitions):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e184a0238440

This showed a bunch of transitions on border-right-color.

> As well as changing these properties to be unanimatable (with and without
> this bug's patches):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e268f88cb9
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6879d68764

Disabling animation of border left/right properties resulted in a normal-looking result.
The transition that is causing the slow down is on the bookmark toolbar icon (the star).  The Windows browser.css has a rule that sets its -moz-border-end to none.  During the tart test, there is a lot of switching back and forth between the main tart page and a newly opened tab.  On the main tart page, the bookmark toolbar button is enabled, but on the new tab page it isn't.  While -moz-border-end:none is the winning rule in both cases (icon on an enabled and disabled button), the color property is changed.  Since -moz-border-end:none will set -moz-border-end-color to currentColor, we try to transition the none-style border's colour.

We could fix this either by making the rule use a consistent colour value, such as "-moz-border-end: none transparent", or by changing it to "-moz-border-end-style: none" and leaving the earlier-in-the-cascade rule that sets "border: 1px solid transparent" to set the colour.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8555034 [details] [diff] [review]
Part 9: Avoid transitioning border-end-color on the bookmark toolbar icon when switching between enabled and disabled states.

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

The Windows 7 results are still pending, and I suspect we'll see a difference there. See:
https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css?#714
... where we use a different border colour for Windows Vista and Windows 7.
Attachment #8555034 - Flags: review?(bmcbride) → review-
Windows 7 turned out OK on Talos, and the existing -moz-border-end:none rule overrides the Win7-specific rule you point out.  I checked locally and the transition doesn't run with this patch on Win7.  So I don't think we need any other changes.
Flags: needinfo?(bmcbride)
Ah! Well, in that case then...
Flags: needinfo?(bmcbride)
Attachment #8555034 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/a80438019d25
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
See Also: → 1132859
You need to log in before you can comment on or make changes to this bug.