Closed Bug 1029866 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, defect)

29 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

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
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 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.
https://hg.mozilla.org/mozilla-central/rev/85760b5abe33
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.