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

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron:]

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.

Comment 1

[Cameron McCormack (:heycam)]

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?

Comment 2

[David Baron :dbaron:]

Yes, sounds right, modulo bug 649145 also being fixed to preserve order in declarations.

Comment 3

[David Baron :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.

Comment 4

[Jet Villegas (inactive)]

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.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add macros and flags for defining logical properties and don't allocate storage for them in nsRuleData.

Comment 6

[Cameron McCormack (:heycam)]

FYI, I'm going to put the new logical properties behind the vertical text pref.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add macros and flags for defining logical properties and don't allocate storage for them in nsRuleData.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 2: Convert logical properties to their physical equivalents during the cascade.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 3: Convert logical padding properties.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 4: Convert logical margin properties.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Part 5: Convert logical border properties.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Part 6: Remove support for shorthand-implemented logical properties.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 7: Move CSS_PROPERTY_LOGICAL up into one of the earlier free bits.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 8: Tests.

Comment 15

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52ee8799abe0

Comment 16

[David Baron :dbaron:]

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.

Comment 17

[David Baron :dbaron:]

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.

Comment 18

[David Baron :dbaron:]

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

Comment 19

[David Baron :dbaron:]

Comment on attachment 8540524 [details] [diff] [review]
Part 4: Convert logical margin properties.

same as comment 18

Comment 20

[David Baron :dbaron:]

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.

Comment 21

[David Baron :dbaron:]

Comment on attachment 8540526 [details] [diff] [review]
Part 6: Remove support for shorthand-implemented logical properties.

Please also remove CSSParserImpl::ParseDirectionalBorderSide.

Comment 22

[David Baron :dbaron:]

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.

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add macros and flags for defining logical properties and don't allocate storage for them in nsRuleData. (v2)

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 2.1: Test that logical properties in property_database.js are longhands.

Comment 26

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7ad55bdf5a7

Comment 27

[Cameron McCormack (:heycam)]

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

Comment 28

[Cameron McCormack (:heycam)]

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

[David Baron :dbaron:]

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.

Comment 30

[David Baron :dbaron:]

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

Comment 31

[Cameron McCormack (:heycam)]

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

Comment 32

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0eb691d6695
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5725cd39a31
https://hg.mozilla.org/integration/mozilla-inbound/rev/1335630cf287
https://hg.mozilla.org/integration/mozilla-inbound/rev/35c42cd138e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/15ed55c61f4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/67748675e669
https://hg.mozilla.org/integration/mozilla-inbound/rev/69ddb2036c50
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0252d2620d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/936703c75200

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c43c3f8398 for https://treeherder.mozilla.org/logviewer.html#?job_id=5012184&repo=mozilla-inbound

Comment 34

[David Baron :dbaron:]

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

Comment 35

[Cameron McCormack (:heycam)]

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

Comment 36

[Cameron McCormack (:heycam)]

Attachment: Part 1.1: Assert that CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED is not used on logical properties.

Per bug 1083134 comment 31.

Comment 37

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3644e19027a3

Comment 38

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3530d66d2c99
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95188620e45
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0de22d0c46
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9d2d244af9
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e2ac9ec1fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c08e19efe6
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c76e4fa0c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb1cfa7bf79
https://hg.mozilla.org/integration/mozilla-inbound/rev/a66355d68d36
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d7fe12db6c

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3530d66d2c99
https://hg.mozilla.org/mozilla-central/rev/b95188620e45
https://hg.mozilla.org/mozilla-central/rev/fd0de22d0c46
https://hg.mozilla.org/mozilla-central/rev/ec9d2d244af9
https://hg.mozilla.org/mozilla-central/rev/29e2ac9ec1fa
https://hg.mozilla.org/mozilla-central/rev/53c08e19efe6
https://hg.mozilla.org/mozilla-central/rev/89c76e4fa0c4
https://hg.mozilla.org/mozilla-central/rev/3eb1cfa7bf79
https://hg.mozilla.org/mozilla-central/rev/a66355d68d36
https://hg.mozilla.org/mozilla-central/rev/54d7fe12db6c

Comment 40

[Cameron McCormack (:heycam)]

Probably have to back this out for a Talos regressions:

http://graphs.mozilla.org/graph.html#tests=[[293,131,25]]&sel=1421385440000,1421558240000&displayrange=7&datatype=geo
http://graphs.mozilla.org/graph.html#tests=[[293,131,31]]&datatype=geo&sel=1421385880000,1421558680000

Comment 41

[David Baron :dbaron:]

Maybe first try using try to bisect which changeset caused it?

Comment 42

[David Baron :dbaron:]

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.

Comment 43

[David Baron :dbaron:]

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

Comment 44

[David Baron :dbaron:]

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

Comment 45

[David Baron :dbaron:]

I did a try push to see if it helps:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a279a007dc4

Comment 46

[David Baron :dbaron:]

And the point of comparison for that try push is:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8678824be3ab

Comment 47

[David Baron :dbaron:]

... which says that that's not the problem.

Comment 48

[Cameron McCormack (:heycam)]

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.

Comment 49

[Cameron McCormack (:heycam)]

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.

Comment 50

[David Baron :dbaron:]

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

Comment 51

[David Baron :dbaron:]

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.

Comment 52

[David Baron :dbaron:]

Try push testing comment 51:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95513a46278d
whose baseline should be:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=0c454540fc2b

Comment 53

[Cameron McCormack (:heycam)]

(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

Comment 54

[Cameron McCormack (:heycam)]

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

Comment 55

[David Baron :dbaron:]

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

Comment 56

[Cameron McCormack (:heycam)]

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.

Comment 57

[Cameron McCormack (:heycam)]

(That was https://treeherder.mozilla.org/#/jobs?repo=try&revision=515779f55f6c.)

Comment 58

[Cameron McCormack (:heycam)]

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

Comment 59

[Cameron McCormack (:heycam)]

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

Comment 60

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 61

[Cameron McCormack (:heycam)]

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.

Comment 62

[Joel Maher ( :jmaher ) (UTC -8)]

you can do this on try server with:
try: -b o -p win32 -u none -t svgr mozharness: --spsProfile

Comment 63

[Cameron McCormack (:heycam)]

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.

Comment 64

[Cameron McCormack (:heycam)]

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.

Comment 65

[David Baron :dbaron:]

Should be testable with some printfs of what we're animating/transitioning.

Comment 66

[Cameron McCormack (:heycam)]

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

Comment 67

[Cameron McCormack (:heycam)]

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

Comment 68

[Cameron McCormack (:heycam)]

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.

Comment 69

[Cameron McCormack (:heycam)]

Attachment: Part 9: Avoid transitioning border-end-color on the bookmark toolbar icon when switching between enabled and disabled states.

Comment 70

[Cameron McCormack (:heycam)]

Talos looks happy with this patch:

without: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ab745d7467
with:    https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f93454b0e75

Comment 71

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 72

[Cameron McCormack (:heycam)]

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.

Comment 73

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah! Well, in that case then...

Comment 74

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a80438019d25

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a80438019d25

Comment 76

[David Baron :dbaron:]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29a9317a44c2c18325ad4e823fe87779eb0f85c2
Bug 649142 followup:  Correct #undef.  No review.

Comment 77

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29a9317a44c2