Open
Bug 1463015
Opened 5 years ago
Updated 4 months ago
Remove mozIDOMWindow/mozIDOMWindowProxy
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
NEW
People
(Reporter: kmag, Assigned: kmag)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(4 files)
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•5 years ago
|
Priority: -- → P2
Comment 5•5 years ago
|
||
Ill add some comments soon but I dont have the time to think through all of that stuff right now.
Comment 6•5 years 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•5 years 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•5 years 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•5 years 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+
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
Updated•4 months ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•