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

RESOLVED FIXED in mozilla33

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

29 Branch
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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
Assignee: nobody → bugs
Created attachment 8445532 [details] [diff] [review]
autojsapi.init.diff

Compiles locally
https://tbpl.mozilla.org/?tree=Try&rev=93cb28bc31ac
Attachment #8445532 - Flags: review?(bobbyholley)
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.
Attachment #8445532 - Flags: review?(bobbyholley) → review?(bobowencode)

Comment 3

4 years ago
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.
Attachment #8445532 - Flags: review?(bobowencode) → review+
I'll update the comments, and can fix the Legacy case too.
Created attachment 8445790 [details] [diff] [review]
InitWithLegacyErrorReportingUsingWin renamed
https://hg.mozilla.org/mozilla-central/rev/85760b5abe33
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.