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

RESOLVED FIXED in mozilla34

Status

()

Core
Layout: Images
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla34
x86
Mac OS X
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8450433 [details] [diff] [review]
(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.
Attachment #8450433 - Flags: review?(ncameron)
(Assignee)

Comment 2

4 years ago
Created attachment 8450439 [details] [diff] [review]
(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. =)
Attachment #8450439 - Flags: review?(ncameron)

Comment 4

4 years ago
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
Attachment #8450433 - Flags: review?(ncameron) → review+

Comment 5

4 years ago
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.
Attachment #8450439 - Flags: review?(ncameron) → review+
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
Created attachment 8459941 [details] [diff] [review]
(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.)
(Assignee)

Updated

4 years ago
Attachment #8450433 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8459943 [details] [diff] [review]
(Part 2) - Make nsImageRenderer::Draw private

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

Updated

4 years ago
Attachment #8450439 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/152a6d71f739
https://hg.mozilla.org/mozilla-central/rev/fd4c9f4cd1f0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.