Closed Bug 1404222 Opened 7 years ago Closed 7 years ago

Support shape-outside: <image>

Categories

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

Product:
Core
Component:
Layout: Floats
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox61 --- fixed

People

(Reporter: TYLin, Assigned: bradwerth)

References

(Blocks 3 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [designer-tools][wptsync upstream])

Attachments

(10 files, 42 obsolete files)

Bug 1404222 Part 4: Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 5: Add web-platform-tests for linear-gradient with writing-modes.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 6: Add a crashtest.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 7: Turn off a 'todo' in a mochitest.
59 bytes, text/x-review-board-request
Details
Bug 1404222 Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1404222 Part 2: Extend ImageLoader to associate flags with each request-frame relationship.
4.97 KB, patch
Details | Diff | Splinter Review
Bug 1404222 Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed, the image fails to load, or reflow is complete. r=dbaron,dholbert
25.93 KB, patch
Details | Diff | Splinter Review
Bug 1404222 Part 1: Implement shape-outside: <image>.
24.95 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Depends on: 1404243
Do you attach wrong patches? Looks like the patches not related to shape-outside.
Flags: needinfo?(tlin)
Oops. Wrong bug number ...
Flags: needinfo?(tlin)
Attachment #8918815 - Attachment is obsolete: true
Attachment #8918815 - Flags: review?(mtseng)
Attachment #8918816 - Attachment is obsolete: true
Attachment #8918816 - Flags: review?(mtseng)
Depends on: 1418224
Depends on: 1418930
Given that bug 1366049 was landed to enable stylo on fennec, it seems no longer a need to support gecko implementation here.
TY, can we just make this bug invalid now?
Flags: needinfo?(tlin)
I want to use this bug to implement layout part of shape-outside: <image>.
Flags: needinfo?(tlin)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> I want to use this bug to implement layout part of shape-outside: <image>.

I see. Thanks.
Do we have code & tests to enforce that these images aren't allowed cross-origin? (specifically: "use the potentially CORS-enabled fetch method" per https://drafts.csswg.org/css-shapes/#shape-outside-property 

<img src="..."> works with pretty much any URL (including cross-origin URLs) for legacy reasons -- but the embedding page can't figure anything out about the image beyond its dimensions, so it's pretty harmless.  However, "shape-outside" needs to be stricter about cross-origin resources, because it lets the page figure out the actual image contents.

(I did a quick search for "cors" and "origin" in the Part 1 patch as well as in testing/web-platform/tests/css/css-shapes/shape-outside/shape-image , and I didn't get any hits, so I'm guessing this might not be tested & might not be implemented right now.)
Flags: needinfo?(tlin)
Comment on attachment 8942865 [details]
Bug 1404222 Part 3 - Adjust the #image width in shape-image-001.html.

https://reviewboard.mozilla.org/r/213072/#review219052

::: commit-message-0a35c:3
(Diff revision 1)
> +Per spec [1], the shape image's width is the #image's content box
> +width (150px), so there's available space for for the two 'X'. I change
> +the #image's width 100px so that the two "X" fit in the container.

I don't think this patch is actually correct. The test might need fixing, but not in this way.  In particular -- with this change, the test passes for me regardless of whether "shape-outside" support is enabled.  So I think that means it's wrong / not-what-the-author-intended.

Really, I think the test *does* intend for this element to have width:150px here -- and then the test intends for 50px of its width to get cancelled out via "shape-outside".  It's trying to achieve the same effect as if we had the following:
 shape-outside:inset(0px 50px 0px 0px);

That ^^ "inset()" tweak makes the test pass for me in Nightly (with the original width restored).  So I think you need to investigate what needs to happen with the test and/or code to get that effect via the image here.

(If you like: that might mean leaving this test marked as "fails" for the moment & filing a followup for further investigation.)
Attachment #8942865 - Flags: review?(dholbert) → review-
Comment on attachment 8942866 [details]
Bug 1404222 Part 3 - Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors.

https://reviewboard.mozilla.org/r/213074/#review219060

::: commit-message-8a2d6:1
(Diff revision 1)
> +Bug 1404222 Part 4 - Replace hash '#' by %023 to fix XML parse errors.

Three changes to this first line:

s/by/with/
s/023/23/
s/to fix/in SVG data URIs, to fix/

::: commit-message-8a2d6:3
(Diff revision 1)
> +Bug 1404222 Part 4 - Replace hash '#' by %023 to fix XML parse errors.
> +
> +The hash symbol '#' is reserved by as fragment identifier per bug 1430526

s/by as/as/
Attachment #8942866 - Flags: review?(dholbert) → review+
Comment on attachment 8942867 [details]
Bug 1404222 Part 5 - Adjust tolerance value in shape-outside-radial-gradient-004.html.

https://reviewboard.mozilla.org/r/213076/#review219068

::: commit-message-9d055:6
(Diff revision 1)
> +In this test, the diff pixels of <span id='test0'> is 1.01666259765625, so I
> +loose the tolerance to 1.5 pixels, which is the same as the tolerance value
> +use in shape-outside-radial-gradient-001.html.

Could we use 1.1 instead of 1.5 here?  (so that we're increasing the tolerance by 10% rather than by 50%)

It looks like you chose 1.5 so that this would be consistent with the other test.  But I'll bet the other test *actually needs* a tolerance of (near) 1.5, whereas this test clearly does not.  We might as well use as strict a tolerance as we can, so as not to inadvertently miss regressions / unexpected behavior-changes etc.  (Particularly given that this is a shared 3rd-party test, and our changes will make a difference for other vendors.)  So: 1.1 seems better here.

Also: do you know who's correct, between us & Chrome, on the rendering of this tescase? The difference is subtle, but it (visually) looks like we have a 1px disagreement on the placement of this 'test0' div, and it seems like it should be possible to analyze the sizes of the shapes involved & determine who is correct.  (It's probably a subtle rounding difference, but I imagine the behavior should still be well-defined.)  That part is perhaps not a blocker for this bug, but material for a followup?
Attachment #8942867 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #16)
> Do we have code & tests to enforce that these images aren't allowed
> cross-origin?

Oh, I'm now seeing that the CORS stuff is tracked in bug 1418930, which doesn't have any patches yet.  I'm glad that that's tracked -- but I'm a bit uneasy about landing the rest of these patches until we've *also* got the CORS support, though, because I think (?) that means we'll opening our enthusiast users (users who've preffed this on) to possible-attack in the interim.

This shape-outside feature *is* preffed off by default, so users wouldn't be vulnerable by default -- but I'm worried that developers might pref it on for testing & leave it that way, without realizing that they're potentially opening themselves up to dangerous cross-origin information-stealing attacks.  The pref sounds trivially safe right now ("layout.css.shape-outside.enabled") -- even to someone who knows the spec/feature, since the spec has these security guarantees built-in -- so that's why I'm worried that developers might try turning it on & leaving it on without realizing that it's dangerous.

Ideally, we should hold these patches until bug 1418930 is ready.  If that's impractical, I think I'd also probably be OK if we had some sort of blocking baked in at the C++ level, so that we'll fail towards safety even in builds where users have toggled the pref.  I don't know what that blocking would look like off the top of my head, though -- simplest option would be adding "#ifdef NIGHTLY_BUILD" around the code that triggers the image request, but that'd still leave our Nightly enthusiasts potentially exposed if they've preffed this feature on, so that's still somewhat problematic.  Or it could maybe be an "ac_add_options --enable-shape-outside" compile-time opt-in sort of thing, where that build option would control some #ifdef (and we could include that mozconfig line in mozilla-central's treeherder configs, but not on official Nightly builds & not on beta/release trees), perhaps.

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 8942867 [details]
> Bug 1404222 Part 5 - Adjust tolerance value in
> shape-outside-radial-gradient-004.html.
> [...]
> Also: do you know who's correct, between us & Chrome, on the rendering

I did a bit more investigation & I think Chrome is correct on this testcase -- I filed bug 1430932 with more details.
Blocks: 1430932
Comment on attachment 8942869 [details]
Bug 1404222 Part 5 - Add a crashtest.

https://reviewboard.mozilla.org/r/213080/#review219102
Attachment #8942869 - Flags: review?(dholbert) → review+
Comment on attachment 8942870 [details]
Bug 1404222 Part 8 - Update test expectations after implementing shape-outside: <image>.

https://reviewboard.mozilla.org/r/213082/#review219104
Attachment #8942870 - Flags: review?(dholbert) → review+
Blocks: 1430983
Comment on attachment 8942865 [details]
Bug 1404222 Part 3 - Adjust the #image width in shape-image-001.html.

https://reviewboard.mozilla.org/r/213072/#review219052

> I don't think this patch is actually correct. The test might need fixing, but not in this way.  In particular -- with this change, the test passes for me regardless of whether "shape-outside" support is enabled.  So I think that means it's wrong / not-what-the-author-intended.
> 
> Really, I think the test *does* intend for this element to have width:150px here -- and then the test intends for 50px of its width to get cancelled out via "shape-outside".  It's trying to achieve the same effect as if we had the following:
>  shape-outside:inset(0px 50px 0px 0px);
> 
> That ^^ "inset()" tweak makes the test pass for me in Nightly (with the original width restored).  So I think you need to investigate what needs to happen with the test and/or code to get that effect via the image here.
> 
> (If you like: that might mean leaving this test marked as "fails" for the moment & filing a followup for further investigation.)

You're right. The test pass even if "shape-outside" support is disabled.

I think the original author might want to achieve the effect like `shape-outside:inset(0px 50px 0px 0px)`. However, if I read the spec correctly, there's no way to adjust the shape image's width because the shape image's width is the element's width. I filed bug 1430983 for investigation with more details.
(In reply to Daniel Holbert [:dholbert] from comment #20)
> (In reply to Daniel Holbert [:dholbert] from comment #16)
> > Do we have code & tests to enforce that these images aren't allowed
> > cross-origin?

Yeh, I think the test mentioned in Bug 1373743 does the job.

> 
> Oh, I'm now seeing that the CORS stuff is tracked in bug 1418930, which
> doesn't have any patches yet.  I'm glad that that's tracked -- but I'm a bit
> uneasy about landing the rest of these patches until we've *also* got the
> CORS support, though, because I think (?) that means we'll opening our
> enthusiast users (users who've preffed this on) to possible-attack in the
> interim.
> 
> This shape-outside feature *is* preffed off by default, so users wouldn't be
> vulnerable by default -- but I'm worried that developers might pref it on
> for testing & leave it that way, without realizing that they're potentially
> opening themselves up to dangerous cross-origin information-stealing
> attacks.  The pref sounds trivially safe right now
> ("layout.css.shape-outside.enabled") -- even to someone who knows the
> spec/feature, since the spec has these security guarantees built-in -- so
> that's why I'm worried that developers might try turning it on & leaving it
> on without realizing that it's dangerous.
> 
> Ideally, we should hold these patches until bug 1418930 is ready.  If that's
> impractical, I think I'd also probably be OK if we had some sort of blocking
> baked in at the C++ level, so that we'll fail towards safety even in builds
> where users have toggled the pref.  I don't know what that blocking would
> look like off the top of my head, though -- simplest option would be adding
> "#ifdef NIGHTLY_BUILD" around the code that triggers the image request, but
> that'd still leave our Nightly enthusiasts potentially exposed if they've
> preffed this feature on, so that's still somewhat problematic.  Or it could
> maybe be an "ac_add_options --enable-shape-outside" compile-time opt-in sort
> of thing, where that build option would control some #ifdef (and we could
> include that mozconfig line in mozilla-central's treeherder configs, but not
> on official Nightly builds & not on beta/release trees), perhaps.

True, if we land those patches without implementing CORS stuff, it might be some hazard in the interim. I'll start bug 1418930 to see if it's easier to get it implemented than adding some work around.
Flags: needinfo?(tlin)
Comment on attachment 8942863 [details]
Bug 1404222 Part 1 - Implement shape-outside: <image>.

https://reviewboard.mozilla.org/r/213068/#review219412

Here's a partial review on "part 1", from a first-pass skim over parts of the patch.

If possible, it'd probably be nice to have dbaron do the actual review this part instead of me, since he's more familiar with our float manager and its invariants etc.  (It looks like he reviewed the most recent nontrivial changes to nsFloatManager.cpp, at least, over in bug 1326407.)  Alternately: if he can't, I can do some digging to get up to speed on how all this stuff works, too.

::: layout/generic/nsFloatManager.cpp:1014
(Diff revision 1)
> +  float aShapeImageThreshold,
> +  const nsRect& aContentRect,
> +  WritingMode aWM,
> +  const nsSize& aContainerSize)
> +{
> +  const uint8_t threshold = aShapeImageThreshold * 255;

Two things:
 (1) I think this wants to be:
    NSToIntRound(aShapeImageThreshold * 255))


 (2) You should add an assertion here that "aShapeImageThreshold" is >=0.0 and <=1.0, to validate that this assignment makes sense.

::: layout/generic/nsFloatManager.cpp:1084
(Diff revision 1)
> +  }
> +
> +  if (aWM.IsVerticalRL()) {
> +    // Because we scan the columns from left to right, we need to reverse
> +    // the array so that it's sorted (in ascending order) on the block
> +    // direction .

nit: stray space before "." here

::: layout/generic/nsFloatManager.cpp:1173
(Diff revision 1)
> +  if (IsEmpty()) {
> +    // No need to create shape info for an empty float.
> +    return;
> +  }

I think the comment might be misleading here.  AFAICT, IsEmpty() doesn't mean "empty float" -- instead, it means "empty float container" (or something like that)... right?

(Looks to me like this IsEmpty() is defined as mRect.IsEmpty(), which it looks like is basically equivalent to aContainerSize.IsEmpty().  So we're testing the passed-in aContainerSize here, basically.)

::: layout/generic/nsFloatManager.cpp:1499
(Diff revision 1)
> +/* static */ UniquePtr<nsFloatManager::ShapeInfo>
> +nsFloatManager::ShapeInfo::CreateImageShape(
> +  const UniquePtr<nsStyleImage>& aShapeImage,
> +  float aShapeImageThreshold,
> +  nsIFrame* const aFrame,
> +  WritingMode aWM,
> +  const nsSize& aContainerSize)
> +{
> +  nsImageRenderer imageRenderer(aFrame, aShapeImage.get(),

As a sanity-check (and as documentation about the implicit relationships between the args here), could you add something like:

  MOZ_ASSERT(aShapeImage ==
             aFrame->StyleDisplay()->mShapeOutside->GetShapeImage(),
             "aFrame should be the frame that we got aShapeImage from");

::: layout/painting/nsImageRenderer.h:257
(Diff revision 1)
> +  /**
> +   * Draw the image to aRenderingContext which can be used to define the
> +   * float area.

s/float area/float area in the presence of "shape-outside: <image>"/

(Since, out of context, it's not directly clear what this comment is talking about -- there are no other mentions of 'float' or 'shape-outside' in this file.)

::: layout/painting/nsImageRenderer.cpp:7
(Diff revision 1)
>  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* utility functions for drawing borders and backgrounds */

Let's update this comment, since this patch is adding code here that's unrelated to borders & backgrounds.

Maybe to this:
/* utility code for drawing images as CSS borders, backgrounds, & shapes */
Attachment #8942863 - Flags: review?(dholbert)
Comment on attachment 8942864 [details]
Bug 1404222 Part 2 - Trigger reflow after "shape-outside" image has finished decoding a frame.

https://reviewboard.mozilla.org/r/213070/#review219426

::: commit-message-3bba0:1
(Diff revision 1)
> +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame.

s/after the image/after "shape-outside" image/

::: commit-message-3bba0:3
(Diff revision 1)
> +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame.
> +
> +When we finish decode an image frame, we need to trigger reflow for the

s/finish decode/finish decoding/

::: commit-message-3bba0:4
(Diff revision 1)
> +Bug 1404222 Part 2 - Trigger reflow after the image has finished decoding a frame.
> +
> +When we finish decode an image frame, we need to trigger reflow for the
> +frame containing a float with shape image.

Perhaps:
  s/shape image/shape-outside:<image>/
?

::: layout/style/ImageLoader.cpp:388
(Diff revision 1)
> +void
> +ImageLoader::DoReflow(FrameSet* aFrameSet)
> +{

Could we call this RequestReflow instead of DoReflow?

The current name, "DoReflow", makes it sound like it synchronously reflows (inside of this function itself).  We also already have a function called DoReflow(), on PresShell, which *does* synchronously reflow.  So we shouldn't add other identically-named functions unless they've got approximately the same semantics.

(I think you were going for naming consistency with the "DoRedraw" function - but really, *that* function probably wants a rename, too, since it doesn't synchronously redraw. :)  Maybe include another patch or spin off a helper bug to rename DoRedraw, if you don't mind?)

::: layout/style/ImageLoader.cpp:394
(Diff revision 1)
> +    if (frame->StyleDisplay()->mShapeOutside.GetShapeImageData()) {
> +      // Tell the container of the float to reflow because the shape image
> +      // is ready, or changed, etc.
> +      nsIFrame* floatContainer = frame->GetInFlowParent();
> +      floatContainer->PresShell()->FrameNeedsReflow(
> +        floatContainer, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
> +    }

Two things:
 (1) I think we only need to bother with this for frames that are floated, right? (Could we add a check on...
  frame->StyleDisplay()->mFloat != StyleFloat::None
...to the outer condition here?)

 (2) I think "or changed" in the code-comment is too vague & is possibly incorrect.  (What "change" does this code actually handle?  If the actual <image> value changes e.g. to a different URL, then that gets handled in nsStyleDisplay::CalcDifference, not here, IIUC.)  I think the only "change" that can/should trigger this code are the image finishing its first frame -- so maybe reword to make that clearer? E.g.:
"...because the shape image has finished decoding its first frame"

::: layout/style/ImageLoader.cpp:508
(Diff revision 1)
> +  // We also want to reflow if the image is used by shape-outside.
> +  DoReflow(frameSet);

It looks like this will fire on *every* image-frame decode, for e.g. an animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which gets called on each frame).  So that would trigger a lot of needless reflows.

Can we avoid this, e.g. by unregistering/disassociating after this has fired once?  (Or: if we already do that, please add a comment here indicating where/how, so that it's clearer that we don't *actually intend* to reflow on every frame-complete-notification, even though it looks like that's what'd happen here.)

Also: can you make sure we have a test somewhere with an animated GIF (or PNG, or SVG) which ensures that we specifically only use the first frame for float calculations?

::: layout/style/nsStyleStruct.h:2478
(Diff revision 1)
>    {
>      MOZ_ASSERT(mType == StyleShapeSourceType::Image, "Wrong shape source type!");
>      return mShapeImage;
>    }
>  
> +  imgRequestProxy* GetShapeImageData() const;

Could you add a brief comment here, to make it clearer that this is just a convenience method & that callers don't have to bother e.g. checking the type before calling this?

Maybe:
  // Iff we have "shape-outside:<image>" w/ an image URI (not a gradient), this
  // method returns the corresponding imgRequestProxy*. Else, returns null.
Attachment #8942864 - Flags: review?(dholbert) → review-
Comment on attachment 8942868 [details]
Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes.

https://reviewboard.mozilla.org/r/213078/#review219474

::: commit-message-12e5f:1
(Diff revision 1)
> +Bug 1404222 Part 6 - Add web-platfrom-tests for linear-gradient with writing-modes.

Typo: s/platfrom/platform/  (the "r" and "o" are swapped)

::: testing/web-platform/meta/MANIFEST.json:123186
(Diff revision 1)
> +   "css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html": [
> +    [
> +     "/css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html",

Chrome on my machine doesn't pass any of the new tests in this patch, AFAICT (shape-outside-linear-gradient-005.html through 008).  I see red & misshapen squares in all of them.

Could you file a bug[1] for at least the first two (005 and 006) to be sure they're aware & to be sure we're in agreement on the correct spec-interpretation here?  (The last two tests (007 and 008) trivially fail in Chrome because they don't support sideways-rl/sideways-lr[2], so no need to file a new bug on those ones.)

[1] https://bugs.chromium.org/p/chromium/issues/
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=680331
Comment on attachment 8942868 [details]
Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes.

https://reviewboard.mozilla.org/r/213078/#review219508

::: testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/gradients/shape-outside-linear-gradient-005.html:24
(Diff revision 1)
> +    #float-left {
> +      float: left;

Note: It might be helpful to people reading/grokking this test if you added a comment inside this rule, like the following:

      /* Note: In .container's writing-mode, "float:left" actually
         floats us towards the top. */

And similar in the #float-right{...} rule (which floats us towards the bottom, IIUC.  Consider adding these comments to all 4 new testcases here.  (They'd be identical in the first 3 tests, and the comments would have "top"/"bottom" swapped in the final test).
Attachment #8942868 - Flags: review?(dholbert) → review+
Comment on attachment 8942863 [details]
Bug 1404222 Part 1 - Implement shape-outside: <image>.

https://reviewboard.mozilla.org/r/213068/#review219412

> I think the comment might be misleading here.  AFAICT, IsEmpty() doesn't mean "empty float" -- instead, it means "empty float container" (or something like that)... right?
> 
> (Looks to me like this IsEmpty() is defined as mRect.IsEmpty(), which it looks like is basically equivalent to aContainerSize.IsEmpty().  So we're testing the passed-in aContainerSize here, basically.)

I think the mRect is initialized as the aMarginRect (of the float), not aContainerSize. I'll make the comment clearer.
Comment on attachment 8942864 [details]
Bug 1404222 Part 2 - Trigger reflow after "shape-outside" image has finished decoding a frame.

https://reviewboard.mozilla.org/r/213070/#review219426

> Could we call this RequestReflow instead of DoReflow?
> 
> The current name, "DoReflow", makes it sound like it synchronously reflows (inside of this function itself).  We also already have a function called DoReflow(), on PresShell, which *does* synchronously reflow.  So we shouldn't add other identically-named functions unless they've got approximately the same semantics.
> 
> (I think you were going for naming consistency with the "DoRedraw" function - but really, *that* function probably wants a rename, too, since it doesn't synchronously redraw. :)  Maybe include another patch or spin off a helper bug to rename DoRedraw, if you don't mind?)

Yeh. `RequestReflow()` is a better name.

Filed bug 1433039 for renaming `DoRedraw()`.

> It looks like this will fire on *every* image-frame decode, for e.g. an animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which gets called on each frame).  So that would trigger a lot of needless reflows.
> 
> Can we avoid this, e.g. by unregistering/disassociating after this has fired once?  (Or: if we already do that, please add a comment here indicating where/how, so that it's clearer that we don't *actually intend* to reflow on every frame-complete-notification, even though it looks like that's what'd happen here.)
> 
> Also: can you make sure we have a test somewhere with an animated GIF (or PNG, or SVG) which ensures that we specifically only use the first frame for float calculations?

I haven't look into how to avoid firefox multiple reflow requests on every frame-complete-notification. Perhaps it suffices to fire requeust reflow once on the decode-complete phase.

testing/web-platform/tests/css/css-shapes/shape-outside/shape-image/shape-image-025.html tests animated GIF. `nsImageRenderer::DrawShapeImage` in Part 1 pass `imgIContainer::FRAME_FIRST` flag to image container's `Draw()` to ensure the first frame of the GIF is used.
Comment on attachment 8942868 [details]
Bug 1404222 Part 4 - Add web-platform-tests for linear-gradient with writing-modes.

https://reviewboard.mozilla.org/r/213078/#review219474

> Chrome on my machine doesn't pass any of the new tests in this patch, AFAICT (shape-outside-linear-gradient-005.html through 008).  I see red & misshapen squares in all of them.
> 
> Could you file a bug[1] for at least the first two (005 and 006) to be sure they're aware & to be sure we're in agreement on the correct spec-interpretation here?  (The last two tests (007 and 008) trivially fail in Chrome because they don't support sideways-rl/sideways-lr[2], so no need to file a new bug on those ones.)
> 
> [1] https://bugs.chromium.org/p/chromium/issues/
> [2] https://bugs.chromium.org/p/chromium/issues/detail?id=680331

Sure. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=805809
Attachment #8942865 - Attachment is obsolete: true
Attachment #8942867 - Attachment is obsolete: true
Attachment #8942870 - Attachment is obsolete: true
The latest patch set should have addressed all the review comments except the multiple reflow requests issue mentioned in comment 30.

This bug required dedicating time to work on. I'm sorry that I might not be able to contribute to this feature for the moment. So unassign myself.
Assignee: aethanyc → nobody
Status: ASSIGNED → NEW
See Also: → 1433039
I'll try to complete this work.
Assignee: nobody → bwerth
(In reply to Daniel Holbert [:dholbert] from comment #26)
> ::: layout/style/ImageLoader.cpp:508
> (Diff revision 1)
> > +  // We also want to reflow if the image is used by shape-outside.
> > +  DoReflow(frameSet);
> 
> It looks like this will fire on *every* image-frame decode, for e.g. an
> animated-GIF "shape-outside:[image]" (since this is in OnFrameComplete which
> gets called on each frame).  So that would trigger a lot of needless reflows.

I have confirmed it does NOT fire onFrameComplete on every image-frame decode, though I haven't figured out why yet. When I figure it out, I'll add an explanatory comment.

There is a separate problem with the implementation of DoReflow/RequestReflow: it triggers reflow in an onFrameComplete from a background-image associated with the frame, which is incorrect. I'll figure out a fix for this as well.
(In reply to Brad Werth [:bradwerth] from comment #40)
> I have confirmed it does NOT fire onFrameComplete on every image-frame
> decode, though I haven't figured out why yet. When I figure it out, I'll add
> an explanatory comment.

I think this is just event notification collapsing -- that all 3 frames in my example are decoded before the first event is fired, so my callback is only getting called once. An animated GIF with more frames will likely fire more events, so this still needs a fix. My plan is to extend ImageLoader::AssociateRequestToFrame with a new bool parameter indicating that animation refresh request is needed or not.
Attachment #8942864 - Attachment is obsolete: true
Attachment #8949216 - Attachment is obsolete: true
Attachment #8949218 - Attachment is obsolete: true
Attachment #8949219 - Attachment is obsolete: true
Attachment #8949220 - Attachment is obsolete: true
Attachment #8949221 - Flags: review?(dholbert)
Attachment #8942863 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Pulling and reposting the patches seriously confused MozReview -- I did something wrong. I've tried to setup review flags that preserve the existing r+ on Ting Yu's patches.
Yeah, I think MozReview can't handle multiple people posting patches on bus. :( It looks you've cleaned up Bugzilla to make this coherent, but even so, once you click over to MozReview, it's got two separate concepts of patch-stacks for this bug (one with TYLin's stack, and one with your version).

We might want to just abandon MozReview for this bug and use attachments for Parts 1 and 2 at least (the parts that need r?dbaron) -- IIRC, dbaron prefers normal attachments rather than MozReview anyway, particularly when MozReview goes nuts as it has here. :)
Comment on attachment 8949221 [details]
Bug 1404222 Part 7: Turn off a 'todo' in a mochitest.

https://reviewboard.mozilla.org/r/218598/#review224422

(anyway, r=me on the CORS mochitest todo-removal)
Attachment #8949221 - Flags: review?(dholbert) → review+
Attachment #8942863 - Attachment is obsolete: true
Attachment #8942863 - Flags: review?(dbaron)
Attachment #8942866 - Attachment is obsolete: true
Attachment #8942868 - Attachment is obsolete: true
Attachment #8942869 - Attachment is obsolete: true
I guess I should abandon my patch stack to avoid confusing MozReview, and let Brad re-upload his patches and set review flags properly.
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
Many of the web platform tests are intermittent. Clearing review flags until the patch consistently passes all of them (that don't also involve shape-margin).
I've confirmed that the intermittent wpt failures are due to the reftest receiving a "load" event before the shape-outside image has been applied by the nsFloatManager. I need advice on how to block the load event until the image has been loaded and floated.

bz, can you give me some advice on how to proceed? I can sort of see how image loading blocks the load event, and I imagine I can find the place where the initial reflow blocks the loading event. But I don't see how that first reflow float calculation is chained to the image loading, which would give me an indication of how to add images loaded from shape-outside into that dependency chain. All insights appreciated.
Flags: needinfo?(bzbarsky)
So I'd have thought the stuff in patch 2 -- that modifies the ImageLoader and its calling code -- should have dealt with that.

It's analogous to what that code does background images and border images.  But in this case an image load completion requires reflow, which is not true of background-image or border-image... but it *used* to be true for border-image, so there might be old code in the file's history that could be resurrected, that might be slightly preferable to the code there.  (I'd consider the code in ImageLoader::RequestReflowIfNeeded to be a little roundabout -- the old code used to handle whether reflow was needed in the registration.)

I wonder if the call to nsLayoutUtils::DeregisterImageRequest is messing things up.
(Though there might be the process of waiting for an image decode somewhere in the middle.)
In general, firing the load event will wait for the image load, but will _not_ do an extra reflow unless there are user fonts involved.  That said, if layout has been requested by then by something, it would presumably happen before we snapshot things or any time we ask for layout information.

You probably do want to associate the new request before disassociating the old one.
Flags: needinfo?(bzbarsky)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #88)
> So I'd have thought the stuff in patch 2 -- that modifies the ImageLoader
> and its calling code -- should have dealt with that.
> 
> It's analogous to what that code does background images and border images. 
> But in this case an image load completion requires reflow, which is not true
> of background-image or border-image... but it *used* to be true for
> border-image, so there might be old code in the file's history that could be
> resurrected, that might be slightly preferable to the code there.  (I'd
> consider the code in ImageLoader::RequestReflowIfNeeded to be a little
> roundabout -- the old code used to handle whether reflow was needed in the
> registration.)
> 
> I wonder if the call to nsLayoutUtils::DeregisterImageRequest is messing
> things up.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #90)
> In general, firing the load event will wait for the image load, but will
> _not_ do an extra reflow unless there are user fonts involved.  That said,
> if layout has been requested by then by something, it would presumably
> happen before we snapshot things or any time we ask for layout information.
> 
> You probably do want to associate the new request before disassociating the
> old one.

Thank you both for your help. I've confirmed that the order of registering and registering image requests is not the culprit (though the next posted patch will do as bz proposes above), and that the deregistering the shape-outside image after getting the first frame is not the culprit either. The image fetch for the shape-outside is just not getting added to the document loadgroup or doing anything else to block the load event firing.

Interestingly, in my testing, it appears that background-image loads don't block the load event, either (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage). That surprises me -- I assume that would also cause intermittent reftest failures. So I'm not finding examples that show a way to block the load event from anything other than HTMLImageElements. I'll test border-image next and see if that points to a solution.
> background-image loads don't block the load event, either
> (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage)

The "DOMContentLoaded" event and the "load" event are two different events.  The former fires when parsing is done and is not affected by pending subresource loads.  The latter _is_ affected by pending subresource loads.

> The image fetch for the shape-outside is just not getting added to the document loadgroup

That's odd.  If you're going through nsContentUtils::LoadImage then NewImageChannel should add things to the right loadgroups...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #92)
> > background-image loads don't block the load event, either
> > (nsDocument::UnblockDOMContentLoaded fires before imgLoader::LoadImage)
> 
> The "DOMContentLoaded" event and the "load" event are two different events. 
> The former fires when parsing is done and is not affected by pending
> subresource loads.  The latter _is_ affected by pending subresource loads.

Ah, that explains that result, then. What's the right place to breakpoint the "load" event firing?

> > The image fetch for the shape-outside is just not getting added to the document loadgroup
> 
> That's odd.  If you're going through nsContentUtils::LoadImage then
> NewImageChannel should add things to the right loadgroups...

I was incorrect. The image does get added to the loadgroup. The call chain is going from nsStyleDisplay::FinishStyle through to nsContentUtils::LoadImage. But if that call happens after the load event has fired, we could still see the observed behavior.
Flags: needinfo?(bzbarsky)
(In reply to Brad Werth [:bradwerth] from comment #93)
> Ah, that explains that result, then. What's the right place to breakpoint
> the "load" event firing?

I'm using nsDocumentViewer::LoadComplete now. Looks like that is the right signifier.
Flags: needinfo?(bzbarsky)
> What's the right place to breakpoint the "load" event firing?

nsDocumentViewer::LoadComplete, as you found.

> But if that call happens after the load event has fired, we could still see the observed behavior.

Right.  We should be making sure somehow that it happens before we've fired the load event...

What's the callstack to that FinishStyle call?
Here's the relevant call stacks, in order.

1) RestyleManager::ProcessRestyledFrames -> ... -> nsStyleDisplay::FinishStyle (loads the image)
2) PresShell::DoReflow -> ... -> nsFloatManager::AddFloat (first reflow, but image isn't ready yet -- only size is available)
3) nsDocumentViewer::LoadComplete (fires onload)
4) ImageLoader::onFrameComplete (image available, requests restyle)
5) PresShell::DoReflow -> ... -> nsFloatManager::AddFloat (second reflow, image ready, successful)

So the load event IS being delayed past the loading the image, contrary to my earlier repeated assertions. The actual problem is that the float can't happen on the first reflow because the first frame isn't loaded yet (nsStyleImage::IsComplete is returning false).

So solutions I can imagine are...

a) In the first nsFloatManager::AddFloat, when it's detected that the shape-outside image isn't complete, block the document onLoad, then release that on the second nsFloatManager::AddFloat. Have to handle timeouts and other nastiness here.
b) Make the image load synchronous during the first nsFlaotManager::AddFloat, or otherwise block until image is complete.

I'll start working on option a. Thank you again for your help.
Hold on.

Image _loads_ block onload, but image _decodes_ do not, last I checked.  But image decoding (apart from knowing the image size) can't affect layout ... at least until this patch.  Looking at the spec for shape-outside, it sounds like we need to block onload on decoding of shape-outside images, right?

In that case, option (a), or some way of indicating to imagelib that we want to block load on decode, seems to be the right way to go.  Presumably we are already indicating in some way that we want to do a eager decode on these images?
Blocks: 1439709
Attachment #8952483 - Attachment is obsolete: true
Attachment #8949216 - Flags: review?(dbaron)
Attachment #8949217 - Flags: review?(dbaron)
Attachment #8951808 - Flags: review?(dbaron)
Attachment #8951809 - Flags: review?(dbaron)
Attachment #8951810 - Flags: review?(dbaron)
Attachment #8951083 - Flags: review?(dbaron)
Attachment #8949218 - Flags: review?(dholbert)
Attachment #8949219 - Flags: review?(dholbert)
Attachment #8949220 - Flags: review?(dholbert)
Attachment #8949922 - Flags: review?(dholbert)
Attachment #8949547 - Flags: review?(dholbert)
Comment on attachment 8949218 [details]
Bug 1404222 Part 4: Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors.

https://reviewboard.mozilla.org/r/218592/#review228374
Attachment #8949218 - Flags: review?(dholbert) → review+
Comment on attachment 8949219 [details]
Bug 1404222 Part 5: Add web-platform-tests for linear-gradient with writing-modes.

https://reviewboard.mozilla.org/r/218594/#review228380
Attachment #8949219 - Flags: review?(dholbert) → review+
Comment on attachment 8949220 [details]
Bug 1404222 Part 6: Add a crashtest.

https://reviewboard.mozilla.org/r/218596/#review228384
Attachment #8949220 - Flags: review?(dholbert) → review+
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review228388

::: commit-message-22502:1
(Diff revision 5)
> +Bug 1404222 Part 11: Fix wpt reftest shape-image-001.html to correct a too-wide container.
> +
> +MozReview-Commit-ID: 3fwtUNCqWLX

In cases where we're editing another vendor's test, we should try to be clear about what was wrong & that the change is correct and not arbitrary.

So, in light of that -- could you provide a bit more detail in the commit message about what was wrong here and why the fix is correct?  That'll help justify the change when this is merged to upstream WPT, if anyone there gets curious about why we messed with this CSS.

(And: if Chrome/Blink "passes" this test regardless of your change, and that's incorrect, perhaps file a bug on them?)
Attachment #8949922 - Flags: review?(dholbert) → review-
Comment on attachment 8949547 [details]
Bug 1404222 Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin.

https://reviewboard.mozilla.org/r/218898/#review228390

::: commit-message-8229d:1
(Diff revision 8)
> +Bug 1404222 Part 12: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin or SVG data URIs.
> +
> +MozReview-Commit-ID: FNgeeBpFtMs

(1) Could you add a brief mention in a second line of this commit message, about why these two specific categories of tests aren't passing yet?

i.e.:
 - shape-margin: not yet supported
 - SVG Data URIs: I'm not sure

(2) ...and actually, it looks like you *are* marking some tests as PASS that *do* rely on SVG data URIs: shape-image-002.html and shape-image-005.html in particular.  So this needs a bit of clarification.
Attachment #8949547 - Flags: review?(dholbert) → review-
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review228388

> In cases where we're editing another vendor's test, we should try to be clear about what was wrong & that the change is correct and not arbitrary.
> 
> So, in light of that -- could you provide a bit more detail in the commit message about what was wrong here and why the fix is correct?  That'll help justify the change when this is merged to upstream WPT, if anyone there gets curious about why we messed with this CSS.
> 
> (And: if Chrome/Blink "passes" this test regardless of your change, and that's incorrect, perhaps file a bug on them?)

Sure, makes sense. I'll explain the rationale in an expanded commit message. Chrome does not pass the test as written.
Comment on attachment 8949216 [details]
Bug 1404222 Part 1: Implement shape-outside: <image>.

https://reviewboard.mozilla.org/r/218588/#review229352


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsFloatManager.cpp:1042
(Diff revision 10)
> +    // If we're in the area between w and aStride, skip this pixel,
> +    // or if we're vertical, skip the remainder of the loop.
> +    if (col >= w) {
> +      if (aWM.IsVertical()) {
> +        break;
> +      } else {

Warning: Do not use 'else' after 'break' [clang-tidy: readability-else-after-return]
Comment on attachment 8949547 [details]
Bug 1404222 Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin.

https://reviewboard.mozilla.org/r/218898/#review230124
Attachment #8949547 - Flags: review?(dholbert) → review+
Comment on attachment 8949922 [details]
Bug 1404222 Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container.

https://reviewboard.mozilla.org/r/219222/#review230126

::: commit-message-22502:5
(Diff revision 7)
> +Bug 1404222 Part 11: Fix wpt reftest shape-image-001.html to correct a too-wide container.
> +
> +MozReview-Commit-ID: 3fwtUNCqWLX
> +
> +The test currently stretches a 100 x 100 image to 150 pixels wide, which makes the shaded region of the png stretch to 75 pixels. None of the math in the rest of the test accounts for this stretching, and the test fails on all browsers. It seems clear that the intention was to use an unstretched, 100 pixel wide image, which makes the test pass.

(not a big deal in the grand scheme of this patch stack -- hence not ticking "open an issue" -- but you might want to rewrap this long comment to be 80 characters per line.

(Typically, we put the main commit message on a single line regardless of its length (since the first line of a commit message has special semantic meaning), but then subsequent extra-info lines get wrapped at 80 characters.)
Attachment #8949922 - Flags: review?(dholbert) → review+
Blocks: 1442702
Attachment #8955588 - Flags: review?(dholbert)
Attachment #8955588 - Flags: review?(dholbert)
Attachment #8951808 - Attachment is obsolete: true
Attachment #8951808 - Flags: review?(dbaron)
Attachment #8951809 - Attachment is obsolete: true
Attachment #8951809 - Flags: review?(dbaron)
Attachment #8951810 - Attachment is obsolete: true
Attachment #8951810 - Flags: review?(dbaron)
Attachment #8951083 - Attachment is obsolete: true
Attachment #8951083 - Flags: review?(dbaron)
Attachment #8955588 - Attachment is obsolete: true
Due to MozReview's inability to inspect old patch state in a usable way
(some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
and the fact that the MozReview history is no longer visible in bugzilla
because the comments are hidden, I'm no longer accepting review requests
in MozReview.  If you'd like me to review patches, please submit those
review requests as attached patch files.
Flags: needinfo?(bwerth)
Attachment #8949216 - Flags: review?(dbaron)

        Attachment #8949216 -
        Attachment is obsolete: true

        Attachment #8957249 -
        Flags: review?(dbaron)

    
    

    
  


  

        Attachment #8949217 -
        Attachment is obsolete: true

        Attachment #8949217 -
        Flags: review?(dbaron)
Flags: needinfo?(bwerth)

        Attachment #8957250 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8957249 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

># User Brad Werth <bwerth@mozilla.com>

Didn't Ting-Yu write part of this?  If so, his name and email should be
on here also.  (Just list both, comma-separated.)


>+  // An Interval stores line-left and line-right at a given block coordinate
>+  // in the float manager's coordinate space. The Y() of mLineLeft and
>+  // mLineRight should be the same.

This should probably use ".y" rather than "Y()" since that's far more common.
Further, I think it should be clearer that the y is actually a logical coordinate
in the block direction, and the .x is in inline direction.

nsFloatManager::ImageShapeInfo::ImageShapeInfo should
MOZ_ASSERT(aStride >= aImageSize.width).

>+  const uint8_t threshold = NSToIntRound(aShapeImageThreshold * 255);

This seems wrong.  Since the threshold is defined in the spec so that
pixels that are *more* opaque than the threshold should be used, I think
this should use NSToIntFloor, so that, if the unrounded value is 49.7,
the uint8_t will be 49 so that a value of 50 will count and a value of
49 won't.

There should also be a test that tests this... preferably in
layout/reftests/w3c-css/submitted/shapes1/.

>+  int32_t min = -1;
>+  int32_t max = -1;
...
>+      // Reset min and max for the next row or column.
>+      min = -1;
>+      max = -1;

Just declare these variables inside the outer loop rather than doing
this resetting.

>+      if (max < curr) {
>+        max = curr;
>+      }

Seems better to just:
   MOZ_ASSERT(max < curr);
   max = curr;


That said, I guess I don't see how this loop actually works for vertical
writing-modes, since it always scans the image in horizontal rows, but the
logic depends on the image being scanned an inline-direction row at a time.
It seems like in a horizontal writing mode, we'll scan one row and then
enter this condition:
  >+    // At the end of a row (or column if vertical), and found something.
  >+    if (curr == (aWM.IsVertical() ? h - 1 : w - 1) && (min != -1)) {
but in a vertical writing-mode, we'll scan almost the entire image and
then enter that condition for every pixel as we scan the bottom row,
thus putting almost all the data from the image in the interval for the
leftmost physical column of the image.  Or am I missing something here?
(I'm marking the patch review- for this.)

>+      if (aWM.IsVerticalLR() && aWM.IsSideways()) {
>+        // sideways-lr: its physical directions of line-left and line-right
>+        // are bottom and top, which are the opposite of other vertical
>+        // writing modes.
>+        lineLeft.MoveBy(colAppUnits, maxAppUnits);
>+        lineRight.MoveBy(colAppUnits, minAppUnits);
>+      } else if (aWM.IsVertical()) {
>+        lineLeft.MoveBy(colAppUnits, minAppUnits);
>+        lineRight.MoveBy(colAppUnits, maxAppUnits);
>+      } else {
>+        // horizontal-tb
>+        lineLeft.MoveBy(minAppUnits, rowAppUnits);
>+        lineRight.MoveBy(maxAppUnits, rowAppUnits);
>+      }

Given that there was once discussion of having sideways-rl, I think it's
more future proof to make the first test just for Vertical rather than
VerticalLR, and then restructure so the code is:

  if (aWM.IsVertical()) {
    if (aWM.IsSideways()) {
      // ...
    } else {
      // ...
    }
  } else {
    // ...
  }

That said, I think once the other issues are fixed, a simpler way to
structure this code may fall out.


I think the LineLeft and LineRight methods should do binary search
rather than linear search.  The data structure is even already set up for it.
(They could share the code.)

>+  if (IsEmpty()) {
>+    // Per spec, a float area defined by a shape is clipped to the float’s
>+    // margin box. Therefore, no need to create a shape info if the float's
>+    // margin rect is empty.
>+    return;
>+  }

Hmmm... could you update this comment elsewhere in nsFloatManager.cpp to cite
css-shapes saying it's actually correct:
>      // For compatibility, ignore floats with empty rects, even though it
>      // disagrees with the spec.  (We might want to fix this in the
>      // future, though.)
(Might still want to mention that CSS2 says it's incorrect... if it
still does.)


I'd like dholbert (or someone else with imagelib knowledge) to review
nsFloatManager::ShapeInfo::CreateImageShape and
nsImageRenderer::DrawShapeImage; he knows the image code better than I
do.

I'm a little worried about the interaction with decoding and discarding,
and what happens when the frame's content rect changes.



Are there tests in layout/reftests/w3c-css/submitted/shapes1/ (or
imported web-platform-tests from other browsers) that test:
 * different writing modes.  (I suspect not, given above.)
 * zoom (this actually can't be in the W3C tests since reftest-zoom
   isn't part of the format)
(and probably these should be tested with png, svg, and gradients).

        Attachment #8957249 -
        Flags: review?(dbaron) → review-

    
    

    
  
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

Do you know what the implications are of using std::unordered_set for codesize?  I'd be inclined to suggest use of our own hashtable classes instead, if you're going to use hashtables.
Flags: needinfo?(bwerth)

    
    

    
  
Also, I don't understand why it makes sense to call ForgetAllBlockingResources at the callsite.  Could you explain?  And preferably also add comments to explain.

    
    

    
  
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

I don't see why the new methods on nsIDocument should be virtual; they
could just be like DefaultStyleAttrURLData, I'd think, and be nsIDocument
methods implemented in nsDocument.cpp.

(more later...)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #224)
> Comment on attachment 8957250 [details] [diff] [review]
> Bug 1404222 Part 2: Block onload when shape-outside images are first
> decoded, and keep it blocked until a second reflow is complete.
> 
> I don't see why the new methods on nsIDocument should be virtual; they
> could just be like DefaultStyleAttrURLData, I'd think, and be nsIDocument
> methods implemented in nsDocument.cpp.

Yeah, agreed, that would be extremely nice, specially since I'm devirtualizing them lately (bug 1443553, bug 1443756, bug 1444580). We have a lot of stuff that that declares a function as pure virtual, then only implements it once, it'd be nice to stop using this pattern :)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #223)
> Also, I don't understand why it makes sense to call
> ForgetAllBlockingResources at the callsite.  Could you explain?  And
> preferably also add comments to explain.

This was a misfire. In an earlier formulation of the patch, I used this callsite to prevent a load timeout. With the current state of the patch, I think the only way we can get a timeout is if the shape-outside image reports its size but never decodes its first frame (if OnSizeAvailable fires but OnFrameComplete doesn't). I *think* that is impossible and therefore the code is safe from a timeout.

I've removed the callsite to ForgetAllBlockingResources, and the function definition itself. If future callers start using BlockOnloadOnResource and need a way to clear out all resources at once, that function can be re-created (and tested) then.

    
    

    
  
Comment on attachment 8957250 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

... continued from comment 222, comment 223, comment 224...


nsDocument::UnblockOnloadOnResource should probably assert that
aResource is present in mOnloadBlockingResources (maybe using the return
value of erase?), unless there's a good reason not to.

nsDocument::BlockOnloadOnResource should assert that resource isn't
already in the set.  (Except it seems like that assertion will fail if
multiple elements use the same imgIRequest, which can certainly
happen...  but is the code all going to work correctly in that case?)

The code in ImageLoader.cpp that figures out if the current imgRequest
is the shape-outside of the frame (in both BlockOnloadIfNeeded and
RequestReflowIfNeeded) is quite awkward -- could you instead pass a
value (probably an enum?) in so that you don't have to do this?

Why does BlockOnloadIfNeeded happen in OnSizeAvailable?  Isn't that once
the beginning of the image has arrived from the network?  Couldn't
onload have fired already by that point?  (You should be able to add a
test for this with a loading mechanism that introduces a small delay,
though maybe it's not worth it.)

What happens when a frame is destroyed while its shape-outside image is
loading (say, because the page made an ancestor display:none?)?
nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
ImageLoader::DropRequestsForFrame... which doesn't remove the resource
from the map... which seems like it will then block onload forever.

If you fix that in the obvious way, then what happens if multiple
elements use the same imgIRequest?  We'd call BlockOnloadOnResource
multiple times.  What if we then (with the new codepath added to fix the
previous issue) remove the imgIRequest from the map?  Doesn't that then
mean that we then won't wait for the image to fire onload, even though
other elements are still waiting for it?

I'm also not crazy about using the nsIReflowCallback mechanism.  It's an
old clunky thing that I'd wish would go away; I'd rather not add another
user.

I'd also suggest that GetShapeImageData return imgIRequest* rather than
imgRequestProxy*, since that's all the caller needs.

I think it might be worth looking at the old code that we had for
border-image back when border-image affected layout.  It may have
avoided some or all of these problems.  Though maybe not...

        Attachment #8957250 -
        Flags: review?(dbaron) → review-

    
    

    
  


  

        Attachment #8957249 -
        Attachment is obsolete: true

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #221)
> Comment on attachment 8957249 [details] [diff] [review]
> Bug 1404222 Part 1: Implement shape-outside: <image>.
> That said, I guess I don't see how this loop actually works for vertical
> writing-modes, since it always scans the image in horizontal rows, but the
> logic depends on the image being scanned an inline-direction row at a time.

The loop does work as written. I added a better comment in the updated Part 1 patch:

>// Scan the pixels row by row, from top to bottom (if vertical, column by
>// column, from left to right). We iterate generically over the total
>// number of pixels, and then calculate col and row to create the desired
>// traversal order (by rows for horizontal writing modes, by columns
>// for vertical writing modes).
Flags: needinfo?(bwerth)

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #231)
> The loop does work as written. I added a better comment in the updated Part
> 1 patch:
> 
> >// Scan the pixels row by row, from top to bottom (if vertical, column by
> >// column, from left to right). We iterate generically over the total
> >// number of pixels, and then calculate col and row to create the desired
> >// traversal order (by rows for horizontal writing modes, by columns
> >// for vertical writing modes).

Oh, I guess I'd convinced myself that it was looping over the pixels in the order they were in aAlphaPixels.

Given that it's not -- could you just make it a pair of nested loops over rows and cols?  I think that would be much clearer, wouldn't require manually skipping the padding for the stride, and would allow the per-row logic to be inside the outer loop but outside the inner loop, avoiding the need for special conditions?


(Also, please don't miss comment 227.)
Flags: needinfo?(bwerth)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #232)
> Given that it's not -- could you just make it a pair of nested loops over
> rows and cols?  I think that would be much clearer, wouldn't require
> manually skipping the padding for the stride, and would allow the per-row
> logic to be inside the outer loop but outside the inner loop, avoiding the
> need for special conditions?

I'm not sure this is going to be clearer. Since we want to iterate col within row for horizontal, and row within col for vertical, the loop logic is going to look pretty weird. I'll do my best and you can decide if you think it is an improvement.

    
    

    
  


  

        Attachment #8958227 -
        Attachment is obsolete: true

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #227)
> Comment on attachment 8957250 [details] [diff] [review]
> nsDocument::UnblockOnloadOnResource should probably assert that
> aResource is present in mOnloadBlockingResources (maybe using the return
> value of erase?), unless there's a good reason not to.
> 
> nsDocument::BlockOnloadOnResource should assert that resource isn't
> already in the set.  (Except it seems like that assertion will fail if
> multiple elements use the same imgIRequest, which can certainly
> happen...  but is the code all going to work correctly in that case?)

I may be thinking of this wrong, but I see the current behavior as a feature. I wanted to make a mechanism that would be useful for a different calling pattern, and earlier versions of the patch required support for that calling pattern, with mismatched calls. Assuming the intent of blocking onload on a resource is to unblock as soon as the resource is "ready", then duplicate block requests SHOULD be coalesced, and the first unblock request should be the only one that matters. There is some value in asserting on the unblock of a resource that was never blocked, but since that doesn't cause any problems (there was never a block!) it feels like at most it should be a warning, not an assert. A reference count like system would also just be identical to calling BlockOnload and UnblockOnload on the nsDocument directly -- since the calls have to be paired. If you feel strongly that it should be more like a reference count, I'll strip out these new methods and revert to direct calls of BlockOnload and UnblockOnload.

> The code in ImageLoader.cpp that figures out if the current imgRequest
> is the shape-outside of the frame (in both BlockOnloadIfNeeded and
> RequestReflowIfNeeded) is quite awkward -- could you instead pass a
> value (probably an enum?) in so that you don't have to do this?

I agree it's clunky. I'll try to find a more elegant method.

> Why does BlockOnloadIfNeeded happen in OnSizeAvailable?  Isn't that once
> the beginning of the image has arrived from the network?  Couldn't
> onload have fired already by that point?  (You should be able to add a
> test for this with a loading mechanism that introduces a small delay,
> though maybe it's not worth it.)

I think this is a safe location to start blocking. All the images in the stylesheet are treated as "dependent resources" so the first layout can't occur until sizes are read. The onload can't fire until that layout is complete. So if we block onload when the size is read, we're adding a block within the already existing block/unblock pair, and there is no coverage gap.

> What happens when a frame is destroyed while its shape-outside image is
> loading (say, because the page made an ancestor display:none?)?
> nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
> ImageLoader::DropRequestsForFrame... which doesn't remove the resource
> from the map... which seems like it will then block onload forever.

This would block onload, yes. I'll have to find a solution for this.

> If you fix that in the obvious way, then what happens if multiple
> elements use the same imgIRequest?  We'd call BlockOnloadOnResource
> multiple times.  What if we then (with the new codepath added to fix the
> previous issue) remove the imgIRequest from the map?  Doesn't that then
> mean that we then won't wait for the image to fire onload, even though
> other elements are still waiting for it?

This is one reason to keep the semantics I propose at the beginning of this response. I think those semantics are useful for these cases.

> I'm also not crazy about using the nsIReflowCallback mechanism.  It's an
> old clunky thing that I'd wish would go away; I'd rather not add another
> user.

I agree it feels bad to allocate a callback object and then have it delete itself. I would love to find another solution to trigger an event at the end of layout. Please let me know of other examples I could use as a template.

> I'd also suggest that GetShapeImageData return imgIRequest* rather than
> imgRequestProxy*, since that's all the caller needs.

I'll do that.

> I think it might be worth looking at the old code that we had for
> border-image back when border-image affected layout.  It may have
> avoided some or all of these problems.  Though maybe not...

Yes, you mentioned this before. I struggled to find the original border-image bug. I'll be more diligent and find it.
Flags: needinfo?(bwerth)

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #233)
> I'm not sure this is going to be clearer. Since we want to iterate col
> within row for horizontal, and row within col for vertical, the loop logic
> is going to look pretty weird. I'll do my best and you can decide if you
> think it is an improvement.

Sorry -- I meant that the loops would be over the logical rows/cols, not the physical ones.  So the pixel index calculation would have to vary on writing mode.

(In reply to Brad Werth [:bradwerth] from comment #236)
> I may be thinking of this wrong, but I see the current behavior as a
> feature. I wanted to make a mechanism that would be useful for a different
> calling pattern, and earlier versions of the patch required support for that
> calling pattern, with mismatched calls. Assuming the intent of blocking
> onload on a resource is to unblock as soon as the resource is "ready", then
> duplicate block requests SHOULD be coalesced, and the first unblock request
> should be the only one that matters. There is some value in asserting on the
> unblock of a resource that was never blocked, but since that doesn't cause
> any problems (there was never a block!) it feels like at most it should be a
> warning, not an assert. A reference count like system would also just be
> identical to calling BlockOnload and UnblockOnload on the nsDocument
> directly -- since the calls have to be paired. If you feel strongly that it
> should be more like a reference count, I'll strip out these new methods and
> revert to direct calls of BlockOnload and UnblockOnload.

I guess it could be viable, but you need to figure out how to handle what happens when one particular thing no longer needs the resource, but something else might.

> I think this is a safe location to start blocking. All the images in the
> stylesheet are treated as "dependent resources" so the first layout can't
> occur until sizes are read. The onload can't fire until that layout is
> complete. So if we block onload when the size is read, we're adding a block
> within the already existing block/unblock pair, and there is no coverage gap.

OK, that should be explained in a comment.

    
    

    
  


  

        Attachment #8958315 -
        Attachment is obsolete: true

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #227)
> Comment on attachment 8957250 [details] [diff] [review]

> What happens when a frame is destroyed while its shape-outside image is
> loading (say, because the page made an ancestor display:none?)?
> nsFrame::DestroyFrom calls PresShell::NotifyDestroyingFrame which calls
> ImageLoader::DropRequestsForFrame... which doesn't remove the resource
> from the map... which seems like it will then block onload forever.

I've found a solution for this. ImageLoader::DropRequestsForFrame is also used for frame recreation, so it wasn't suitable to use as the point to unblock onload on the document. Instead I focused on cases where the imgIRequest was going away, being untracked, etc. Those places are:

* ImageLoader::OnFrameComplete: Will unblock the document even if no frames are associated with the request anymore.
* ImageLoader::ClearFrames: If all the frames are going away, requests get cleared, too.
* ImageLoader::RemoveImage: This gets called by the css::ImageValue destructor. The request itself is being destroyed.

    
    

    
  


  

        Attachment #8958593 -
        Attachment is obsolete: true

    
  

        Attachment #8958925 -
        Flags: review?(dbaron)

    
  

        Attachment #8958925 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8958925 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

I'd suggest the following renames in the ImageShapeInfo constructor, to
be closer to our conventions for logical sizes and positions ("inline"
and "block" style naming):

  outerLimit -> bSize
  innerLimit -> iSize
  outer -> b (same with AppUnits suffix)
  inner -> i
  min -> iMin (same with AppUnits suffix)
  max -> iMax (same with AppUnits suffix)

I'd also note I'm a little disturbed by the computations here being done
in CSS pixels; that seems to imply that the image was sized with image
pixels matching CSS pixels.  I think it would probably be better to size
the image in device pixels; this will be more accurate on high-DPI
displays.

In MinIntervalIndexWithYLargerThan, I think you should rename iStart,
iEnd, and iMid to startIdx, endIdx, midIdx, since iStart and iEnd in
layout code tend to mean "inline" rather than "index".  (See previous
comment on naming.)

>+    if (midY < aTargetY) {

If you change this to (midY <= aTargetY) it will both:
* match the function's description more clearly
* allow you to remove this bit below:
  >+      if (midY == aTargetY) {
  >+        break;
  >+      }
  (which also violates the "greater than" condition)

>+      iStart = iMid;

This should be iStart = iMid + 1.

This is since the code is structured so that iStart and iEnd both might
be the answer (given how they're initialized), but iEnd might not be a
valid index.  Since the test (with the correction above) means that iMid
doesn't satisfy the condition of having a y() value (midY) greater than
aTargetY, so iStart can be iMid + 1.

This, in turn, means that you can remove:
  >+      if (iStart == iMid) {
  >+        break;
  >+      }
since you don't need it to get the loop to terminate.

So, overall, that simplifies the loop (without the renames above) to be:

+  while (iStart < iEnd) {
+    size_t iMid = iStart + (iEnd - iStart) / 2;
+    nscoord midY = mIntervals[iMid].mLineLeft.Y();
+    if (midY <= aTargetY) {
+      iStart = iMid + 1;
+    } else {
+      iEnd = iMid;
+    }
+  }

It also means that if you instead wanted the description of the function
to be "greater than or equal" (and the name changed to match) you could
just change the "<=" to "<".

(It might also be clearer if you flip the then/else branches and invert
the test, though.)

>+  for (size_t i = MinIntervalIndexWithYLargerThan(aBStart);
>+          i < intervalCount;
>+          ++i) {

This should just be two lines, with the i on the second line lined up
with the size_t on the first.

But this also seems like the wrong interval to start considering; you
should be considering an interval that starts before aBStart as long as
it *ends* after aBStart.

>+      auto& interval = mIntervals[i];

strange indentation on this line

>-/* utility functions for drawing borders and backgrounds */
>+/* This file contains utility code for drawing images as CSS borders,
>+ * backgrounds, and shapes. */

Skip the "This file contains ".  These comments are designed to show up
in https://dxr.mozilla.org/mozilla-central/source/layout/painting so
they should be short.

        Attachment #8958925 -
        Flags: review?(dbaron) → review-

    
    

    
  
Comment on attachment 8958927 [details] [diff] [review]
Bug 1404222 Part 2: Block onload when shape-outside images are first decoded, and keep it blocked until a second reflow is complete.

In nsIDocument.h, use nsPtrHashKey<void> (no "*"), and ditch the (void**)
casts in nsDocument.cpp.

I'm still not happy about IsRequestForShapeOutsideOnFrame (which is now
refactored, but still doing fundamentally the same thing I was unhappy
about in comment 227).  I would much prefer this information be passed
in from the caller.  It's the more normal way of writing this sort of
thing, and it better extends to adding new features that work in similar
ways.  (For example, could the |FrameSet| typedef be an array of structs
that have both a frame and a word of flags, where the first flag is
whether the frame requires reflow?)

>+  // We may need reflow (for example if the image is from shape-outside).
>+  bool reflowRequested = RequestReflowIfNeeded(frameSet, aRequest);
>+  if (reflowRequested) {
>+    // Currently, the only reason we would request reflow is because the
>+    // image is used by shape-outside, which means that we don't want
>+    // to get any more of these events. Deregister with the refresh driver.
>+    nsPresContext* presContext = GetPresContext();
>+    if (presContext) {
>+      nsLayoutUtils::DeregisterImageRequest(presContext,
>+                                            aRequest,
>+                                            nullptr);
>+    }
>+  }

I didn't catch this last time, but...

What happens here if the image is used both as a background-image and
(on a different frame) as a shape-outside?  (This isn't a particularly
farfetched scenario.)  Will this do the wrong thing for the one with the
background-image?  (Consider also the case of somebody *later* using
the same request as a background-image.)  Or does something guarantee
that we never get the same imgIRequest pointer?  If so, you should add
assertions to verify that and explain what would go wrong if the
condition didn't hold.

Speaking of which... what happens when 'shape-outside' refers to an
animated image?  Should the layout change as the image animates?  Does
this approach work for that?

(There should be a test for 'shape-outside' with an animated image, at
the very least to make sure it doesn't crash or assert, but preferably
more.)



Also, I guess I don't see a reason for the BlockOnloadOnResource /
UnblockOnloadOnResource API to be on the document at all.  Why aren't
they just methods on the image loader with the hashtable on the image
loader?  It seems like the more widely they're used, the more likely
things are to get confused.

I'd also prefer it to be based on the image loader's own state rather
than a duplicate set of state in a separate hashtable, actually.  If
whether we're blocked/unblocked could be determined from the contents of
the mRequestToFrameMap (or mFrameToRequestMap, if that makes more sense)
and perhaps a counter, that would seem cleaner and also less likely to
get out of sync.  (See my previous comment about FrameSet being an array
of structs.)

(I guess part of what I'm saying is that I don't know how to verify that
this patch makes the UnblockOnloadOnResource calls in the right places,
and I'm suggesting a way to structure it that I think would be easier to
verify and maintain.)

I guess I should have been clearer about suggesting this particular
approach in the previous review, although I hadn't really come up with
all of it at the time.

        Attachment #8958927 -
        Flags: review?(dbaron) → review-

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #246)
> Comment on attachment 8958927 [details] [diff] [review]
> I'm still not happy about IsRequestForShapeOutsideOnFrame (which is now
> refactored, but still doing fundamentally the same thing I was unhappy
> about in comment 227).  I would much prefer this information be passed
> in from the caller.  It's the more normal way of writing this sort of
> thing, and it better extends to adding new features that work in similar
> ways.  (For example, could the |FrameSet| typedef be an array of structs
> that have both a frame and a word of flags, where the first flag is
> whether the frame requires reflow?)
I see. I understand your proposed solution better, now. I'll implement it as you suggest.

> What happens here if the image is used both as a background-image and
> (on a different frame) as a shape-outside?  (This isn't a particularly
> farfetched scenario.)  Will this do the wrong thing for the one with the
> background-image?  (Consider also the case of somebody *later* using
> the same request as a background-image.)  Or does something guarantee
> that we never get the same imgIRequest pointer?  If so, you should add
> assertions to verify that and explain what would go wrong if the
> condition didn't hold.
> 
> Speaking of which... what happens when 'shape-outside' refers to an
> animated image?  Should the layout change as the image animates?  Does
> this approach work for that?
Ah, that is a problem since this code would prevent the background-image from animating. I'll address that. The de-registering that happens here was intended as an optimization since shape-outside only needs the first frame, per spec. I'll instead change it to only generate the float area on the first frame complete notification, but not actually deregister for future events.

> (There should be a test for 'shape-outside' with an animated image, at
> the very least to make sure it doesn't crash or assert, but preferably
> more.)
This is tested by the wpt test css/css-shapes/shape-outside/shape-image/shape-image-025.html.

> Also, I guess I don't see a reason for the BlockOnloadOnResource /
> UnblockOnloadOnResource API to be on the document at all.  Why aren't
> they just methods on the image loader with the hashtable on the image
> loader?  It seems like the more widely they're used, the more likely
> things are to get confused.
> 
> I'd also prefer it to be based on the image loader's own state rather
> than a duplicate set of state in a separate hashtable, actually.  If
> whether we're blocked/unblocked could be determined from the contents of
> the mRequestToFrameMap (or mFrameToRequestMap, if that makes more sense)
> and perhaps a counter, that would seem cleaner and also less likely to
> get out of sync.  (See my previous comment about FrameSet being an array
> of structs.)
Yes, I agree that the logic can and should live within ImageLoader. I'll move it there. I'll try to find a way to tie it to the internal state instead of using a potentially redundant set of blocking imgIRequest pointers.

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #245)
> Comment on attachment 8958925 [details] [diff] [review] 
> I'd also note I'm a little disturbed by the computations here being done
> in CSS pixels; that seems to imply that the image was sized with image
> pixels matching CSS pixels.  I think it would probably be better to size
> the image in device pixels; this will be more accurate on high-DPI
> displays.
I still stumble on the different pixel definitions and I was unsure how to choose the correct one here. I confess I'm still fuzzy on it. If device pixels have a higher resolution than css pixels, that means the rendering in nsFloatManager::ShapeInfo::CreateImageShape would generate a denser bitmap. Since this algorithm has to process each pixel in that bitmap, I assume we will have to make a judgment call about how dense is too dense. Does that consideration make CSS pixels the right choice here, even if less accurate?
Flags: needinfo?(dbaron)

    
    

    
  
I tend to think the right thing to do is the full accuracy of what we're rendering -- if that turns out to be a performance problem I think we could consider optimizing it later to a different resolution.
Flags: needinfo?(dbaron)

    
    

    
  
I guess we might want to choose a higher resolution to generate a denser bitmap even if the CSS pixel and the device pixel has 1:1 mapping to fix bug 1430932.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  

        Attachment #8958925 -
        Attachment is obsolete: true

        Attachment #8958925 -
        Flags: review?(dholbert)

        Attachment #8960383 -
        Flags: review?(dbaron)

    
    

    
  
Current patch structure addresses these issues:
1) Math is done in dev pixels rather than css pixels (higher resolution).
2) Onload blocking is now more closely tied to the state of the request-to-frame map in the ImageLoader.
3) ImageLoader associates flags with each request/frame pair (in the request-to-frame map) and one of those flags notes when a request is for a shape-outside image which will require special processing.

    
    

    
  
Comment on attachment 8960383 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

>+  // An Interval stores the inline axis left and right edges of the
>+  // float area for one block axis pixel. These three values are
>+  // stored in two points; the x() coordinates of the two points are
>+  // inline axis edges, and the y() coordinates are both the block
>+  // axis pixel value.

This comment should be clearer that the interval really represents a
rectangle that is one device pixel high, not just the line between the
two points.  It might be worth repeating that explanation in LineEdge.

>+    mBEnd = mIntervals[mIntervals.Length() - 1].mLineLeft.Y();

Do you need to add aAppUnitsPerDevPixel here so that you get the bottom
of the row?

>+/* Utility code for drawing images as CSS borders, backgrounds, and shapes. */

start with lowercase "u" to match other files


r=dbaron with that

        Attachment #8960383 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8960383 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

I'd like dholbert to review the image code here, though.

        Attachment #8960383 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8960384 [details] [diff] [review]
Bug 1404222 Part 2: Extend ImageLoader to associate flags with each request-frame relationship.

Maybe FrameWithFlagsComparitor() could be called FrameEqualityOnlyComparitor() or FrameOnlyComparitor() or something that says that it compares the frame but not the flags?

    
    

    
  
Comment on attachment 8960385 [details] [diff] [review]
Bug 1404222 Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed or reflow is complete.

Remove the #include from nsDocument.h.

>+    if (newShapeImage) {
>+      imageLoader->AssociateRequestToFrame(newShapeImage, this);
>+    }
>+    if (oldShapeImage && HasImageRequest()) {
>+      imageLoader->DisassociateRequestFromFrame(oldShapeImage, this);
>+    }

Perhaps put these in the other order like in the border-image case?  It
would at least keep the arrays in the frameToRequestMap a little smaller
in some cases.


>+    const nsStyleDisplay* display = aFrame->StyleDisplay();
>+    if (display->mFloat != StyleFloat::None &&
>+        display->mShapeOutside.GetShapeImageData() == aRequest) {

Could you add a flag parameter to AssociateRequestToFrame, and then test
that flag here, instead of doing this computation here?  (And maybe call
it REQUEST_REQUIRES_REFLOW rather than REQUEST_IS_SHAPE_OUTSIDE, and
document that it means that onload will block?)


The code you've added to ImageLoader::AssociateRequestToFrame doesn't
handle the case if the request is already there, but without the flags
set.  At first glance:
 * I'd think you need to add the flags to the existing entry.
 * I think it would also be good if ImageLoader::AssociateRequestToFrame
   asserted that the request already being present in the frameSet is equal
   to whether the request is already present in the requestSet.

However, this exposes the somewhat deeper problem that the existing code
is wrong, and doesn't deal correctly with (for example) an image being
used three times by a frame, and then changing to being used only twice
by the frame.  (This is actually probably most likely with multiple
background images or multiple mask images.)  This change makes it worse
because there's a risk of getting the flags wrong... although there's
also still the risk of getting the invalidation wrong.  It seems we
mostly cover this up by calling nsIFrame::AssociateImage whenever we
paint, I suppose on the theory that some associations might not go
through DidSetStyleContext.  It's not clear to me if that's actually
true, or if that code exists (today, at least) only to work around the
other bug.  Filing a followup bug to fix this mess would probably be
good.

It's not immediately clear to me whether the better path to fixing this
involves:
 * allowing multiple entries with the same frame but different flags
   (which requires somewhat complex management), or
 * always readding the association for the shape-outside image in
   DidSetStyleContext (or at least always doing it if any associations
   have been removed for border images or background images)
though I suspect the latter will be a lot simpler, though a little
slower.


UnblockOnloadIfNeeded now seems unnecessarily complicated.  At the
caller you already have almost all of the data you need, but then you
find it again inside UnblockOnloadIfNeeded.  You should replace the call
to UnblockOnloadIfNeeded and the call to RemoveElementSorte with:
 * call frameSet->BinaryIndexOf()
 * if the result isn't NoIndex:
   * look at the entry
   * unblock onload if the flag is set
   * remove the entry, by index
and then delete UnblockOnloadIfNeeded.

        Attachment #8960385 -
        Flags: review?(dbaron) → review-

    
    

    
  
Comment on attachment 8960383 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

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

::: layout/painting/nsImageRenderer.cpp
@@ +965,5 @@
> +  }
> +
> +  ImgDrawResult result = ImgDrawResult::SUCCESS;
> +
> +  switch (GetType()) {

just use "mType" directly here, rather than GetType().

(It's a bit more direct, and it's what we do elsewhere in this file to switch over the type.)

@@ +968,5 @@
> +
> +  switch (GetType()) {
> +    case eStyleImageType_Image: {
> +      nsIntSize imageIntSize = CSSIntSize::FromAppUnitsRounded(mSize).ToUnknownSize();
> +      result = mImageContainer->Draw(&aRenderingContext, imageIntSize,

(1) This "ToUnknownSize()" API only exists as an intermediate step for an in-progress code-rewrite, per the XXX comment above its declaration:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/gfx/2d/Point.h#289-297

So we should probably be using some actual units here...  But maybe don't bother too much with that, because:

 (2) I think you really want to be calling nsLayoutUtils::DrawSingleImage(...), and let that handle the "app unit to pixel value" conversions for you.  (Otherwise: right now, this would be the only place in this file where we're *directly* calling mImageContainer->Draw(...), and that seems like a red flag.)

Or really, maybe this whole function wants to be wrapping the more-general "nsImageRenderer::Draw()" function? (It handles both images and gradients, and seems to be a generalization of what we're doing here.)  Is there a reason we can't share that general codepath here?  If so, it'd be nice to document that somewhere.  Otherwise, it seems like it'd be better to share code...

        Attachment #8960383 -
        Flags: review?(dholbert) → review-

    
    

    
  
Er, maybe scratch that last paragraph -- it looks like nsImageRenderer::Draw() calls "nsLayoutUtils::DrawBackgroundImage()", which is probably not what you want here.  So maybe never mind about calling into that function.

So I suspect nsLayoutUtils::DrawSingleImage() is what you really want to be calling here, at least for the "image" (not gradient) codepath.

    
    

    
  
Comment on attachment 8960383 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

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

A few more nits on part 1:

::: layout/generic/nsFloatManager.cpp
@@ +1558,5 @@
> +  nsDeviceContext* dc = aFrame->PresContext()->DeviceContext();
> +  int32_t appUnitsPerDevPixel = dc->AppUnitsPerDevPixel();
> +  LayoutDeviceIntSize imageIntSize =
> +    LayoutDeviceIntSize::FromAppUnitsRounded(contentRect.Size(),
> +                                             appUnitsPerDevPixel);

"imageIntSize" is misnamed, I think.  That name sounds like it's the image's intrinsic size. But really this has nothing to do with the image-size -- rather, this is the content-box size in dev pixels, I think?

So this probably wants to be named "contentSizeInDevPixels"...?

@@ +1563,5 @@
> +
> +  // Use empty CSSSizeOrRatio to force set the preferred size as the frame's
> +  // content box size scaled to dev pixels
> +  nsSize contentSizeForDevPixels(contentRect.Size() * (AppUnitsPerCSSPixel() /
> +                                                       appUnitsPerDevPixel));

This variable feels problematic for two reasons:
 (a) its name is quite similar to my above-suggested rename for "imageIntSize", so it's easy to confuse. :)
 (b) its units (and what it represents) are hard to pin down.  It's in app-units (nscoord) but it's not really in any coordinate-space that we typically work with -- it's got some sort of CSSPixel::DevPixel scaling applied to it.

We only use it in one spot -- for imageRenderer.SetPreferredSize() -- so I want to guess that its weird coordinate-space is unnecessary & is really indicative that there's a bug in imageRenderer.DrawShapeImage() somewhere. (Perhaps we need a coordinate-space conversion somewhere inside of that function, which this conversion is prematurely handling for us right now?)

@@ +1567,5 @@
> +                                                       appUnitsPerDevPixel));
> +  imageRenderer.SetPreferredSize(CSSSizeOrRatio(), contentSizeForDevPixels);
> +
> +  RefPtr<gfx::DrawTarget> drawTarget =
> +    gfx::Factory::CreateDrawTarget(gfx::BackendType::SKIA,

CreateDrawTarget is fallible. Please null-check its result & return nullptr if it fails.

@@ +1570,5 @@
> +  RefPtr<gfx::DrawTarget> drawTarget =
> +    gfx::Factory::CreateDrawTarget(gfx::BackendType::SKIA,
> +                                   imageIntSize.ToUnknownSize(),
> +                                   gfx::SurfaceFormat::A8);
> +  RefPtr<gfxContext> context = gfxContext::CreateOrNull(drawTarget);

This method is also sort-of fallible (but infallible if passed a non-null drawTarget, it looks like).

Please add an assertion like this:
  MOZ_ASSERT(context); // already checked the target above
...to match the convention that we use in several places in e.g. gfxUtils.cpp, like here:
https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/gfx/thebes/gfxUtils.cpp#485-486

@@ +1593,5 @@
> +             "Who changes the size?");
> +
> +  uint8_t* alphaPixels = map.GetData();
> +  int32_t stride = map.GetStride();
> +  return MakeUnique<ImageShapeInfo>(alphaPixels,

Please add a comment here saying e.g.
  // NOTE: ImageShapeInfo does not keep a persistent copy of "alphaPixels";
  // it's only used during the constructor to compute pixel ranges.

(since otherwise this looks suspiciously like it might be returning an object with a pointer to soon-to-be-deleted memory)

    
    

    
  
Comment on attachment 8960384 [details] [diff] [review]
Bug 1404222 Part 2: Extend ImageLoader to associate flags with each request-frame relationship.

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

r=me on part 2, with nits:

::: layout/style/ImageLoader.h
@@ +84,5 @@
> +  // These are used for special handling of events for requests.
> +  typedef uint32_t FrameFlags;
> +
> +  struct FrameWithFlags {
> +    FrameWithFlags(nsIFrame *aFrame)

nit: shift the "*" to the left of the space

@@ +90,5 @@
> +      mFlags(0)
> +    {
> +      MOZ_ASSERT(mFrame);
> +    }
> +    nsIFrame* mFrame;

Please add "const" after the star, to enforce that the mFrame pointer doesn't change after we've constructed an instance of this struct. i.e.:
  nsIFrame* const mFrame;

        Attachment #8960384 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #262)
> Comment on attachment 8960383 [details] [diff] [review]
> This comment should be clearer that the interval really represents a
> rectangle that is one device pixel high, not just the line between the
> two points.  It might be worth repeating that explanation in LineEdge.
> 
> >+    mBEnd = mIntervals[mIntervals.Length() - 1].mLineLeft.Y();
> 
> Do you need to add aAppUnitsPerDevPixel here so that you get the bottom
> of the row?

Thank you for this. Your comments have clarified for me that an interval should really be nsRect that is one block-axis pixel "high" (stored in the y coordinates). This actually simplifies some of the code and makes the patch cleaner in addition to being more correct.

    
    

    
  
Comment on attachment 8960385 [details] [diff] [review]
Bug 1404222 Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed or reflow is complete.

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

A few thoughts on part 3 (not sure how relevant they are after dbaron's review feedback):

::: layout/style/ImageLoader.h
@@ +74,5 @@
> +  struct ImageReflowCallback : public nsIReflowCallback
> +  {
> +    ImageLoader* mLoader;
> +    nsIFrame* mFrame;
> +    imgIRequest* mRequest;

If possible, these pointers should be declared with "const" after the asterisk, since I think they're all intended to be immutable.

THOUGH: since there's some time delay between when this ImageReflowCallback object is initialized & gets fired, I worry a little about the lifetimes of these objects... I think *at least* mLoader should probably be a RefPtr<ImageLoader>, since that's the one that we actually dereference when this callback fires, and we want to be sure that dereference isn't a use-after-free.

(We might want to make mRequest a nsCOMPtr, but that & mFrame are just used as hash keys, I think...? So the consequences are less bad if they happen to be destroyed before we use them [though I can still imagine there might be at least minor brokenness if we did happen to destroy & recreate a frame with the same address before this callback fires].  It'd be useful to explicitly document that somewhere, though, to remind ourselves not to dereference them...  and/or to document how we're sure that that's not a problem.)

@@ +83,5 @@
> +    : mLoader(aLoader)
> +    , mFrame(aFrame)
> +    , mRequest(aRequest)
> +    {}
> +    virtual ~ImageReflowCallback() {}

I don't think this needs to be declared as virtual, since this class doesn't have any subclasses.  In fact, we don't even need to declare a destructor in the first place, since it doesn't have anything to do.  So let's just delete this line.

(And maybe declare the struct as "final", to be sure that we truly don't have any subclasses & hence don't need to worry about declaring a virtual destructor.)

(Note that our superclass nsIReflowCallback doesn't have a virtual destructor, but that's fine, because nobody ever calls "delete foo" on any "nsIReflowCallback* foo" pointers.)

        Attachment #8960385 -
        Flags: review?(dholbert) → review-

    
    

    
  


  

        Attachment #8960383 -
        Attachment is obsolete: true

        Attachment #8961770 -
        Flags: review?(dholbert)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8961776 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8961770 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

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

::: layout/generic/nsFloatManager.cpp
@@ +1102,5 @@
> +  }
> +
> +  if (!mIntervals.IsEmpty()) {
> +    mBStart = mIntervals[0].Y();
> +    mBEnd = mIntervals[mIntervals.Length() - 1].YMost();

If you like, you can abbreviate "mIntervals[mIntervals.Length() - 1]" to mIntervals.LastElement().

@@ +1574,5 @@
> +                                             appUnitsPerDevPixel);
> +
> +  // Use empty CSSSizeOrRatio to force set the preferred size as the frame's
> +  // content box size. When drawn it will get scaled to dev pixels.
> +  imageRenderer.SetPreferredSize(CSSSizeOrRatio(), contentRect.Size());

The second sentence here is a bit confusing - this code doesn't really have anything to do with dev pixels now.  (I think that text is from the previous patch, when we were using a csspixel::devpixel scaled quantity here).

So: probably just drop that second sentence.

@@ +1580,5 @@
> +
> +  RefPtr<gfx::DrawTarget> drawTarget =
> +    gfx::Factory::CreateDrawTarget(gfx::BackendType::SKIA,
> +                                   contentSizeInDevPixels.ToUnknownSize(),
> +                                   gfx::SurfaceFormat::A8);

I admit I don't know about the gfx::Factory APIs very much, but I suspect you really want to be using the CreateOffscreenSurface API here (which takes the same args, minus the first one -- it picks the backend type internally for you):
I think it's as simple as
 s/gfx::Factory::CreateDrawTarget(gfx::BackendType::SKIA/
   gfxPlatform::GetPlatform()->CreateOffscreenSurface(/

::: layout/painting/nsImageRenderer.cpp
@@ +951,5 @@
>    return Draw(aPresContext, aRenderingContext, aDirtyRect, destTile,
>                fillRect, destTile.TopLeft(), repeatSize, aSrc);
>  }
> +ImgDrawResult
> +nsImageRenderer::DrawShapeImage(nsPresContext* aPresContext,

Please insert a blank line before this method (after the preceding "}")

@@ +953,5 @@
>  }
> +ImgDrawResult
> +nsImageRenderer::DrawShapeImage(nsPresContext* aPresContext,
> +                                gfxContext& aRenderingContext,
> +                                int32_t aAppUnitsPerDevPixel)

This aAppUnitsPerDevPixel param is new in this version of the patch, but it seems to be unused. Probably get rid of it, unless it's needed later on for some reason?

    
    

    
  
Comment on attachment 8961770 [details] [diff] [review]
Bug 1404222 Part 1: Implement shape-outside: <image>.

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

r=me on the nsImageRenderer.cpp stuff, with Comment 285 and the following addressed. (Note I didn't look at the nsFloatManager changes - I assume dbaron's earlier review covered those, but if you'd like me to look at [new?] stuff there, let me know.)

::: layout/painting/nsImageRenderer.cpp
@@ +973,5 @@
> +      nsLayoutUtils::DrawSingleImage(aRenderingContext, aPresContext,
> +                                     mImageContainer, SamplingFilter::GOOD,
> +                                     dest, dest, Nothing(),
> +                                     imgIContainer::FRAME_FIRST |
> +                                     imgIContainer::FLAG_SYNC_DECODE,

Rather than hardcoding FLAG_SYNC_DECODE here, you should use ConvertImageRendererToDrawFlags(mFlags) to match other codepaths here and to make use of the nsImageRenderer::FLAG_SYNC_DECODE_IMAGES thing that you passed into the constructor here.

So this flags arg should just be:
                       ConvertImageRendererToDrawFlags(mFlags) |
                       imgIContainer::FRAME_FIRST,
(which I think fits there without going over 80 characters -- though if it does go over, just pull it out into a helper var).

        Attachment #8961770 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8961771 [details] [diff] [review]
Bug 1404222 Part 2: Extend ImageLoader to associate flags with each request-frame relationship.

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

r=me on part 2, with nits:

::: layout/style/ImageLoader.h
@@ +90,5 @@
> +      mFlags(0)
> +    {
> +      MOZ_ASSERT(mFrame);
> +    }
> +    const nsIFrame* mFrame;

Per comment 269, please add const *after* the star here, to enforce & document that the pointer-value itself does not change (e.g. we won't accidentally null it out or point it at some other nsIFrame object, even though we do stomp on the other member-var mFlags to add flags)

So this should be e.g. "const nsIFrame* const mFrame;"

@@ +96,5 @@
> +  };
> +
> +  // A helper class to compare FrameWithFlags by comparing mFrame and
> +  // ignoring mFlags.
> +  class FrameOnlyComparitor {

s/ritor/rator/

The "a" spelling is correct, based on DXR searches:
https://dxr.mozilla.org/mozilla-central/search?q=Comparitor  0 results
https://dxr.mozilla.org/mozilla-central/search?q=Comparator  1,774 results

        Attachment #8961771 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8961776 [details]
Bug 1404222 Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant.

https://reviewboard.mozilla.org/r/230620/#review236328

::: commit-message-24e0c:1
(Diff revision 1)
> +Bug 1404222 Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant.
> +
> +This test is designed to check that the float area is calculated from the
> +first frame of an animated GIF. The reference rendering is a solid green
> +square with the lower-left of the square supplied by the pixels in the GIF.
> +Frame 2 of the GIF has white pixels in that quadrant. If layout is completed
> +and onload is fired while frame 2 is showing, the test will fail even though
> +the float area has been correctly calculated. This change puts green in the
> +lower-left quadrant of all frames of the GIF so that the test is only testing
> +the float area and not also testing that onload fires while a certain frame
> +is showing.

Does your change to this test mean we're not testing the "shape-outside" GIF-frame-selection as thoroughly?  (i.e. hypothetically if a browser had a bug where they picked the second frame of the GIF, does your change mean this test will no longer catch that bug?)

Or, perhaps this change doesn't make a difference for shape-outside logic, since you're replacing white pixels with green pixels (IIUC), and those fully-opaque pixels are all equivalent as far as shape-outside is concerned?  (If that's the case: that seems fine, but it would be nice to emphasize this in the commit message -- i.e. "Note that this change doesn't impact the thoroughness of how we're testing shape-outside, since ..."  Particularly since this test comes from another vendor, so we want to be extra sure that we're justifying our changes & making clear that we're not just papering over our own bug.)

    
    

    
  
Comment on attachment 8961774 [details] [diff] [review]
Bug 1404222 Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed or reflow is complete.

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

::: layout/generic/nsFrame.cpp
@@ +1189,5 @@
>      if (oldBorderImage && HasImageRequest()) {
>        imageLoader->DisassociateRequestFromFrame(oldBorderImage, this);
>      }
>      if (newBorderImage) {
> +      imageLoader->AssociateRequestToFrame(newBorderImage, this, 0);

For this "AssociateRequestToFrame" call and the one below it, we figure out the flags on our own ("0" in this case), rather than asking ImageLoaderFlagsForRequest() to tell us which flags to use.

That seems worth briefly explaining with a code comment, since the ImageLoaderFlagsForRequest() documentation seems to suggest that we should be calling that method to give us some flags.

::: layout/style/ImageLoader.cpp
@@ +116,5 @@
> +
> +#ifdef DEBUG
> +  // Do some sanity checking to ensure that we only add to one mapping
> +  // iff we also add to the other mapping.
> +  bool didAddToFrameSet = false;

Don't use the #ifdef wrapper here (and around the assignments).  Instead, just declare these variables as DebugOnly<bool> (and #include mozilla/DebugOnly.h at the top of this file).

See https://dxr.mozilla.org/mozilla-central/source/mfbt/DebugOnly.h for more info.

@@ +130,3 @@
>    }
> +
> +  // Add frame to the request set if it wasn't already there.

s/Add frame/Add request/, I think?

@@ +202,5 @@
> +    if (i > 0 && aFrame == frameSet->ElementAt(i-1).mFrame) {
> +      FrameWithFlags& fwf = frameSet->ElementAt(i-1);
> +      if (fwf.mFlags & REQUEST_HAS_BLOCKED_ONLOAD) {
> +        mDocument->UnblockOnload(false);
> +        fwf.mFlags &= ~REQUEST_HAS_BLOCKED_ONLOAD;

I don't think we need to bother unsetting this flag, do we? We're about to remove this element from the array entirely (the next line of code), so this mFlags-tweak doesn't seem to have any possible impact.

(Maybe you're just trying to make this comparable to the similar chunk of ImageLoader::UnblockOnloadIfNeeded, which *does* need to unset the flag?  If so, I'd suggest just putting a comment here about the flag & element-removal ["No need to unset..."], rather than needlessly toggling the flag.)

@@ +675,5 @@
> +  // pointers only as hashkeys. If these pointers are accidentally
> +  // reused for a matching frame/request pair, the worst that will
> +  // happen is that we'll prematurely unblock onload before a needed
> +  // reflow.
> +  mLoader->UnblockOnloadIfNeeded(mFrame, mRequest);

This worst-case scenario would still be a web-observable correctness bug, right?  It'd probably be edge-casey, but still not great.

Could we change mFrame to be a WeakFrame[1], to avoid this problem altogether?  And then check mFrame.IsAlive() before calling UnblockOnloadIfNeeded(...) on it, here and in ReflowCallbackCanceled().

(WeakFrame has an operator that lets it be directly coerced to a nsIFrame*, too, so you should be able to still pass it directly to UnblockOnloadIfNeeded after checking it for aliveness.)

[1] https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/layout/generic/nsIFrame.h#4675  (documentation is ~100 lines up from there)

And as long as we're doing that, we should probably just go all the way & make mRequest a nsCOMPtr<imgIRequest>, too.  Even though we don't dereference it right now, we might dereference it in the future (on purpose or by accident)... And that change will also prevent the need for worrying/explanatory-comments about "what if the request is deleted and another one is created in its same address before this callback fires" scenarios.  (Right now, I think that sort of scenario would still produce correctness bugs, at least.)


(Sorry for not taking a firmer position on this up in comment 271 -- that was mostly me thinking out loud about "how OK are these raw pointers", and I didn't have a good feel for whether we had any mitigation to avoid the deletion-and-recreation scenario.  I'd feel better about the "only used as hash keys" justification if we had a mitigation/guarantee of some sort to prevent that scenario under sane conditions.)

        Attachment #8961774 -
        Flags: review?(dholbert) → review-

    
    

    
  
Probably best for you (Brad) to revise part 3 per dholbert's comments and then I'll look again, but if you want me to look first, let me know (probably via needinfo?).

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #287)
> Comment on attachment 8961771 [details] [diff] [review]
> Per comment 269, please add const *after* the star here, to enforce &
> document that the pointer-value itself does not change (e.g. we won't
> accidentally null it out or point it at some other nsIFrame object, even
> though we do stomp on the other member-var mFlags to add flags)

I changed this to a WeakFrame as proposed in comment 289, and that seemed to make the const designation unnecessary.

(In reply to Daniel Holbert [:dholbert] from comment #288)
> Comment on attachment 8961776 [details]
> Does your change to this test mean we're not testing the "shape-outside"
> GIF-frame-selection as thoroughly?  (i.e. hypothetically if a browser had a
> bug where they picked the second frame of the GIF, does your change mean
> this test will no longer catch that bug?)

No, for the test to pass, the float area has to be calculated from the first frame of the GIF. The other frames have different alpha pixel patterns and constructing the float area from them would cause the test to fail in a way that has nothing to do with the green pixels in the lower left.

> Or, perhaps this change doesn't make a difference for shape-outside logic,
> since you're replacing white pixels with green pixels (IIUC), and those
> fully-opaque pixels are all equivalent as far as shape-outside is concerned?
> (If that's the case: that seems fine, but it would be nice to emphasize this
> in the commit message -- i.e. "Note that this change doesn't impact the
> thoroughness of how we're testing shape-outside, since ..."  Particularly
> since this test comes from another vendor, so we want to be extra sure that
> we're justifying our changes & making clear that we're not just papering
> over our own bug.)

I added another sentence to the extended comment.

(In reply to Daniel Holbert [:dholbert] from comment #289)
> Comment on attachment 8961774 [details] [diff] [review]
> For this "AssociateRequestToFrame" call and the one below it, we figure out
> the flags on our own ("0" in this case), rather than asking
> ImageLoaderFlagsForRequest() to tell us which flags to use.
> 
> That seems worth briefly explaining with a code comment, since the
> ImageLoaderFlagsForRequest() documentation seems to suggest that we should
> be calling that method to give us some flags.

I've changed all the AssociateRequestToFrame() calls to use the new ImageLoaderFlagsForRequest() method, for consistency and clarity.

> Could we change mFrame to be a WeakFrame[1], to avoid this problem
> altogether?  And then check mFrame.IsAlive() before calling
> UnblockOnloadIfNeeded(...) on it, here and in ReflowCallbackCanceled().
> 
> And as long as we're doing that, we should probably just go all the way &
> make mRequest a nsCOMPtr<imgIRequest>, too.  Even though we don't
> dereference it right now, we might dereference it in the future (on purpose
> or by accident)... And that change will also prevent the need for
> worrying/explanatory-comments about "what if the request is deleted and
> another one is created in its same address before this callback fires"
> scenarios.  (Right now, I think that sort of scenario would still produce
> correctness bugs, at least.)

I made these changes and updated the comments now that the correct/safe behavior is enforced.

    
    

    
  


  

        Attachment #8961770 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #291)
> (In reply to Daniel Holbert [:dholbert] from comment #288)
> > Comment on attachment 8961776 [details]
> > Does your change to this test mean we're not testing the "shape-outside"
> > GIF-frame-selection as thoroughly? [...]
> > Or, perhaps this change doesn't make a difference for shape-outside logic[...]
> 
> I added another sentence to the extended comment.

Thanks.  Looks like that update hasn't made it to MozReview, but that's OK given the state of patch-juggling here. :) I'll take your word for it that the extra comment clarifies things, & I'll mark patch 9 as r+ in mozreview.

    
    

    
  
Comment on attachment 8961776 [details]
Bug 1404222 Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant.

https://reviewboard.mozilla.org/r/230620/#review237332

        Attachment #8961776 -
        Flags: review?(dholbert) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
Now that we're rendering in dev pixels, I changed the sampling filter to SamplingFilter::POINT to keep all rendered pixels with values equal to some pixel in the source. This is necessary for some web-platform tests that specify a shape-image-threshold but rely upon crisply-defined edges for the float area. I added a significant explanation in nsImageRenderer::DrawShapeImage.

        Attachment #8962901 -
        Attachment is obsolete: true

    
    

    
  


  
I introduced a flaw in ImageLoader::RemoveRequestToFrameMapping while implementing the changes requested in Comment 289. The code was dropping the request from the RequestSet without triggering UnblockOnload(), thus the timeouts seen in the try run.

        Attachment #8962903 -
        Attachment is obsolete: true

        Attachment #8962903 -
        Flags: review?(dbaron)

        Attachment #8963348 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8963348 [details] [diff] [review]
Bug 1404222 Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed or reflow is complete.

My previous review of this patch was in comment 265.



Don't create the function nsIFrame::ImageLoaderFlagsForRequest.  It
still does the thing I've asked you not to do.

Instead, pass 0 from AddAndRemoveImageAssociations, from AssociateImage,
and from the existing call in DidSetComputedStyle for border-image, and
pass ImageLoader::REQUEST_REQUIRES_REFLOW for the new call for
shape-image.  This means that if we add new things, the caller will have
to explicitly consider which flags they get rather than getting the
default.  (Though maybe it makes sense to add the parameter to
AssociateImage too, and pass 0 from all the callers explicitly, but
probably not... though maybe it depends how you've handled the existing
bugs I pointed out in my last review and whether you'll need
AssociateImage.)


>+      if(NS_SUCCEEDED(aRequest->GetImageStatus(&status)) &&

space in "if ("


>+  // See if the frameSet already has this frame.
>   uint32_t i = frameSet->IndexOfFirstElementGt(fwf, FrameOnlyComparator());
>+  if (i > 0 && aFrame == frameSet->ElementAt(i-1).mFrame) {
>+    // We're already tracking this frame, so prepare to modify the
>+    // existing FrameWithFlags object.
>+    fwfToModify = &frameSet->ElementAt(i-1);

Please use BinaryIndexOf here and drop the explicit frame check:

>  // See if the frameSet already has this frame.
>  size_t i = frameSet->BinaryIndexOf(fwf, FrameOnlyComparator());
>  if (i != decltype(frameSet)::NoIndex) {
>    // We're already tracking this frame, so prepare to modify the
>    // existing FrameWithFlags object.
>    fwfToModify = &frameSet->ElementAt(i);

and then also drop the explicit frame check here:

>+  // If we weren't already tracking this frame, add it to the frameSet.
>   if (i == 0 || aFrame != frameSet->ElementAt(i-1).mFrame) {
>     frameSet->InsertElementAt(i, fwf);

by changing it to:

>  // If we weren't already tracking this frame, add it to the frameSet.
>  if (i != decltype(frameSet)::NoIndex) {
>    frameSet->InsertElementAt(i + 1, fwf);

And then change:

>   i = requestSet->IndexOfFirstElementGt(aRequest);
>   if (i == 0 || aRequest != requestSet->ElementAt(i-1)) {
>     requestSet->InsertElementAt(i, aRequest);

likewise to:

>   i = requestSet->BinaryIndexOf(aRequest);
>   if (i != decltype(requestSet)::NoIndex)) {
>     requestSet->InsertElementAt(i+1, aRequest);


Please file a followup bug on this bit of comment 265:

> However, this exposes the somewhat deeper problem that the existing code
> is wrong, and doesn't deal correctly with (for example) an image being
> used three times by a frame, and then changing to being used only twice
> by the frame.  (This is actually probably most likely with multiple
> background images or multiple mask images.)  This change makes it worse
> because there's a risk of getting the flags wrong... although there's
> also still the risk of getting the invalidation wrong.  It seems we
> mostly cover this up by calling nsIFrame::AssociateImage whenever we
> paint, I suppose on the theory that some associations might not go
> through DidSetStyleContext.  It's not clear to me if that's actually
> true, or if that code exists (today, at least) only to work around the
> other bug.  Filing a followup bug to fix this mess would probably be
> good.



Please also adjust ImageLoader::UnblockOnloadIfNeeded to use BinaryIndexOf
rather than IndexOfFirsTElementGt, as above, and as I suggested at the end of
comment 265.


r=dbaron with that

        Attachment #8963348 -
        Flags: review?(dbaron) → review+

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (mostly unavailable April 2-20) from comment #308)
> Comment on attachment 8963348 [details] [diff] [review]

> and then also drop the explicit frame check here:
> 
> >+  // If we weren't already tracking this frame, add it to the frameSet.
> >   if (i == 0 || aFrame != frameSet->ElementAt(i-1).mFrame) {
> >     frameSet->InsertElementAt(i, fwf);
> 
> by changing it to:
> 
> >  // If we weren't already tracking this frame, add it to the frameSet.
> >  if (i != decltype(frameSet)::NoIndex) {
> >    frameSet->InsertElementAt(i + 1, fwf);
> 
> And then change:
> 
> >   i = requestSet->IndexOfFirstElementGt(aRequest);
> >   if (i == 0 || aRequest != requestSet->ElementAt(i-1)) {
> >     requestSet->InsertElementAt(i, aRequest);
> 
> likewise to:
> 
> >   i = requestSet->BinaryIndexOf(aRequest);
> >   if (i != decltype(requestSet)::NoIndex)) {
> >     requestSet->InsertElementAt(i+1, aRequest);

Both of these searches are for determining that an element is NOT in the set. IndexOfFirstElementGt is a binary search that has the beneficial side effect of returning the appropriate insertion point if the element is not present. If I change the searches to be BinaryIndexOf, then the insertions would have to become InsertElementSorted. Two binary searches for insertion instead of one, and I believe insertion to be the common case. The only benefit of the change would be avoiding the pointer comparison in the case of the element already being present, which I claim is an uncommon case.

For this reason, I'm not going to make this change. In UnblockOnloadIfNeed, I will change it BinaryIndexOf as suggested, since the search is not used for insertion.

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #309)
> For this reason, I'm not going to make this change. In UnblockOnloadIfNeed,
> I will change it BinaryIndexOf as suggested, since the search is not used
> for insertion.

Oops, sorry, that sounds good.

    
  
Blocks: 1450768

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (mostly unavailable April 2-20) from comment #308)
> Comment on attachment 8963348 [details] [diff] [review] 
> Please file a followup bug on this bit of comment 265:
> 
> > However, this exposes the somewhat deeper problem that the existing code
> > is wrong, and doesn't deal correctly with (for example) an image being
> > used three times by a frame, and then changing to being used only twice
> > by the frame.  (This is actually probably most likely with multiple
> > background images or multiple mask images.)  This change makes it worse
> > because there's a risk of getting the flags wrong... although there's
> > also still the risk of getting the invalidation wrong.  It seems we
> > mostly cover this up by calling nsIFrame::AssociateImage whenever we
> > paint, I suppose on the theory that some associations might not go
> > through DidSetStyleContext.  It's not clear to me if that's actually
> > true, or if that code exists (today, at least) only to work around the
> > other bug.  Filing a followup bug to fix this mess would probably be
> > good.

Bug 1450768 filed to address this.

    
    

    
  
This is ready for checkin. The complicated review history has lost the r+ on these parts:

Part 1: r+ dbaron, dholbert
Part 2: r+ dbaron
Part 3: r+ dbaron, dholbert
Part 7: r+ dholbert
Keywords: checkin-needed

    
    

    
  
I can check this in for you - let's coordinate over IRC.

    
    

    
  
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57eeb849ab88
Part 1: Implement shape-outside: <image>. r=dbaron,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0baffdd79b
Part 2: Extend ImageLoader to associate flags with each request-frame relationship.  r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b9096da915
Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed or reflow is complete. r=dbaron,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/59038f2db68a
Part 4: Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ac4167f702
Part 5: Add web-platform-tests for linear-gradient with writing-modes.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/026c74a92d04
Part 6: Add a crashtest. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0450620fdabd
Part 7: Turn off a 'todo' in a mochitest. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b79d6e8318db
Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e4294c1c59
Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7183b8104399
Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin. r=dholbert

    
    

    
  
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a99136300c
followup: add 'explicit' keyword to fix static analysis build error. (no review, trivial bustagefix)

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #312, following up to comment 308 and comment 265)
> Bug 1450768 filed to address this.

That doesn't look like it describes the problem I was pointing out, which is that the existing code doesn't deal correctly with a single removal that results from the number of places referencing a still-loading image decreasing (or changing), but not to zero.  (And this patch makes it worse since there aren't any AssociateImage calls to cover up the bug.)

    
    

    
  
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98749dde9018
Backed out 11 changesets for static analysis failures on a CLOSED TREE.

    
    

    
  
Backed out 11 changesets (bug 1404222) for static analysis failures on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/98749dde9018b91a50b668cbd2ca729cc5bc7357

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7183b81043999bbcc3d95091d498f3bf930132b2

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=171497168&repo=mozilla-inbound&lineNumber=14058

Log snippet: 
[task 2018-04-02T20:48:25.876Z] 20:48:25     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -target x86_64-apple-darwin11 -B /builds/worker/workspace/build/src/cctools/bin -isysroot /builds/worker/workspace/build/src/MacOSX10.11.sdk -std=gnu++14 -o nsDOMWindowUtils.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.dylib -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -Werror -Wno-error=shadow  -MD -MP -MF .deps/nsDOMWindowUtils.o.pp   /builds/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp
[task 2018-04-02T20:48:25.876Z] 20:48:25     INFO -  In file included from /builds/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp:108:
[task 2018-04-02T20:48:25.876Z] 20:48:25     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/css/ImageLoader.h:115:5: error: bad implicit conversion constructor for 'FrameWithFlags'
[task 2018-04-02T20:48:25.876Z] 20:48:25     INFO -      FrameWithFlags(nsIFrame* aFrame)
[task 2018-04-02T20:48:25.876Z] 20:48:25     INFO -      ^
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/css/ImageLoader.h:115:5: note: consider adding the explicit keyword to the constructor
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  1 error generated.
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1024: recipe for target 'nsDOMWindowUtils.o' failed
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  make[4]: *** [nsDOMWindowUtils.o] Error 1
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2018-04-02T20:48:25.877Z] 20:48:25     INFO -  make[4]: *** Waiting for unfinished jobs....
Flags: needinfo?(bwerth)

    
    

    
  


  

        Attachment #8963347 -
        Attachment is obsolete: true

    
    

    
  


  
This fixes a correctness problem in Part 3 with CORS Mode handling, revealed by the failed landing attempt. When this is approved, I'll roll it back into Part 3 for landing (and update the Part 3 commit message).

        Attachment #8964450 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8964450 [details] [diff] [review]
Bug 1404222 Part 3b: Correct ImageLoader to unblock document onload when image request completes without sending any frames (as in a CORS mode violation).

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

r=me, two nits:

::: layout/style/ImageLoader.cpp
@@ +99,5 @@
>      if ((fwfToModify->mFlags & REQUEST_HAS_BLOCKED_ONLOAD) == 0) {
>        fwfToModify->mFlags |= REQUEST_HAS_BLOCKED_ONLOAD;
>  
>        // Block document onload until we either remove the frame in
> +      // RemoveRequestToFrameMapping or onLoadComplete or complete a reflow.

nit: add a comma after "onLoadComplete" here (to make this parse better, grammar-wise -- since "complete a reflow" isn't something that we can be "in")

@@ +109,5 @@
>        // reflow now, because we'll never get a call to onFrameComplete.
>        uint32_t status = 0;
>        if(NS_SUCCEEDED(aRequest->GetImageStatus(&status)) &&
>           status & imgIRequest::STATUS_FRAME_COMPLETE) {
> +        fwfToModify->mFlags |= REQUEST_HAS_REQUESTED_REFLOW;

(This is just before a call to RequestReflowOnFrame(aFrame, aRequest))

Optional nit: So, it look like now both of the RequestReflowOnFrame callers will be setting this bit.  Perhaps really RequestReflowOnFrame itself should be in charge of setting this bit, so that callers don't have to each remember to do so? (e.g. if we add another caller somewhere else down the line)

(That would mean RequestReflowOnFrame would need to take "FrameWithFlags" as its arg, rather than nsIFrame* -- which is fine, I think.)

        Attachment #8964450 -
        Flags: review?(dholbert) → review+

    
    

    
  
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f21533e6db
Part 1: Implement shape-outside: <image>. r=dbaron,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f645937f6c0
Part 2: Extend ImageLoader to associate flags with each request-frame relationship. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/501cc7513f9d
Part 3: Block onload when shape-outside images are requested for a frame, and keep it blocked until the frame is removed, the image fails to load, or reflow is complete. r=dbaron,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd731b1a6861
Part 4: Replace hash '#' with %23 in SVG data URIs, to fix XML parse errors. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c489650a9730
Part 5: Add web-platform-tests for linear-gradient with writing-modes. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d43b922e06b
Part 6: Add a crashtest. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc754735a6f
Part 7: Turn off a 'todo' in a mochitest. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c17b43d22af
Part 8: Fix wpt reftest shape-image-001.html to correct a too-wide container. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/491be7851cfc
Part 9: Fix wpt reftest shape-image-025.html to make every frame of the animated GIF use a green box in the lower-left quadrant.
https://hg.mozilla.org/integration/mozilla-inbound/rev/917f8c2a17da
Part 10: Mark as PASS all shape-outside image web-platform-tests that don't rely on shape-margin. r=dholbert
Depends on: 1451123

    
    

    
  
This has been a challenging bug and a challenging landing. Claiming victory for now.
Flags: needinfo?(bwerth)
Depends on: 1451196
Whiteboard: [designer-tools] → [designer-tools][wptsync upstream error]

    
    

    
  
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10417 for changes under testing/web-platform/tests
Whiteboard: [designer-tools][wptsync upstream error] → [designer-tools][wptsync upstream]
Whiteboard: [designer-tools][wptsync upstream] → [designer-tools][wptsync upstream error]

    
    

    
  
Hmm, unstable result on /css/css-shapes/shape-outside/shape-image/shape-image-025.html. Passes 9/10 times. This is the test that Part 9 of this patch was designed to fix -- it was already intermittent because the image could be showing white pixels when the onload event fired. How can we confirm if this upstream sync result is also absorbing the change made to the GIF in attachment 8961776 [details]?
Flags: needinfo?(bwerth) → needinfo?(dbaron)

    
    

    
  
(In reply to Brad Werth [:bradwerth] from comment #335)
> Hmm, unstable result on
> /css/css-shapes/shape-outside/shape-image/shape-image-025.html. Passes 9/10
> times. This is the test that Part 9 of this patch was designed to fix -- it
> was already intermittent because the image could be showing white pixels
> when the onload event fired. How can we confirm if this upstream sync result
> is also absorbing the change made to the GIF in attachment 8961776 [details]?

https://travis-ci.org/w3c/web-platform-tests/jobs/365025877

Ahh, the test result is generated when part 4 is merged, BEFORE part 9 is merged. The test will be intermittent until part 9 is merged. Perhaps the solution is to move part 9 to be the first part of the patch?
Whiteboard: [designer-tools][wptsync upstream error] → [designer-tools][wptsync upstream]

    
    

    
  
I don't know ... jgraham would probably know more, or know who to ask.  But given comment 337 I suspect it no longer matters.
Flags: needinfo?(dbaron)

    
    

    
  
The compat data for this is updated: https://developer.mozilla.org/en-US/docs/Web/CSS/shape-outside#Browser_compatibility.

I'm marking this dev-doc-complete, but please comment if we need anything else here.
Keywords: dev-doc-needed → dev-doc-complete
No longer blocks: 1430983

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: