1029866 - Rename InitUsingWin(nsPIDOMWindow* ...) to Init(nsPIDOMWindow*..)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I don't see why the a bit long and odd sounding method name InitUsingWin couldn't just
be Init.
For the case like nsGlobalWindow, couldn't we have some specialized template or
would even having nsGlobalWindow* param work

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: autojsapi.init.diff

Compiles locally
https://tbpl.mozilla.org/?tree=Try&rev=93cb28bc31ac

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8445532 [details] [diff] [review]
autojsapi.init.diff

Review of attachment 8445532 [details] [diff] [review]:
-----------------------------------------------------------------

I'll let Bob take a look since he was the one who investigated this.

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 8445532 [details] [diff] [review]
autojsapi.init.diff

Review of attachment 8445532 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tidying this up.

I did think about using extra casting to resolve the ambiguity, but I thought it would look worse than the different function name.
I didn't think about doing it this way though.
* bobowen kicks himself

My only concern is that we're adding knowledge about the structure of other classes to this one (although I'd already done that).
In other object oriented languages I'd probably have added an interface that you would have to implement, if you wanted to initialise an AutoJSAPI with that class.
That way the logic sits with that class and AutoJSAPI only needs to know about the interface.
However C++ doesn't seem to lend itself to this type of thing (this could just be my lack of C++ knowledge again :-) ).

Anyway, this is only a bit of casting, so I don't think it is much of a problem, as long as we don't start adding lots of other overloads.
It's certainly better that the knowledge is here and not in all the consumers of AutoJSAPI.

::: content/base/src/nsDOMDataChannel.cpp
@@ +2,5 @@
>  /* vim: set sw=2 ts=8 et tw=80 : */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +              

nit: stowaway trailing spaces

::: dom/base/ScriptSettings.h
@@ +155,5 @@
>    // false and use of cx() will cause an assertion.
>    bool InitWithLegacyErrorReporting(nsIGlobalObject* aGlobalObject);
>  
>    // Convenience functions to take an nsPIDOMWindow*, when it is more easily
>    // available than an nsIGlobalObject.

Should this comment be updated now?

@@ +166,1 @@
>    bool InitWithLegacyErrorReportingUsingWin(nsPIDOMWindow* aWindow);

Probably makes sense to do the same for this one.
I can pick this up if you like.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I'll update the comments, and can fix the Legacy case too.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: InitWithLegacyErrorReportingUsingWin renamed

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85760b5abe33

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/85760b5abe33