Closed Bug 504617 Opened 11 years ago Closed 11 years ago

nsSVGImageListener::FrameChanged uses wrong parameter type in method signature (triggers build warning)

Categories

(Core :: SVG, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Unassigned)

References

Details

(Whiteboard: [build_warning])

Attachments

(1 file)

Attached patch fix (trivial)Splinter Review
I hit this warning while building mozilla-central:

nsStubImageDecoderObserver.h:63: warning: ‘virtual nsresult nsStubImageDecoderObserver::FrameChanged(imgIContainer*, gfxIImageFrame*, nsIntRect*)’ was hidden
nsSVGImageFrame.cpp:65: warning:   by ‘virtual nsresult nsSVGImageListener::FrameChanged(imgIContainer*, gfxIImageFrame*, nsRect*)’

The problem is that the superclass uses nsIntRect* as its final parameter, while the subclass uses nsRect*.

I'm attaching a patch to make the subclass use nsIntRect.  (The parameter is ignored, anyway, so it doesn't affect behavior at all.)  I've confirmed that this fixes the build warning.
It looks like bug 339612's patch introduced a number of instances of this problem (the use of nsRect in a method signature, where the supercalss uses nsIntRect). But from a quick check, I think all of the rest of these are already fixed. (mostly by bug 448830 -- e.g. http://hg.mozilla.org/mozilla-central/diff/a7f7ec7f347c/layout/xul/base/src/nsImageBoxFrame.h )
It looks like they were there before bug 339612.  (I suspect back then nsRect and nsIntRect were typedefs for each other so this wasn't considered an overloading.)
Ah, I see.  I was specifically looking at the change from
>   NS_DECL_IMGICONTAINEROBSERVER
to
>  // imgIContainerObserver (override nsStubImageDecoderObserver)
>  NS_IMETHOD FrameChanged(imgIContainer *aContainer, gfxIImageFrame *newframe,
>                          nsRect * dirtyRect);
in a number of places, in bug 339612's patch. (where presumably the macro would expand to use nsIntRect, but the added explicit signature used nsRect).

But now I see that
 (a) in those cases, the method *definitions* were already using nsRect *
 (b) nsIntRect was indeed typedef'd to nsRect at that point -- they weren't separated until bug 448830's patch landed.

So I guess technically, this is more of a regression from bug 448830. :)
Blocks: 448830
Attachment #388952 - Flags: review?(dbaron)
Comment on attachment 388952 [details] [diff] [review]
fix (trivial)

oops, I'd meant to request review on the fix -- doing that now.
It seems like this should have caused a test somewhere to fail, given that this code wasn't getting executed.  Shouldn't we have a test somewhere that depends on this function getting called?

That said, it looks like http://hg.mozilla.org/mozilla-central/rev/b94bc4be53ca made this patch obsolete and fixed the problem.

It might be worth checking for other occurrences of the same problem, but I suspect Joe Drew already did while writing that patch.
Component: Layout → SVG
Flags: in-testsuite?
QA Contact: layout → general
Comment on attachment 388952 [details] [diff] [review]
fix (trivial)

looks fine, but I think it's obsolete (per above), but marking r=dbaron anyway
Attachment #388952 - Flags: review?(dbaron) → review+
Indeed, bug 753 fixed this. Resolving as fixed by that bug.
Assignee: dholbert → nobody
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Depends on: 753
Resolution: --- → FIXED
Whiteboard: [build_warning]
You need to log in before you can comment on or make changes to this bug.