Remove mozIDOMWindow/mozIDOMWindowProxy

NEW
Assigned to

Status

()

enhancement
P2
normal
Last year
3 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Assignee

Description

Last year
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 hidden (mozreview-request)

Updated

Last year
Depends on: 1463018

Updated

Last year
Priority: -- → P2
Ill add some comments soon but I dont have the time to think through all of that stuff right now.

Comment 6

11 months ago
mozreview-review
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.
Attachment #8979129 - Flags: review?(nika) → review-

Comment 7

11 months ago
mozreview-review
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 :-).
Attachment #8979130 - Flags: review?(nika) → review+

Comment 8

11 months ago
mozreview-review
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;
Attachment #8979131 - Flags: review?(nika) → review+

Comment 9

11 months ago
mozreview-review
Comment on attachment 8979132 [details]
Bug 1463015: Part 4 - Remove mozIDOMWindow/mozIDOMWindowProxy interfaces.

https://reviewboard.mozilla.org/r/245368/#review264278
Attachment #8979132 - Flags: review?(nika) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.