1418930 - Implement CORS-enabled fetch for shape images

Status: RESOLVED FIXED

(Core :: Layout: Floats, enhancement, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Per spec, we need to implement CORS-enabled fetch ("Anonymous" mode) for shape image URLs.

https://drafts.csswg.org/css-shapes/#valdef-shape-outside-image

Comment 2

[Brad Werth [:bradwerth]]

I'll try to move this forward.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 19

[Brad Werth [:bradwerth]]

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

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222280

r=me, with the flag double-checked and the enum class, thanks!

::: layout/style/ImageLoader.h:68
(Diff revision 4)
>    // in because this can be called during presentation destruction after the
>    // presshell pointer on the document has been cleared.
>    void ClearFrames(nsPresContext* aPresContext);
>  
>    void LoadImage(nsIURI* aURI, nsIPrincipal* aPrincipal, nsIURI* aReferrer,
> -                 Image* aCSSValue);
> +                 Image* aCSSValue, bool aUseAnonymousCORS);

Can we make this an enum class (even if it's just a Yes / No variant) to make this clear on the caller?

```
enum class UseAnonymousCORS {
    Yes,
    No
};
```

::: layout/style/ImageLoader.cpp:260
(Diff revision 4)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aUseAnonymousCORS)) {

This looks legit, but please double-check this with someone more familiar than I with the networking code, to double check that we don't need anything else.

Also since LOAD_NORMAL is 0, this may look cleaner with the ternary operator (or not, not sure, your call :P)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

https://reviewboard.mozilla.org/r/215780/#review222282

::: layout/style/nsCSSValue.h:207
(Diff revision 4)
>  protected:
>    // Only used by ImageValue.  Declared up here because otherwise bindgen gets
>    // confused by the non-standard-layout packing of the variable up into
>    // URLValueData.
>    bool mLoadedImage = false;
> +  bool mUseAnonymousCORS = false;

Here we can use the enum class defined before and all that :)

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222284

What do you think about that? I'd really prefer to reduce mutation in our style structs as much as possible, even though here we're guaranteed to be on the main thread.

::: commit-message-4d588:1
(Diff revision 4)
> +Bug 1418930 Part 3: Update nsStyleStruct::FinishStyle to add flag for anonymous CORS to shape-outside images.

nit: `nsStyleDisplay::FinishStyle`.

::: layout/style/nsStyleStruct.cpp:3716
(Diff revision 4)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetUseAnonymousCORS(true);

Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222286

Is there any reason this can't be written as a WPT test? I think we should default to WPT for new tests unless there's a strong reason not to.

::: layout/style/test/test_shape_outside_with_cors.html:7
(Diff revision 4)
> +<html>
> +<head>
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:[e-mail]"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>

nit: let's link to the csswg draft instead:

https://drafts.csswg.org/css-shapes/

::: layout/style/test/test_shape_outside_with_cors.html:8
(Diff revision 4)
> +<head>
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:[e-mail]"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +

nit: stray newline?

::: layout/style/test/test_shape_outside_with_cors.html:9
(Diff revision 4)
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:[e-mail]"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:10
(Diff revision 4)
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:[e-mail]"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:12
(Diff revision 4)
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +
> +<style type="text/css">

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:45
(Diff revision 4)
> +
> +function runTests() {
> +  // Test 1: Confirm that a shape-outside with a same-origin URL works.
> +  let divAccept = document.getElementById("accept");
> +  let divAcceptChild = divAccept.children[0];
> +  divAccept.style.shapeOutside = 'url("http://mochi.test:8888/dom/html/test/image.png")';

This is not guaranteed to work at all I think, why would the image loading be synchronous?

You should probably specify them on the stylesheet, so the loads block the document load event.

Comment 24

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 25

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 26

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 27

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 28

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222356

::: layout/style/ImageLoader.cpp:260
(Diff revision 4)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aUseAnonymousCORS)) {

I'm relying on https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/image/imgLoader.cpp#2270 to indicate that CORS mode is already triggerable with a simple flag. I'll add bz as reviewer to confirm (he reviewed parts of Bug 664299 which added that code).

Regardling LOAD_NORMAL and additional flags, I'll leave it as a bitwise OR, since now that we're using the CORSMode enum, it's possible for there to be other policies added later (such as CORS_USE_CREDENTIALS) where the bitwise will flow better.

Comment 29

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222284

> Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.

Yes, it's weird. I'm prepping a new version of the patch that moves this to the shape-outside case of nsRuleNode::ComputeDisplayData(). I'll also try to make it more of a parameter-passing call instead of a drill-through call.

Comment 30

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222534

Comment 31

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222286

One factor for this bug is that the feature is not yet enabled; we're trying to assure ourselves that the CORS mode is respected before we land the bug that enables the feature. There are already lots of WPT tests of shape-outside that we'll turn on once we land the feature patch itself (blocked by this bug). To test just the CORS rejection, I thought it would be useful to set a todo in a new mochitest that we want to see unexepectedly pass when the feature is landed.

> This is not guaranteed to work at all I think, why would the image loading be synchronous?
> 
> You should probably specify them on the stylesheet, so the loads block the document load event.

Good point. I was hoping to avoid an embedded iframe for this test, but it may be unavoidable. We're challenged because a pref has to be turned on, and if we use SpecialPowers to do that, we have to load the shape-outside styled content afterwards. I'll rebuild the test around an embedded iframe.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222572

::: layout/style/ImageLoader.cpp:260
(Diff revision 5)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aCorsMode == CORSMode::CORS_ANONYMOUS)) {

Why do we need this MOZ_UNLIKELY?

Do we really want to load CORS_USE_CREDENTIALS as LOAD_NORMAL?  Seems like we'd want to convert that to imgILoader::LOAD_CORS_USE_CREDENTIALS.

Also, I'd really prefer it if we had some utility method here instead of adding a third copy of this "map the CORS flags over to the imagelib flag" code.

Comment 33

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #29)
> Comment on attachment 8945647 [details]
> Bug 1418930 Part 3: Set CORS Mode for ImageValues created by shape-outside
> declarations.
> 
> https://reviewboard.mozilla.org/r/215782/#review222284
> 
> > Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.
> 
> Yes, it's weird. I'm prepping a new version of the patch that moves this to
> the shape-outside case of nsRuleNode::ComputeDisplayData(). I'll also try to
> make it more of a parameter-passing call instead of a drill-through call.

I just realized an advantage of putting this call in this weird place. If it's here, it applies to both Gecko and Stylo parsed styles. If the call is moved back to the point of creation of the ImageValue, we have to change it in at least two places (Gecko in nsRuleNode::ComputeDisplayData, and Stylo in basic_shape::parse or thereabouts). I'm not confident in my ability to produce the servo changes quickly in a way that isn't totally gross. Because this bug is a blocker for landing shape-outside, which is a target for 60, I'd like to land the ugly version first and file a bug for a "CORS Mode per property" cleanup that would move this earlier for both Gecko and Servo. Emilio, is that acceptable?

Comment 34

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 35

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 36

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 37

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 38

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 39

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222572

> Why do we need this MOZ_UNLIKELY?
> 
> Do we really want to load CORS_USE_CREDENTIALS as LOAD_NORMAL?  Seems like we'd want to convert that to imgILoader::LOAD_CORS_USE_CREDENTIALS.
> 
> Also, I'd really prefer it if we had some utility method here instead of adding a third copy of this "map the CORS flags over to the imagelib flag" code.

Utility method added as a new Part 1 of this patch that addresses all these issues.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222854

r=me

Comment 41

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222974

::: layout/style/nsStyleStruct.cpp:3735
(Diff revision 6)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetCORSMode(

Mutations are bad and this is ugly. But I'd still like to land the code this way since it works for both Gecko and Stylo. If it's acceptable, I'll open a new bug to move the CORS Mode changes to the point of creation of the ImageValue in both Gecko and Stylo, and put a bug comment here.

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222540

r=me, reluctantly :)

As I said I'd prefer fixing it instead of adding it here, but your call.

::: layout/style/nsStyleStruct.cpp:3735
(Diff revision 6)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetCORSMode(

I think this shouldn't be hard to fix. Image values in Servo are constructed at parse-time in `Url::build_image_value`. It's a matter of making a `CorsMode` arrive there.

Which itself it's a matter of moving the parsing of `Image` to something like `parse_with_cors_mode`, and making a `struct ShapeImage(pub Image)` (or `CorsImage`? I'm terrible at naming) using it in `components/style/values/specified/basic_shape.rs`, calling it.

I'd appreciate fixing it, honestly this feels like a huge hack. But if you don't, please file a bug, and do reference it from a comment here.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223000

I'm not sure there's much value in landing the test if as you mentioned there are WPT tests for this, and this test is marked as "todo" since this still hasn't landed. Mind elaborating?

Anyway I guess more tests are fine, just want to know the reasoning before stamping.

::: layout/style/test/test_shape_outside_CORS.html:16
(Diff revision 1)
> +<script>
> +SimpleTest.waitForExplicitFinish();
> +
> +// Set a pref that we'll need before we load contents into the iframe.
> +const oldShapeOutsidePref = SpecialPowers.getBoolPref("layout.css.shape-outside.enabled");
> +SpecialPowers.setBoolPref("layout.css.shape-outside.enabled", true);

There's no other `setBoolPref` call in layout/style/test, other tests use pushPrefEnv. What happens if the test times out (not sure, just asking)?

Comment 44

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 45

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 46

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 47

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 48

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 49

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223000

The WPT tests might not check the CORS mode behavior. There are a lot of shape-outside tests, but a CORS mode check is necessarily either browser dependent (check the error text that shows up in the browser console) and therefore not in WPT or it tests the absence of changes from the feature, as this test does. So landing this is a good sanity-check for when we turn on the shape-outside feature, in case WPT doesn't attempt to check CORS.

> There's no other `setBoolPref` call in layout/style/test, other tests use pushPrefEnv. What happens if the test times out (not sure, just asking)?

I changed it to use pushPrefEnv/popPrefEnv. I don't know if it makes the pref state more resilient in a test timeout, but it can't hurt.

Comment 50

[Brad Werth [:bradwerth]]

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

Comment 51

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 52

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 53

[Brad Werth [:bradwerth]]

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

Comment 54

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223152

r=me with the popPrefEnv bit fixed / removed.

::: layout/style/test/file_shape_outside_CORS.html:37
(Diff revision 3)
> +  window.parent.postMessage({
> +    "result": (divAllowChild.getBoundingClientRect().left != divAllow.getBoundingClientRect().left),
> +    "message": "Test 1: Child is at different left offset from parent (shape-outside was allowed).",
> +    "todo": true,
> +    },
> +    "http://mochi.test:8888");

it'd be slightly nice to not hardcode the testing domain, but not sure if it's doable, it's not a big deal either way.

::: layout/style/test/test_shape_outside_CORS.html:21
(Diff revision 3)
> +{
> +  if (event.data.done) {
> +    // Remove ourself as an event listener, just to be thorough.
> +    window.removeEventListener("message", receiveMessage);
> +    // Undo our meddling in preferences, then finish the test.
> +    SpecialPowers.popPrefEnv(SimpleTest.finish());

Oh, I was totally assuming that pushPrefEnv popped the preference itself when the callback returned... But apparently the caller is responsible to call popPrefEnv, and we basically never do.

So the point in using it I think is the fact that the test runner calls flushPrefEnv:

  https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/testing/mochitest/tests/SimpleTest/TestRunner.js#645
  
which means that you don't really need to `popPrefEnv` manually here.

That being said, if you want to do so you probably want to do:

```
popPrefEnv(SimpleTest.finish)
```

Since popPrefEnv gets a `callback` parameter, not a return value, and I think calling popPrefEnv _after_ SimpleTest.finish is somewhat racy (someone could've pushed a pref already in other test).

But per the avobe the prefs are auto-cleaned-up, so just `SimpleTest.finish()` should work.

And that also means that the win of using `pushPrefEnv` vs `setFooPref` is that if the test fails / throws / times out, the runner manages to restore the prefs, instead of affecting the following tests. I guess I learned something today, thanks! :)

Comment 55

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27eb8c7f8025
Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags. r=bz
https://hg.mozilla.org/integration/autoland/rev/105c81284dba
Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers. r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/8008a54aba9a
Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b39948ba905b
Part 4: Update nsStyleStruct::FinishStyle to set CORS mode for shape-outside images. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c0f673033e66
Part 5: Add a test of shape-outside with and without a CORS violation. r=emilio

Comment 57

[Andreea Pavel [:apavel]]

Backed out for failing web platform tests at /service-workers/service-worker/fetch-request-css-images.https.html 

push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c0f673033e66c54a268f8e7d31203512378deac7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160140542&repo=autoland&lineNumber=6224

Backout: https://hg.mozilla.org/integration/autoland/rev/06ff7da60c3d0512a20bc221739562f778fd2465

Comment 58

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 7: Change a web platform test to PASS.

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

Comment 59

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 60

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 61

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 62

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

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

Comment 63

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 64

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

https://reviewboard.mozilla.org/r/217886/#review223710

r=me, thanks!

::: testing/web-platform/meta/service-workers/service-worker/fetch-request-css-images.https.html.ini:4
(Diff revision 1)
>  [fetch-request-css-images.https.html]
>    expected: TIMEOUT
>    [Verify FetchEvent for css image (shapeOutside).]
> -    expected: FAIL
> +    expected: PASS

Just remove this line and the line above it, tests default to `PASS`.

Comment 65

[Brad Werth [:bradwerth]]

Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

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

Comment 66

[Brad Werth [:bradwerth]]

Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

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

Comment 67

[Brad Werth [:bradwerth]]

Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

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

Comment 68

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

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

Comment 69

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215782/diff/9-10/

Comment 70

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 71

[Brad Werth [:bradwerth]]

Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

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

Comment 72

[Brad Werth [:bradwerth]]

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

Comment 73

[Brad Werth [:bradwerth]]

In order to pass testing/web-platform/meta/service-workers/service-worker/fetch-request-css-images.https.html on Gecko, I had to implement the Gecko part of the fix for Bug 1434963. The Stylo fix remains (and Bug 1434963 remains open).

Comment 74

[Brad Werth [:bradwerth]]

Attachment: Bug 1418930 Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS.

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

Comment 75

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215780/diff/9-10/

Comment 76

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215782/diff/10-11/

Comment 77

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 78

[Brad Werth [:bradwerth]]

Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

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

Comment 79

[Brad Werth [:bradwerth]]

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

Comment 80

[Brad Werth [:bradwerth]]

Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215780/diff/10-11/

Comment 81

[Brad Werth [:bradwerth]]

Comment on attachment 8948859 [details]
Bug 1418930 Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS.

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

Comment 82

[Brad Werth [:bradwerth]]

Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215782/diff/11-12/

Comment 83

[Brad Werth [:bradwerth]]

Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

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

Comment 84

[Brad Werth [:bradwerth]]

Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

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

Comment 85

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8948859 [details]
Bug 1418930 Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS.

https://reviewboard.mozilla.org/r/218246/#review224162

looks good, thanks! r=me

Comment 86

[Brad Werth [:bradwerth]]

Attempting to land again.

Comment 87

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d45873f6dcc4
Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags. r=bz
https://hg.mozilla.org/integration/autoland/rev/cac787a13132
Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers. r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/b1868b7597e2
Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9c080d88ee0c
Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e187bbe58b54
Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8554668651cc
Part 6: Add a test of shape-outside with and without a CORS violation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7a8fceb25ceb
Part 7: Change a web platform test to PASS. r=emilio

Comment 88

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d45873f6dcc4
https://hg.mozilla.org/mozilla-central/rev/cac787a13132
https://hg.mozilla.org/mozilla-central/rev/b1868b7597e2
https://hg.mozilla.org/mozilla-central/rev/9c080d88ee0c
https://hg.mozilla.org/mozilla-central/rev/e187bbe58b54
https://hg.mozilla.org/mozilla-central/rev/8554668651cc
https://hg.mozilla.org/mozilla-central/rev/7a8fceb25ceb