122861 - nsXULWindow::GetContentShellById was hidden by nsWebShellWindow::GetContentShellById

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[timeless]

In file included from /home/timeless/mozilla/xpfe/appshell/src/nsAppShellService.cpp:59:
/home/timeless/mozilla/xpfe/appshell/src/nsXULWindow.h:63: warning: `nsXULWindow::GetContentShellById(const PRUnichar *, nsIDocShellTreeItem **)' was hidden
/home/timeless/mozilla/xpfe/appshell/src/nsWebShellWindow.h:88: warning:   by `nsWebShellWindow::GetContentShellById(const nsString &, nsIWebShell **)'
/home/timeless/mozilla/xpfe/appshell/src/nsAppShellService.cpp: In method `nsresult nsAppShellService::JustUseBrowserSibling(nsIXULWindow *, nsIURI *, int, unsigned int, nsIXULWindow **)':

this is from a 097branch, but the code appears to be the same on trunk.  I'm actually trying to use one of these functions, so it'd be nice if this hiding didn't happen.

Comment 1

[Adam Lock]

It looks like both nsIXULWindow and nsIWebShellWindow have GetContentShellById
methods.

nsIXULWindow defines it like this:

  /* nsIDocShellTreeItem getContentShellById (in wstring ID); */
  NS_IMETHOD GetContentShellById(const PRUnichar *ID, nsIDocShellTreeItem
**_retval) = 0;

And nsIWebShellWindow (which is not generated from IDL) defines it like this:

NS_IMETHOD GetContentShellById(const nsString& anID, nsIWebShell** aResult) = 0;

Even though one is a takes a ref and the other a pointer and both return
different interfaces, they're probably too close for comfort for the compiler. 

I would recommend the nsWebShellWindow::GetContentShellById version and the
method on nsIWebShellWindow be totally removed. Only nsWebShellWindow even calls
 this method and totally ignores the return value anyway so there's no point in
the override.

Comment 2

[Adam Lock]

Attachment: Patch removes nsIWebShellWindow::GetContentShellById

Please try out the patch.

It's straightforward enough and it builds but I'll have to do a full build to
verify it functions correctly. I'll do that first thing tomorrow.

Comment 3

[Adam Lock]

Timeless can you review this patch?

I've built and it appears to work but I have no idea how to test the call to
GetContentShellById in nsWebShellWindow::LoadContentAreas. There should be no
reason for it to fail as old impl did little more than wrap the impl in
nsXULWindow with an extra QI anyway.

nsWebShellWindow is a rat's nest of bad code.

Comment 4

[jag (Peter Annema)]

Comment on attachment 67337 [details] [diff] [review]
Patch removes nsIWebShellWindow::GetContentShellById

sr=jag (r=timeless, it seems)

Comment 5

[Adam Lock]

Fix is in

Comment 6

[Adam Lock]

Reopening.

Changes got backed out because of bug 123572. I have no idea what is causing
this - possibly an nsIDocShellTreeItem not implementing nsIWebNavigation but
since I couldn't even get Mozilla to step into this piece of code...

Comment 7

[Adam Lock]

Marking FIXED again. Changes didn't get backed out (so far)