Closed
Bug 1380393
Opened 7 years ago
Closed 7 years ago
Kill off unused offset* bits of nsIDOMHTMLElement
Categories
(Core :: DOM: Core & HTML, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(5 files)
11.44 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
5.92 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
2.92 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
2.86 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Because they're unused.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8885998 -
Flags: review?(continuation)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8885999 -
Flags: review?(continuation)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8886001 -
Flags: review?(continuation)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8886002 -
Flags: review?(continuation)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8886003 -
Flags: review?(continuation)
Comment 6•7 years ago
|
||
Do we need to worry about breaking addons with these IDL changes?
Assignee | ||
Comment 7•7 years ago
|
||
I don't believe we do. All these interfaces are non-scriptable (because script goes through webidl bindings anyway) and we haven't supported binary addons for a while, right?
Comment 8•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #7) > I don't believe we do. All these interfaces are non-scriptable (because > script goes through webidl bindings anyway) and we haven't supported binary > addons for a while, right? Yeah, I'm not concerned about binary addons. So things like this are just dead code that won't work? if (node instanceof Ci.nsIDOMHTMLImageElement && (node.naturalWidth || node.naturalHeight)) Removed in bug 1380413, found here in what looks to be AdblockPlus: https://dxr.mozilla.org/addons/source/addons/1865/lib/child/requestNotifier.js#268 There's also this from another addon: var v_elem = pNode.QueryInterface(Components.interfaces.nsIDOMHTMLElement); [...] return v_elem.innerHTML; which is removed in a patch in this bug. https://dxr.mozilla.org/addons/source/addons/629580/components/idmhelper5.js#365 Anyways, I'm willing to believe you that this doesn't affect addons, I just don't understand how. :)
Assignee | ||
Comment 9•7 years ago
|
||
> if (node instanceof Ci.nsIDOMHTMLImageElement && (node.naturalWidth || node.naturalHeight)) This will work fine. We make the instanceof check work via shim interface info (see xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp the long list in kComponentsInterfaceShimMap) and the .naturalWidth/.naturalHeight gets go through webidl bindings. > var v_elem = pNode.QueryInterface(Components.interfaces.nsIDOMHTMLElement); This will continue to work. Again, Components.interfaces.nsIDOMHTMLElement works via shim interface info, and if pNode is a WebIDL binding for an element then LegacyQueryInterface.webidl sticks a chromeonly QueryInterface method on its proto chain (see Element implements LegacyQueryInterface). This calls dom:QueryInterface over in BindingUtils.cpp, which checks whether "this" QIs to the arg iid and if so just returns "this", otherwise throws. So in cases when you don't expect it to throw it's a silly no-op, but it does the right thing. > return v_elem.innerHTML; Goes via webidl bindings.
Updated•7 years ago
|
Attachment #8885998 -
Flags: review?(continuation) → review+
Updated•7 years ago
|
Attachment #8885999 -
Flags: review?(continuation) → review+
Updated•7 years ago
|
Attachment #8886001 -
Flags: review?(continuation) → review+
Updated•7 years ago
|
Attachment #8886002 -
Flags: review?(continuation) → review+
Updated•7 years ago
|
Attachment #8886003 -
Flags: review?(continuation) → review+
Comment 10•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/18a31ab2a1f0 part 1. Remove some unused nsIDOMHTMLElement bits. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/38221db7dd21 part 2. Remove nsIDOMHTMLElement.tabIndex. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/7825583f045b part 3. Remove nsIDOMHTMLElement.focus. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb51e5f7bd0 part 4. Remove nsIDOMHTMLElement.accessKey. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/358916bfcb06 part 5. Remove nsIDOMHTMLElement.offsetTop/Left. r=mccr8
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/18a31ab2a1f0 https://hg.mozilla.org/mozilla-central/rev/38221db7dd21 https://hg.mozilla.org/mozilla-central/rev/7825583f045b https://hg.mozilla.org/mozilla-central/rev/eeb51e5f7bd0 https://hg.mozilla.org/mozilla-central/rev/358916bfcb06
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•