1341758 - stylo: need image-orientation support

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

It's not obvious to me.

Comment 1

[Manish Goregaokar [:manishearth]]

https://manishearth.github.io/css-properties-list/?stylo=hide&servo=show&firefox=only&chrome=show&mdn=false&alexa=false says we don't support image-orientatiom in Stylo yet. That page was updated quite recently so it  probably hasn't changed since then.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  There various not-marked-failing reftest/image image-orientation tests, but maybe those have orientation in the image itself....

Comment 3

[Jeremy Chen ([e-mail])]

Since stylo-failures.md shows that ~90 failures about this, I'd like to take a look and see if I can make some progress here. Looks like we already had parsing/serialization support in servo [1]. So, I'll set the product from "none" to "gecko" first, and see how it goes. I suspect there might be couple/many errors that need to be fixed.


[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/servo/components/style/properties/longhand/inherited_box.mako.rs#85

Comment 4

[Bobby Holley (:bholley)]

Awesome, thanks Jeremy!

Comment 5

[Jeremy Chen ([e-mail])]

Got a WIP that works fine (I mean the specified/computed values are agreed with the m-c build) on my local machine. Let's see how it goes on try before requesting review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e3b89cc3c50f38ab00c2ea287d6bdcc0d932976

Comment 6

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> Got a WIP that works fine (I mean the specified/computed values are agreed
> with the m-c build) on my local machine. Let's see how it goes on try before
> requesting review.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2e3b89cc3c50f38ab00c2ea287d6bdcc0d932976

Looks like the mochitest expected failures are now passed, and try also shows a lot of unexpected pass on reftests!!! \o/

I noticed that the expected failures of 'test_value_storage.html' are not fixed.
Xidorn, do you have any idea how to fix them?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

I have no idea without some investigation. You can investigate that yourself via running the test locally with --failure-pattern-file=stylo-failures.md and the items you want to investigate removed from it, so that it shows those failures as unexpected failures, and you can get more information.

Comment 8

[Jeremy Chen ([e-mail])]

I digged a bit about test_property_syntax_errors.html failures (see the try result from comment 5), and found that the parsing step is slice different between Servo [1] and Gecko [2].

1. The empty string (image-orientation: "") is invalid in Gecko, whereas it is valid in Servo and is parsed to image-orientation: "0deg"

This one can be easily fixed by applying the Gecko parsing logic to Servo, since this looks like a servo parsing bug.

2. '0' is treated as an invalid value in Gecko (see [3]), but is an valid value for Servo's <angle> unit (see [4])

From spec. for image-orientation [5], it is not clear to me if we should treat '0' as an invalid value. I double checked spec. for <angle> unit [6] and still not sure if we should treat '0' as an invalid value for <angle>.

If we do want the <angle> unit to treat '0' as an invalid value, I shall make the implementation of Servo side to be agreed with the spec. If image-orientation is a special case, then we might need to separate the implementation of Servo side to two different versions, say AngleOrZero and Angle. Not sure if this is related to the originate of VARIANT_ANGLE and VARIANT_ANGLE_OR_ZERO in Gecko...

Hi David, could you enlighten me a bit about this?


[1] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/servo/components/style/properties/longhand/inherited_box.mako.rs#202
[2] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/layout/style/nsCSSParser.cpp#8212
[3] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/layout/style/test/property_database.js#6763
[4] http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/servo/components/style/values/specified/mod.rs#433
[5] https://drafts.csswg.org/css-images-3/#propdef-image-orientation
[6] https://www.w3.org/TR/css3-values/#angles

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Bug 1341758 - add image-orientation related binding functions.

For image-orientation property, we use methods of nsStyleImageOrientation
struct to create/set its computed value. Adding these binding functions
could let us reuse these methods for transferring computed values from Servo
side to Gecko.

The actual changes for the calling these binding functions to do the glue things
for Stylo is in a separate following patch, which should be pure Servo changes.

Review commit: https://reviewboard.mozilla.org/r/127136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127136/

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1341758 - add gecko glue code for image-orientation property.

With the newly added binding functions support, we shall reuse the same methods
from Gecko to pass the computed value from Servo to Gecko.

Review commit: https://reviewboard.mozilla.org/r/127138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127138/

Comment 11

[Cameron McCormack (:heycam)]

In bug 1234357, Emilio wanted to make Gecko follow the recent change to allow unitless 0 angles, but per the comments in that bug, it seems like it's a recent WG discussion topic, so he is waiting for that discussion to happen before proceeding.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I suspect the current state of the spec is untenable, for the reasons Tab mentions in https://github.com/w3c/csswg-drafts/issues/1162 and we should make stylo match gecko's current behavior here.  We could wait on it a bit in case the CSSWG decides to do something hideously complicated to try to resolve the issue, I guess.

Comment 13

[Bobby Holley (:bholley)]

Let's not wait. Matching gecko sounds good to me, and we can always fix it later if the spec discussion ends up with some other solution.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #12)
> I suspect the current state of the spec is untenable, for the reasons Tab
> mentions in https://github.com/w3c/csswg-drafts/issues/1162 and we should
> make stylo match gecko's current behavior here.

This sounds reasonable to me.

Comment 15

[Jeremy Chen ([e-mail])]

Attachment: Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Numbers without any AngleUnit, including unitless 0 angle, should be invalid
for image-orientation. However, current implementation of <angle> parser,
i.e., Angle::parse(), accepts unitless 0 angle, and stores it as '0deg'.
So, I create Angle::parse_without_unitless() and use it to correct the parsing logic.

Review commit: https://reviewboard.mozilla.org/r/128160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128160/

Comment 16

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/1-2/

Comment 17

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/1-2/

Comment 18

[Jeremy Chen ([e-mail])]

Ok, I've uploaded a patch to make image-orientation's parsing logic for Servo match that for Gecko. It can be seen from https://treeherder.mozilla.org/#/jobs?repo=try&revision=55f70817a5050d1ce4cb46730236991e6abe5ba0, test_property_syntax_errors.html failures are gone. Moreover, the test_value_storage.html failures are reduced from 80 to 40. \o/

test_value_storage.html is a verification test, to check the idempotence of parsing, storage, and serialization of CSS values. I'll investigate a bit to see if I could further reduce the test_value_storage.html failures.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I would assume that all the other places where angles are parsed (gradients come to mind) are problematic too, right?

As in, maybe we should in fact be fixing changing Angle::parse().

Comment 20

[Jeremy Chen ([e-mail])]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #19)
> I would assume that all the other places where angles are parsed (gradients
> come to mind) are problematic too, right?
> 
> As in, maybe we should in fact be fixing changing Angle::parse().

Yes, to be more specific, I think the implementations of values::specified::Angle should be fixed/changed to match what Gecko does. Looks like we already filed Bug 1350853 for this. I think all/most the other failures could be fixed in Bug 1350853. However, I'm not sure if we should let Bug 1350853 block our work in this bug...

Comment 21

[Jeremy Chen ([e-mail])]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #19)
> I would assume that all the other places where angles are parsed (gradients
> come to mind) are problematic too, right?
> 
> As in, maybe we should in fact be fixing changing Angle::parse().

Oh, just realize that you're saying that, maybe we should fix Angle::parse() directly, so we could fix other properties that use Angle::parse().

Per Comment 11, I'm not sure if I shall change the current Angle::parse(). So, I'm adding an extra Angle::parse_without_unitless(), and use that to fix the image-orientation parsing. Are you suggesting that, I shall just make the current Angle::parse() not accept unitless angle (including unitless 0 angle)?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yes, that's what I'm suggesting.  See comment 13.

Comment 23

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/2-3/

Comment 24

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/2-3/

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128160/diff/1-2/

Comment 26

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

https://reviewboard.mozilla.org/r/127136/#review130760

Comment 27

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

https://reviewboard.mozilla.org/r/127138/#review130762

::: servo/components/style/properties/gecko.mako.rs:2785
(Diff revision 3)
> +                  skip_longhands="image-orientation">
> +    pub fn set_image_orientation(&mut self, v: longhands::image_orientation::computed_value::T) {
> +        use properties::longhands::image_orientation::computed_value::T;
> +        match v {
> +            T::FromImage => {
> +                unsafe {

Add a note here that we could inline these if needed, but it's not done just for convenience?

Comment 28

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

https://reviewboard.mozilla.org/r/128160/#review130764

Awesome, thanks for fixing this! :)

::: servo/components/style/properties/longhand/box.mako.rs:1472
(Diff revision 2)
>                          Ok(())
>                      }))
>                  },
>                  "rotate" => {
>                      try!(input.parse_nested_block(|input| {
> -                        let theta = try!(specified::Angle::parse(context,input));
> +                        let theta = try!(specified::Angle::parse_with_unitless(context,input));

Please add a comment about we allowing unitless angles here to align with gecko at the beginning of the function.

::: servo/components/style/values/specified/mod.rs:454
(Diff revision 2)
>               _ => return Err(())
>          };
>          Ok(angle)
>      }
> +    /// Parse an angle, including unitless 0 degree.
> +    // Note that numbers without any AngleUnit, including unitless 0

Any reason this is not part of the doc comment? I think it's worth, so feel free to use `///` instead of just `//` here.

Comment 29

[Jeremy Chen ([e-mail])]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #22)
> Yes, that's what I'm suggesting.  See comment 13.

Looks like we're treating unitless zero degree as valid value for rotate() and skew() in CSS transform property. See https://dxr.mozilla.org/mozilla-central/rev/2a3ecdb7d1ea814708021fee6735b3aedcf03e48/layout/style/nsCSSParser.cpp#15896 for the VariantMask settings. So, to match Servo with Gecko, we still have to maintain two versions of Angle::parse() (one accepts unitless zero and the other does not). Hopefully, we could remove one of them once the spec issue is resolved.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yes, you need two versions.  Just the default one should be to not accept unitless 0...

> Hopefully, we could remove one of them once the spec issue is resolved.

I doubt it, if it gets resolved the way I expect.

Comment 31

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

https://reviewboard.mozilla.org/r/128160/#review130764

> Please add a comment about we allowing unitless angles here to align with gecko at the beginning of the function.

Will do.

> Any reason this is not part of the doc comment? I think it's worth, so feel free to use `///` instead of just `//` here.

No problem. Will fix this in the Servo PR. I guess I'm still figuring out when is the right time to use `///` instead of `//`.

Comment 32

[Jeremy Chen ([e-mail])]

Attachment: Bug 1341758 - update mochitest expectations for image-orientation support.

Review commit: https://reviewboard.mozilla.org/r/128292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128292/

Comment 33

[Jeremy Chen ([e-mail])]

Attachment: Bug 1341758 - update reftest expectations for image-orientation support.

Review commit: https://reviewboard.mozilla.org/r/128294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128294/

Comment 34

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/3-4/

Comment 35

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/3-4/

Comment 36

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128160/diff/2-3/

Comment 37

[Jeremy Chen ([e-mail])]

Servo PR: https://github.com/servo/servo/pull/16334

Comment 38

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f463c38dca9564fd06cb29d91e2b293265e717a

There are 60 unexpected-pass for reftest and 70 unexpected-pass for mochitest!! :)

Comment 39

[Jeremy Chen ([e-mail])]

Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dff8581ed4e221d1bd166847ffb354a0ec25d3f

2 reftest regressions....looks like we didn't round -45deg (and 315deg as well) away from 0deg in Servo. :(



[1] https://drafts.csswg.org/css-images-3/#the-image-orientation

Comment 40

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #39)
> Latest try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2dff8581ed4e221d1bd166847ffb354a0ec25d3f
> 
> 2 reftest regressions....looks like we didn't round -45deg (and 315deg as
> well) away from 0deg in Servo. :(
> 

No, I was wrong. Looks like Servo did the right thing, which is indeed doing the "rounding away from 0" as spec. says. I verified this by comparing the result from Gecko's logic with that from Servo's through https://play.rust-lang.org.

In fact, Gecko [2] seems more like doing the "rounding half up" instead. I'm uploading a patch (which passed my local test) to make Servo align with Gecko.


[1] https://drafts.csswg.org/css-images-3/#the-image-orientation
[2] http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/layout/style/nsStyleStruct.h#2024-2029

Comment 41

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/4-5/

Comment 42

[Jeremy Chen ([e-mail])]

Comment on attachment 8856374 [details]
Bug 1341758 - update mochitest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128292/diff/1-2/

Comment 43

[Jeremy Chen ([e-mail])]

Comment on attachment 8856375 [details]
Bug 1341758 - update reftest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128294/diff/1-2/

Comment 44

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/4-5/

Comment 45

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128160/diff/3-4/

Comment 46

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/5-6/

Comment 47

[Jeremy Chen ([e-mail])]

Comment on attachment 8856374 [details]
Bug 1341758 - update mochitest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128292/diff/2-3/

Comment 48

[Jeremy Chen ([e-mail])]

Comment on attachment 8856375 [details]
Bug 1341758 - update reftest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128294/diff/2-3/

Comment 49

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/5-6/

Comment 50

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128160/diff/4-5/

Comment 51

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #39)
> Latest try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2dff8581ed4e221d1bd166847ffb354a0ec25d3f
> 
> 2 reftest regressions....looks like we didn't round -45deg (and 315deg as
> well) away from 0deg in Servo. :(
> 
> 
> 
> [1] https://drafts.csswg.org/css-images-3/#the-image-orientation

Filed Bug 1355107 for correcting the Gecko's implementation.

I've made the Servo's implementation almost align with Gecko (except for the fmod operation). See https://reviewboard.mozilla.org/r/128160/diff/3-5/. The -45deg case seems fixed, whereas the 315deg case still remains. No idea why this change can't fix it...:/

Emilio, could you take a look at this change and give me some feedback?

Comment 52

[Jeremy Chen ([e-mail])]

Revisited the code path, there's still slice difference between Gecko and Stylo. Gecko and Servo both store specified angular values with units (deg, rad, ... etc.). However, for computed values, Gecko take the parsed radians value, do the rounding stuff, and then compute/store it with 2 bits (represents for the 4 possible quarter-turn) within an uint8_t. However, Servo take the parsed radians value, compute and round it to the nearest quarter-turn in radians, and then store it as a radians. In the gecko glue code, we use Servo's rounded computed radians, and pass this to the Gecko's binding method to go through Gecko's rounding process, which is just for the implementation convenience.

The 2 regressions might be caused by precision errors between float and double, since we compute/round with f32 in Servo but with f64 in Gecko. The normalize_angle computation might introduce some potential rounding errors I guess.

Second thoughts, maybe we could mark these 2 regressions for now, and investigate making the Servo to exactly align with Gecko later. So we don't need to block the work in this bug.


[1] http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/servo/components/style/properties/longhand/inherited_box.mako.rs#147

Comment 53

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/6-7/

Comment 54

[Jeremy Chen ([e-mail])]

Comment on attachment 8856374 [details]
Bug 1341758 - update mochitest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128292/diff/3-4/

Comment 55

[Jeremy Chen ([e-mail])]

Comment on attachment 8856375 [details]
Bug 1341758 - update reftest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128294/diff/3-4/

Comment 56

[Jeremy Chen ([e-mail])]

Comment on attachment 8855263 [details]
Bug 1341758 - add gecko glue code for image-orientation property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127138/diff/6-7/

Comment 57

[Jeremy Chen ([e-mail])]

Comment on attachment 8856214 [details]
Bug 1341758 - make Servo's image-orientation parser to be agreed with Gecko's.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128160/diff/5-6/

Comment 58

[Jeremy Chen ([e-mail])]

> The 2 regressions might be caused by precision errors between float and
> double, since we compute/round with f32 in Servo but with f64 in Gecko. The
> normalize_angle [1] computation might introduce some potential rounding errors I
> guess.
> 
> Second thoughts, maybe we could mark these 2 regressions for now, and
> investigate making the Servo to exactly align with Gecko later. So we don't
> need to block the work in this bug.
> 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 624d25b2980cf5f83452b040e6d664460f9b7ec5/servo/components/style/properties/
> longhand/inherited_box.mako.rs#147

Filed Bug 1355380 for this.

Hi Emilio, if you're okay with this proposal, please help review my test expectation parts (1 for mochitest and 1 for reftest).

Comment 59

[Jeremy Chen ([e-mail])]

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

Comment 60

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8856374 [details]
Bug 1341758 - update mochitest expectations for image-orientation support.

https://reviewboard.mozilla.org/r/128292/#review131294

Yeah, your proposal makes sense to me, thanks!

Comment 61

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8856375 [details]
Bug 1341758 - update reftest expectations for image-orientation support.

https://reviewboard.mozilla.org/r/128294/#review131296

Comment 62

[Jeremy Chen ([e-mail])]

Thank you for the review.

Since the Servo PR is ready to merge, I'm going to remove the servo side patches and get the Gecko parts ready.

Comment 63

[Jeremy Chen ([e-mail])]

Comment on attachment 8855262 [details]
Bug 1341758 - add image-orientation related binding functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127136/diff/7-8/

Comment 64

[Jeremy Chen ([e-mail])]

Comment on attachment 8856374 [details]
Bug 1341758 - update mochitest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128292/diff/4-5/

Comment 65

[Jeremy Chen ([e-mail])]

Comment on attachment 8856375 [details]
Bug 1341758 - update reftest expectations for image-orientation support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128294/diff/4-5/

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bda98be428d4
add image-orientation related binding functions. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fa5e5a103448
update mochitest expectations for image-orientation support. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a5d496493a0b
update reftest expectations for image-orientation support. r=emilio

Comment 67

[Boris Zbarsky [:bzbarsky]]

> Second thoughts, maybe we could mark these 2 regressions for now

That's OK if the marking is annotated with a bug number tracking the proper fix. That means that bug needs to be filed and tracked for stylo stuff, etc.

The changesets that were landed do not seem to have such a bug number annotation.  Am I just missing it?

Comment 68

[Daniel Holbert [:dholbert]]

In addition to comment 67, it seems like this might need another "update mochitest expectations" tweak.  This triggered TreeHerder orange with this failure message:
> layout/style/test/test_media_queries.html | failures in this test
> expected 657 failures but got 637 
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a5d496493a0be91e0611dbf2c06eeba917631495&selectedJob=90547487&filter-searchStr=stylo

Sample log: https://treeherder.mozilla.org/logviewer.html#?job_id=90547487&repo=autoland

I've asked that Aryx back this out for now -- before re-landing, please adjust the failure-count as-needed (if the change is expected, e.g. maybe test_media_queries.html tests for image-orientation support?), and follow up on comment 67.

Comment 69

[Daniel Holbert [:dholbert]]

Ah, sorry -- the media query test failures are probably from the previous push:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d6f1d936c4ca562aae5f353fd22582b86c0525f&filter-searchStr=stylo
...which included this change:
"Merge #16342 - Fix a few media query parsing bugs (from emilio:media-query-parsing); r=SimonSapin"

That previous push didn't build successfully for some reason, so the failures showed up on this bug's push (which did build successfully), making this push look like the guilty one.

:hiro's going to push an update to test-failure expectations (thanks hiro!) to take care of that.  So, disregard comment 68 [and Aryx isn't backing out after all] -- but do still see bz's comment 67.

Comment 70

[Emilio Cobos Álvarez [:emilio]]

(In reply to Daniel Holbert [:dholbert] from comment #69)
> Ah, sorry -- the media query test failures are probably from the previous
> push:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=1d6f1d936c4ca562aae5f353fd22582b86c0525f&filter-
> searchStr=stylo
> ...which included this change:
> "Merge #16342 - Fix a few media query parsing bugs (from
> emilio:media-query-parsing); r=SimonSapin"

Yeah, sorry about that. Normally I'd land the test expectation changes right after the Servo patch landed, but yesterday vcs-sync was down for some reason, and was fixed overnight, so I couldn't.

Comment 71

[Jeremy Chen ([e-mail])]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #67)
> > Second thoughts, maybe we could mark these 2 regressions for now
> 
> That's OK if the marking is annotated with a bug number tracking the proper
> fix. That means that bug needs to be filed and tracked for stylo stuff, etc.
> 
> The changesets that were landed do not seem to have such a bug number
> annotation.  Am I just missing it?

Yes, I've filed Bug 1355380 for this.

I shall add some comments with the bug number in the patch.
Since I can't push to autoland, I'll push a followup once it is merged to m-c.

Comment 72

[Jeremy Chen ([e-mail])]

Leave ni for the followup fix.

Comment 73

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/bda98be428d4
https://hg.mozilla.org/mozilla-central/rev/fa5e5a103448
https://hg.mozilla.org/mozilla-central/rev/a5d496493a0b

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecd8cc1333a
followup - add comments to the disabled tests. comment-only, no review, DONTBUILD

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ecd8cc1333a