1463015 - Remove mozIDOMWindow/mozIDOMWindowProxy

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Kris Maglione [:kmag]]

We have a dizzying array of interfaces and concrete classes to refer to inner outer windows. These two are particularly troublesome, given that they're empty interfaces that need to be cast to something useful every time we use them.

Let's update our XPIDL interface to use WebIDL Window/WindowProxy and get rid of these interfaces.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1463015: Part 1 - Allow using WebIDL Window/WindowProxy types in XPIDL.

WindowProxy currently can't be used in XPIDL at all. It's a non-concrete class
which maps to a native type with an abstract interface that WebIDL can convert
to and from but XPIDL can't.

Window can be used, to some extent, but it requires using the
nsGlobalWindowInner type rather than nsPIDOMWindowInner, which maps better to
existing code.

This solution is not ideal, since it adds special cases for Window interfaces.
The only alternative I can think of, though, would require writing a lot of
complex type-detection templates like we use for WebIDL, which would be harder
to maintain and less efficient. Given that I don't expect us to have many
other special cases like this in the future, I don't think that's worth the
effort and maintenance burden.

Review commit: https://reviewboard.mozilla.org/r/245362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245362/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1463015: Part 2 - Convert existing mozIDOMWindow(|Outer) users to nsPIDOMWindow(Inner|Outer).

Review commit: https://reviewboard.mozilla.org/r/245364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245364/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1463015: Part 3 - Remove no-op nsPIDOMWindow(Outer|Inner)::From calls.

Review commit: https://reviewboard.mozilla.org/r/245366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245366/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1463015: Part 4 - Remove mozIDOMWindow/mozIDOMWindowProxy interfaces.

Review commit: https://reviewboard.mozilla.org/r/245368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245368/

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Ill add some comments soon but I dont have the time to think through all of that stuff right now.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979129 [details]
Bug 1463015: Part 1 - Allow using WebIDL Window/WindowProxy types in XPIDL.

https://reviewboard.mozilla.org/r/245362/#review264150

::: dom/bindings/Bindings.conf:1317
(Diff revision 1)
>      'concrete': False,
>  },
>  
>  'Window': {
>      'nativeType': 'nsGlobalWindowInner',
> +    'xpidlNativeType': 'nsPIDOMWindowInner',

I would rather keep consistency with WebIDL, so lets continue to use nsGlobalWindowInner here

::: dom/bindings/Bindings.conf:1331
(Diff revision 1)
>  },
>  
>  'WindowProxy': {
>      'nativeType': 'nsPIDOMWindowOuter',
>      'headerFile': 'nsPIDOMWindow.h',
> +    'xpidlName': 'Window',

I think that it is fine for WindowProxy to be a special case, but I would rather do it elsewhere. this extra flag here requires a lot of code to process, and will never be used by others...

::: xpcom/idl-parser/xpidl/jsonxpt.py:86
(Diff revision 1)
>  
>      if isinstance(type, xpidl.WebIDL):
>          return {
>              'tag': 'TD_DOMOBJECT',
>              'name': type.name,
> +            'xpidlName': type.xpidlName,

I'd like to get rid of all of the idl-parser stuff.

::: xpcom/reflect/xptinfo/xptcodegen.py:227
(Diff revision 1)
>          idx = domobject_cache.get(do['name'])
>          if idx is None:
>              idx = domobject_cache[do['name']] = len(domobjects)
>  
>              includes.add(do['headerFile'])
>              domobjects.append(nsXPTDOMObjectInfo(

I feel like the cleanest option here is actually to check for WindowProxy here. we would define UnwrapWindowProxy, and use it for mUnwrap instead of  UnwrapDOMObject here if do[name] is "WindowProxy"

::: xpcom/reflect/xptinfo/xptcodegen.py:230
(Diff revision 1)
>  
>              includes.add(do['headerFile'])
>              domobjects.append(nsXPTDOMObjectInfo(
>                  "%d = %s" % (idx, do['name']),
>                  # These methods are defined at the top of the generated file.
>                  mUnwrap="UnwrapDOMObject<mozilla::dom::prototypes::id::%s, %s>" %

We should move this into a separate local variable which we set differently for WindowProxy.

::: xpcom/reflect/xptinfo/xptcodegen.py:509
(Diff revision 1)
> +template<>
> +bool WrapDOMObject<nsPIDOMWindowOuter>(JSContext* aCx, void* aObj, JS::MutableHandleValue aHandle)
> +{
> +  auto window = reinterpret_cast<nsPIDOMWindowOuter*>(aObj);
> +  return mozilla::dom::GetOrCreateDOMReflector(aCx, static_cast<nsGlobalWindowOuter*>(window), aHandle);
> +}

this specialization may have to continue to exist.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979130 [details]
Bug 1463015: Part 2 - Convert existing mozIDOMWindow(|Outer) users to nsPIDOMWindow(Inner|Outer).

https://reviewboard.mozilla.org/r/245364/#review264274

::: commit-message-94d22:1
(Diff revision 1)
> +Bug 1463015: Part 2 - Convert existing mozIDOMWindow(|Outer) users to nsPIDOMWindow(Inner|Outer). r?nika

It's mozIDOMWindowProxy not mozIDOMWindowOuter :-P

::: dom/audiochannel/AudioChannelAgent.cpp:60
(Diff revision 1)
>      NotifyStoppedPlaying();
>    }
>  }
>  
>  NS_IMETHODIMP
> -AudioChannelAgent::Init(mozIDOMWindow* aWindow,
> +AudioChannelAgent::Init(nsPIDOMWindowInner* aWindow,

As I said in the previous patch, I'd like us to keep consistent with WebIDL & use nsGlobalWindowInner.

::: dom/audiochannel/AudioChannelAgent.cpp:63
(Diff revision 1)
>  
>  NS_IMETHODIMP
> -AudioChannelAgent::Init(mozIDOMWindow* aWindow,
> +AudioChannelAgent::Init(nsPIDOMWindowInner* aWindow,
>                          nsIAudioChannelAgentCallback *aCallback)
>  {
>    return InitInternal(nsPIDOMWindowInner::From(aWindow),

I don't think we need this 'From' call anymore :-).

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979131 [details]
Bug 1463015: Part 3 - Remove no-op nsPIDOMWindow(Outer|Inner)::From calls.

https://reviewboard.mozilla.org/r/245366/#review264276

Great! You ended up getting rid of them :-)

::: accessible/base/DocManager.cpp:245
(Diff revision 1)
>    nsCOMPtr<nsPIDOMWindowOuter> DOMWindow;
>    aWebProgress->GetDOMWindow(getter_AddRefs(DOMWindow));
>    NS_ENSURE_STATE(DOMWindow);
>  
> -  nsPIDOMWindowOuter* piWindow = nsPIDOMWindowOuter::From(DOMWindow);
> +  nsCOMPtr<nsIDocument> document = DOMWindow->GetDoc();

Can we stop calling this variable DOMWindow & switch to 'window' or similar?

::: dom/base/ThirdPartyUtil.cpp:145
(Diff revision 1)
>        *aResult = true;
>        return NS_OK;
>      }
>    }
>  
> -  nsCOMPtr<nsPIDOMWindowOuter> current = nsPIDOMWindowOuter::From(aWindow), parent;
> +  nsCOMPtr<nsPIDOMWindowOuter> current = aWindow, parent;

I'm confused by the use of the ',' operator here?

::: dom/base/nsFocusManager.cpp:491
(Diff revision 1)
>  
>  NS_IMETHODIMP
>  nsFocusManager::GetLastFocusMethod(nsPIDOMWindowOuter* aWindow, uint32_t* aLastFocusMethod)
>  {
>    // the focus method is stored on the inner window
> -  nsCOMPtr<nsPIDOMWindowOuter> window;
> +  nsCOMPtr<nsPIDOMWindowOuter> window(aWindow);

nit: window = aWindow;

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979132 [details]
Bug 1463015: Part 4 - Remove mozIDOMWindow/mozIDOMWindowProxy interfaces.

https://reviewboard.mozilla.org/r/245368/#review264278