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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: fix (trivial)

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.

Comment 1

[Daniel Holbert [:dholbert]]

Some links to source...

Here's the faulty method signature, in the declaration & definition:
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGImageFrame.cpp?mark=65#63
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGImageFrame.cpp?mark=388#386

And here's the direct superclass's implementation, with correct signature:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsStubImageDecoderObserver.cpp?mark=106#103

And here's the top-level method declaration, with correct signature:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/public/imgIContainerObserver.idl?mark=62#60

Comment 2

[Daniel Holbert [:dholbert]]

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 )

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 4

[Daniel Holbert [:dholbert]]

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. :)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 388952 [details] [diff] [review]
fix (trivial)

oops, I'd meant to request review on the fix -- doing that now.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 388952 [details] [diff] [review]
fix (trivial)

looks fine, but I think it's obsolete (per above), but marking r=dbaron anyway

Comment 8

[Daniel Holbert [:dholbert]]

Indeed, bug 753 fixed this. Resolving as fixed by that bug.