Daniel Holbert [:dholbert] Reporter
	Description • 15 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 15 years ago

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

	Daniel Holbert [:dholbert] Reporter
	Comment 2 • 15 years ago

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 )

	David Baron :dbaron:
	Comment 3 • 15 years ago

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

	Daniel Holbert [:dholbert] Reporter
	Comment 4 • 15 years ago

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

	Daniel Holbert [:dholbert] Reporter
	Comment 5 • 15 years ago

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

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

	David Baron :dbaron:
	Comment 6 • 15 years ago

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.

	David Baron :dbaron:
	Comment 7 • 15 years ago

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

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

	Daniel Holbert [:dholbert] Reporter
	Comment 8 • 15 years ago

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