Closed Bug 801212 Opened 7 years ago Closed 7 years ago

nsWebBrowser fails to override its interface's "PaintWindow()" method, due to subtly different method-signature

Categories

(Core :: User events and focus handling, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dholbert, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 743975 tried to make  nsWebBrowser::PaintWindow() override nsIWidgetListener::PaintWindow(), but the method-signatures are different, so they're different methods.

This triggers this build warning:
{
Warning: -Woverloaded-virtual in /mozilla/../obj/dist/include/nsIWidgetListener.h: ‘virtual bool nsIWidgetListener::PaintWindow(nsIWidget*, nsIntRegion, bool, bool)’ was hidden

../../../dist/include/nsIWidgetListener.h:126:16: warning: ‘virtual bool nsIWidgetListener::PaintWindow(nsIWidget*, nsIntRegion, bool, bool)’ was hidden [-Woverloaded-virtual]

Warning: -Woverloaded-virtual in /mozilla/embedding/browser/webBrowser/nsWebBrowser.h:   by ‘virtual bool nsWebBrowser::PaintWindow(nsIWidget*, bool, nsIntRegion, bool)’

/mozilla/embedding/browser/webBrowser/nsWebBrowser.h:127:18: warning:   by ‘virtual bool nsWebBrowser::PaintWindow(nsIWidget*, bool, nsIntRegion, bool)’ [-Woverloaded-virtual]
}

Note the subtly different method-signatures -- "bool" and "nsIntRegion" are inverted.

This appears to have been broken ever since nsIWidgetListener.h was created, back in bug 743975.

(FWIW, if we'd annotated nsWebBrowser::PaintWindow as MOZ_OVERRIDE, this would have been a build error and we would have caught this.)
Blocks: buildwarning
Depending on how PaintWindow() is invoked, this could mean we're never actually exercising the intended nsWebBrowser::PaintWindow() impl.
Summary: warning: ‘virtual bool nsIWidgetListener::PaintWindow()... was hidden [-Woverloaded-virtual]...by ‘virtual bool nsWebBrowser::PaintWindow() → nsWebBrowser fails to override its interface's "PaintWindow()" method, due to subtly different method-signature
Matt, I noticed you changed the signature in the base class even more just three days after this bug was entered.  Was it in response to this, are you in the middle of fixing this issue?
Flags: needinfo?(matt.woodrow)
Nope, I hadn't seen this bug at all.
Flags: needinfo?(matt.woodrow)
Neil, can you explain?
Flags: needinfo?(enndeakin)
Attached patch patch (obsolete) — Splinter Review
I think it was just intended to override that function. So let's just fix it.
Assignee: nobody → tnikkel
Attachment #692089 - Flags: review?(enndeakin)
Flags: needinfo?(enndeakin)
Comment on attachment 692089 [details] [diff] [review]
patch

>-    virtual bool PaintWindow(nsIWidget* aWidget, bool isRequest, nsIntRegion aRegion, bool aWillSendDidPaint);
>+    virtual bool PaintWindow(nsIWidget* aWidget, nsIntRegion aRegion, uint32_t aFlags);
Use MOZ_OVERRIDE to prevent a future mistake.
(In reply to Masatoshi Kimura [:emk] from comment #6)
> Use MOZ_OVERRIDE to prevent a future mistake.

+1 (per end of comment 0)  :)
Arguably we should be adding that to every widget listener on every function, but I'm not going to do that, so I'll just do it the once here.
Attached patch patchSplinter Review
Attachment #692089 - Attachment is obsolete: true
Attachment #692089 - Flags: review?(enndeakin)
Attachment #692111 - Flags: review?(enndeakin)
Attachment #692111 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/f050eb79064f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.