Closed Bug 1209765 Opened 4 years ago Closed 4 years ago

Implement sync decoding support for border-image

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 6 obsolete files)

5.92 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
11.89 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.42 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.80 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.65 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.79 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.41 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
44.89 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
24.19 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.21 KB, patch
Details | Diff | Splinter Review
2.38 KB, patch
Details | Diff | Splinter Review
Remember bug 1128225 and friends? Time to do it again for border-image! As before we need to return a DrawResult from all border painting functions and record it in a nsDisplayItemGenericImageGeometry or other geometry item derived from nsImageGeometryMixin. This allows us to invalidate when sync decoding images only if repainting would actually produce different results.

Sorry about the sheer amount of code here, Tim. =(
(And note that these patches need to be folded together before landing.)
Depends on: 1209725, 1209739, 1209751
This part is verbose but pretty straightforward. All of the border drawing
functions in nsCSSRendering new to take a new enum which allows callers to
request sync decoding of images; we propagate this, ultimately, to
nsImageRenderer. We then need to propagate DrawResult values outward.
Attachment #8667581 - Flags: review?(tnikkel)
This part and subsequent parts update various callers of the nsCSSRendering
border painting code to properly track DrawResults. The approach here is exactly
what we saw in bug 1128225 and the related bugs.
Attachment #8667583 - Flags: review?(tnikkel)
This one is by far the worst. nsTreeBodyFrame does way too much...
Attachment #8667592 - Flags: review?(tnikkel)
Attachment #8667566 - Attachment is obsolete: true
Rebased part 1.
Attachment #8667581 - Attachment is obsolete: true
Attachment #8667581 - Flags: review?(tnikkel)
Attachment #8670914 - Flags: review?(tnikkel)
OK, try looks good. Looks like this patch series is ready to go.
Comment on attachment 8670914 [details] [diff] [review]
(Part 1) - Support sync decoding of border-image in nsCSSRendering.

>@@ -701,17 +718,17 @@ nsCSSRendering::PaintBorderWithStyleBorder(nsPresContext* aPresContext,
>   nsMargin border = aStyleBorder.GetComputedBorder();
>   if (0 == border.left && 0 == border.right &&
>       0 == border.top  && 0 == border.bottom) {
>     // Empty border area
>-    return;
>+    return result;
>   }

Shouldn't this return success? It could return DrawResult::NOT_READY.

> nsImageRenderer::DrawBorderImageComponent(nsPresContext*       aPresContext,
>   if (!IsReady()) {
>     NS_NOTREACHED("Ensure PrepareImage() has returned true before calling me");
>-    return;
>+    return DrawResult::TEMPORARY_ERROR;
>   }

Shouldn't this return PrepareResult()?
Attachment #8670914 - Flags: review?(tnikkel) → review+
Attachment #8667583 - Flags: review?(tnikkel) → review+
Attachment #8667584 - Flags: review?(tnikkel) → review+
Attachment #8667585 - Flags: review?(tnikkel) → review+
Attachment #8667586 - Flags: review?(tnikkel) → review+
Attachment #8667588 - Flags: review?(tnikkel) → review+
Comment on attachment 8667587 [details] [diff] [review]
(Part 6) - Support sync decoding and track draw results when drawing borders in nsColumnSetFrame.

I don't think the border constructed here can ever draw images, can it?

So would we be better off not adding this extra code and just asserting that we can't have an image?

I think the same is true of the border we draw in DisplayAltFeedback for nsImageFrame, at least.
Attachment #8667591 - Flags: review?(tnikkel) → review+
Attachment #8667590 - Flags: review?(tnikkel) → review+
ni for comment 19
Flags: needinfo?(seth)
(In reply to Timothy Nikkel (:tn) from comment #18)
> Shouldn't this return success? It could return DrawResult::NOT_READY.

Yeah, I think you're right that it should return SUCCESS. I'll make that change.

> 
> > nsImageRenderer::DrawBorderImageComponent(nsPresContext*       aPresContext,
> >   if (!IsReady()) {
> >     NS_NOTREACHED("Ensure PrepareImage() has returned true before calling me");
> >-    return;
> >+    return DrawResult::TEMPORARY_ERROR;
> >   }
> 
> Shouldn't this return PrepareResult()?

Unclear; this case indicates a misuse of the API. It should probably return either TEMPORARY_ERROR or BAD_ARGS. I don't think we ever hit this code in practice, though, so I suppose it's kinda moot.
Flags: needinfo?(seth)
(In reply to Timothy Nikkel (:tn) from comment #19)
> Comment on attachment 8667587 [details] [diff] [review]
> (Part 6) - Support sync decoding and track draw results when drawing borders
> in nsColumnSetFrame.
> 
> I don't think the border constructed here can ever draw images, can it?
> 
> So would we be better off not adding this extra code and just asserting that
> we can't have an image?
> 
> I think the same is true of the border we draw in DisplayAltFeedback for
> nsImageFrame, at least.

That's true in both cases. I went in the direction of adding the DrawResult code everywhere mostly because I didn't want to try to be clever; I thought it was easiest to just ensure that we can handle border-image everywhere, and that way if things change such that we do need to support it, the infrastructure is in place. (Also, when someone decides to copy-paste border drawing code, this way it's correct no matter where they copy it from.) I don't feel strongly about it, though; if you'd prefer that we just assert, we can go that route.
(In reply to Seth Fowler [:seth] [:s2h] from comment #22)
> That's true in both cases. I went in the direction of adding the DrawResult
> code everywhere mostly because I didn't want to try to be clever; I thought
> it was easiest to just ensure that we can handle border-image everywhere,
> and that way if things change such that we do need to support it, the
> infrastructure is in place. (Also, when someone decides to copy-paste border
> drawing code, this way it's correct no matter where they copy it from.) I
> don't feel strongly about it, though; if you'd prefer that we just assert,
> we can go that route.

I think I prefer the asserts, we have enough code in the tree already. :) If someone copy-pastes the code that doesn't support images in borders wrongly then it will assert and they will be forced to copy-paste the correct code. Similarly if columnset changes to add border images the asserts will fire, and they can copy paste the code from one of the other places.
Attachment #8667587 - Flags: review?(tnikkel)
Attachment #8667589 - Flags: review?(tnikkel) → review+
Attachment #8667592 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #18)
> Comment on attachment 8670914 [details] [diff] [review]
> (Part 1) - Support sync decoding of border-image in nsCSSRendering.
> 
> >@@ -701,17 +718,17 @@ nsCSSRendering::PaintBorderWithStyleBorder(nsPresContext* aPresContext,
> >   nsMargin border = aStyleBorder.GetComputedBorder();
> >   if (0 == border.left && 0 == border.right &&
> >       0 == border.top  && 0 == border.bottom) {
> >     // Empty border area
> >-    return;
> >+    return result;
> >   }
> 
> Shouldn't this return success? It could return DrawResult::NOT_READY.

Actually I changed my mind; I think the current behavior of the code is safer, and I prefer not to change it. Basically if we have a border-image and we're just running the non-border-image code because the image hasn't loaded yet, I would prefer that we return DrawResult::NOT_READY until the border-image loads and we take the border-image code path. That way when a border-image is involved, only the border-image code can "sign off" on a sync decode paint succeeding. Anything else is begging for subtle bugs, I think, since this is an edge case we are (realistically) likely to forget about at some point when maintaining this code.
This new version of part 6 just asserts that we aren't drawing a border-image,
and explicitly ignores any DrawResult from border drawing.
Attachment #8677769 - Flags: review?(tnikkel)
Attachment #8667587 - Attachment is obsolete: true
This updated version of part 7 makes us just assert that we're not drawing a
border-image, and explicitly ignore any DrawResult, when drawing the alt
feedback border.
Attachment #8667588 - Attachment is obsolete: true
Attachment #8677769 - Flags: review?(tnikkel) → review+
Attachment #8677769 - Attachment is obsolete: true
Ditto. Didn't notice this on OS X since the filesystem is case-insensitive.
Attachment #8677770 - Attachment is obsolete: true
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.