1034209 - Remove dead code in nsImageRenderer::Draw and replace it with one of the special cases that make it dead

Status: RESOLVED FIXED

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

Description

[Seth Fowler [:seth] [:s2h]]

The eStyleImageType_Image case in nsImageRenderer::Draw is dead code. (This has been verified by adding an unconditional assert in this case and pushing it to try.) The reason it's dead is because every caller has its own special case handling for eStyleImageType_Image.

We should remove the dead code because it's misleading to the reader, who would expect that the code in that case:

- is actually called
- is actually correct

Neither of those things are true. The code in that case doesn't correctly handle tiling because it calls DrawSingleImage. I copied it for the eStyleImageType_Element case in bug 1031576 and got test failures, which is what originally led me to suspect that the eStyleImageType_Image case was dead code.

It makes sense, since we're removing the existing dead code, to replace it with one of the special cases that callers have which cause it to be dead code in the first place.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Replace the dead Image case in nsImageRenderer::Draw with the code used in DrawBackground

This patches implements the change in comment 0. It seemed to me to be most logical to use the DrawBackground case as the new code. This required that nsImageRenderer::Draw take an anchor point parameter. As long as I was at it, I swapped the order of the aDest and aFill parameters to be consistent with other similar functions.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make nsImageRenderer::Draw private

Writing part 1 made it clear that nsImageRenderer::Draw is never called outside of nsImageRenderer itself, and its behavior is a little quirky. (And was even before this patch; note for example that we only actually honor the aSrc argument for the gradient case.)

This makes me feel like nsImageRenderer::Draw is probably better as an implementation detail than a public API. This patch just makes it private, so that external callers are forced to use one of the more specialized Draw methods like DrawBackground.

Nick, if you disagree here, feel free to r- and I won't be sad. =)

Comment 3

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=9c76309c5bfb

Comment 4

[Nick Cameron [:nrc]]

Comment on attachment 8450433 [details] [diff] [review]
(Part 1) - Replace the dead Image case in nsImageRenderer::Draw with the code used in DrawBackground

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

r=me with nits addressed.

::: layout/base/nsCSSRendering.cpp
@@ +4509,5 @@
>  
>        nsCOMPtr<imgIContainer> image(ImageOps::CreateFromDrawable(drawable));
>        nsLayoutUtils::DrawImage(&aRenderingContext, image,
>                                 graphicsFilter, aDest, aFill,
> +                               aAnchor, aDirtyRect, 

whitespace

::: layout/base/nsCSSRendering.h
@@ +175,2 @@
>              const nsRect&        aFill,
> +            const nsPoint&       aAnchor,

Please add an explanation of aAnchor to the comment

Comment 5

[Nick Cameron [:nrc]]

Comment on attachment 8450439 [details] [diff] [review]
(Part 2) - Make nsImageRenderer::Draw private

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

Maybe worth adding that only the gradient case uses aSrc to the comment? I don't mind too much since its private.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Nick! Agreed on all counts.

(In reply to Nick Cameron [:nrc] from comment #5)
> Maybe worth adding that only the gradient case uses aSrc to the comment? I
> don't mind too much since its private.

Heh, I actually had that in the patch originally, but when I made the method private I removed the comment. I agree that it'd be useful to document that somewhere, just to remove one more footgun from this code. I'll add such a comment.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Replace the dead Image case in nsImageRenderer::Draw with the code used in DrawBackground

Final rebase before landing. (Part 2 didn't require any rebasing.)

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make nsImageRenderer::Draw private

Ah, but of course, part 2 did have a review comment that needed addressing. That's now done.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/152a6d71f739
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4c9f4cd1f0

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/152a6d71f739
https://hg.mozilla.org/mozilla-central/rev/fd4c9f4cd1f0