Last Comment Bug 659177 - nsAboutProtocolHandler::StartClone needs to change signature to continue overriding inherited method
: nsAboutProtocolHandler::StartClone needs to change signature to continue over...
[build_warning] bz nominated at comme...
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Daniel Holbert [:dholbert]
: Patrick McManus [:mcmanus]
Depends on:
Blocks: 308590
  Show dependency treegraph
Reported: 2011-05-23 15:57 PDT by Daniel Holbert [:dholbert]
Modified: 2013-12-27 14:33 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (3.08 KB, patch)
2011-05-23 16:06 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review
hg changeset for checkin (3.20 KB, patch)
2011-05-24 00:03 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-05-23 15:57:54 PDT
Bug 308590 changed nsSimpleURI::StartClone and its subclass's version nsSimpleNestedURI::StartClone to take one argument.

I missed that nsSimpleNestedURI *itself* had a subclass with a version that needed changing, too.  As a result, we currently get this build warning:
  nsSimpleNestedURI.h:80:26: warning: ‘virtual nsSimpleURI* nsSimpleNestedURI::StartClone(nsSimpleURI::RefHandlingEnum)’ was hidden
  nsAboutProtocolHandler.h:97:26: warning:   by ‘virtual nsSimpleURI* nsNestedAboutURI::StartClone()’

nsNestedAboutURI::StartClone needs to change to take an argument in order to continue overriding properly.

(I verified that this is the only instance of this type of bug by looking for "::Clone" and "::StartClone" in a build log)
Comment 1 Daniel Holbert [:dholbert] 2011-05-23 16:06:22 PDT
Created attachment 534605 [details] [diff] [review]

Ultimately, it'd be nice for nsSimpleNestedURI::StartClone to replace its "new nsSimpleNestedURI()" call with some virtual function that nsNestedAboutURI can override (returning a "new nsNestedAboutURI" instead).

Then, we'll be able to get rid of nsNestedAboutURI's StartClone specialization.

However, for now, this is a simpler change.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-05-23 19:25:55 PDT
Comment on attachment 534605 [details] [diff] [review]

Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-05-23 19:26:52 PDT
This needs to land in Fx6.
Comment 4 Daniel Holbert [:dholbert] 2011-05-24 00:03:32 PDT
Created attachment 534688 [details] [diff] [review]
hg changeset for checkin
Comment 5 Dão Gottwald [:dao] 2011-05-24 03:08:16 PDT
Comment 6 Johnathan Nightingale [:johnath] 2011-05-31 14:39:08 PDT
I agree this needs to land, but doesn't really need to live on the tracking radar - a=me for aurora landing.
Comment 7 Daniel Holbert [:dholbert] 2011-05-31 14:54:06 PDT
Comment on attachment 534605 [details] [diff] [review]

Requesting approval to land on aurora, per comment 3.

Pretty trivial, low-risk fix - as described in comment 0, this just adds an argument to a method on nsNestedAboutURI, mirroring bug 308590's changes to the same method in nsNestedAboutURI's parent class.  This is necessary for the method to override correctly.
Comment 8 Daniel Holbert [:dholbert] 2011-05-31 14:55:19 PDT
Comment on attachment 534605 [details] [diff] [review]

oops, johnath beat me to the punch, a+'ing the newer revision of the patch. :) Canceling duplicate approval request.
Comment 9 Daniel Holbert [:dholbert] 2011-06-01 11:23:28 PDT
Comment on attachment 534688 [details] [diff] [review]
hg changeset for checkin

gah, sorry, this is already in Aurora -- it landed on m-c a few hours before the branch point.

Canceling approval+, for clarity.

Note You need to log in before you can comment on or make changes to this bug.