Closed Bug 1380393 Opened 3 years ago Closed 3 years ago

Kill off unused offset* bits of nsIDOMHTMLElement

Categories

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

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(5 files)

Because they're unused.
Do we need to worry about breaking addons with these IDL changes?
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?
(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. :)
>  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.
Attachment #8885998 - Flags: review?(continuation) → review+
Attachment #8885999 - Flags: review?(continuation) → review+
Attachment #8886001 - Flags: review?(continuation) → review+
Attachment #8886002 - Flags: review?(continuation) → review+
Attachment #8886003 - Flags: review?(continuation) → review+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.