Closed
Bug 1418930
Opened 5 years ago
Closed 5 years ago
Implement CORS-enabled fetch for shape images
Categories
(Core :: Layout: Floats, enhancement, P3)
Core
Layout: Floats
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: TYLin, Assigned: bradwerth)
References
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
emilio
:
review+
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
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
Reporter | ||
Updated•5 years ago
|
Priority: -- → P3
Reporter | ||
Updated•5 years ago
|
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b52215a1907adcbc4a8dd561dc8e7d92530ac5e2
Assignee | ||
Updated•5 years ago
|
Attachment #8945645 -
Flags: review?(emilio)
Attachment #8945646 -
Flags: review?(emilio)
Attachment #8945647 -
Flags: review?(emilio)
Attachment #8945648 -
Flags: review?(emilio)
Comment 20•5 years ago
|
||
mozreview-review |
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)
Attachment #8945645 -
Flags: review?(emilio) → review+
Comment 21•5 years ago
|
||
mozreview-review |
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 :)
Attachment #8945646 -
Flags: review?(emilio) → review+
Comment 22•5 years ago
|
||
mozreview-review |
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.
Attachment #8945647 -
Flags: review?(emilio)
Comment 23•5 years ago
|
||
mozreview-review |
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:bwerth@mozilla.com"/> > +<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:bwerth@mozilla.com"/> > +<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:bwerth@mozilla.com"/> > +<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:bwerth@mozilla.com"/> > +<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.
Attachment #8945648 -
Flags: review?(emilio)
Assignee | ||
Updated•5 years ago
|
Attachment #8945645 -
Flags: review?(bzbarsky)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•5 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 29•5 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 30•5 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 31•5 years ago
|
||
mozreview-review-reply |
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•5 years ago
|
||
mozreview-review |
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.
Attachment #8945645 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 33•5 years ago
|
||
(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?
Flags: needinfo?(emilio)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•5 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•5 years ago
|
Attachment #8945648 -
Flags: review?(emilio) → review?(bzbarsky)
Attachment #8945647 -
Flags: review?(emilio)
Attachment #8947248 -
Flags: review?(emilio)
![]() |
||
Comment 40•5 years ago
|
||
mozreview-review |
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
Attachment #8945648 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 41•5 years ago
|
||
mozreview-review |
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•5 years ago
|
||
mozreview-review |
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.
Attachment #8945647 -
Flags: review+
Comment 43•5 years ago
|
||
mozreview-review |
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)?
Attachment #8947248 -
Flags: review?(emilio)
Updated•5 years ago
|
Flags: needinfo?(emilio)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•5 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 50•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f4a4d91e9723b38d8e36796929add1714ce9370
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=375adb572c040750c9a5157c90a5097c9c145377
Comment 54•5 years ago
|
||
mozreview-review |
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! :)
Attachment #8947248 -
Flags: review?(emilio) → review+
Comment hidden (mozreview-request) |
Comment 56•5 years ago
|
||
Pushed by bwerth@mozilla.com: 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•5 years ago
|
||
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
Flags: needinfo?(bwerth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8948446 -
Flags: review?(emilio)
Comment 64•5 years ago
|
||
mozreview-review |
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`.
Attachment #8948446 -
Flags: review?(emilio) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 72•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=542237ab08958f8f78ec2b66b682117b79ca6e4a
Assignee | ||
Comment 73•5 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 79•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=130ea051ae08b538281a5c9b219e7b4ad64cf5b9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8948859 -
Flags: review?(emilio)
Comment 85•5 years ago
|
||
mozreview-review |
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
Attachment #8948859 -
Flags: review?(emilio) → review+
Comment 87•5 years ago
|
||
Pushed by bwerth@mozilla.com: 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•5 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•