Open
Bug 893117
Opened 11 years ago
Updated 2 years ago
Remove xpidl for HTML elements
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
NEW
People
(Reporter: dzbarsky, Unassigned)
References
Details
(Keywords: addon-compat, Whiteboard: [leave open])
Attachments
(25 files, 2 obsolete files)
11.46 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
12.81 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
13.35 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
9.31 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
10.63 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
5.70 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
14.25 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
4.62 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.67 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.81 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.84 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.40 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.79 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.36 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
12.92 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.71 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.96 KB,
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
8.78 KB,
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
5.90 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
12.78 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
35.40 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.50 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #774805 -
Flags: review?(peterv)
Reporter | ||
Comment 2•11 years ago
|
||
We need the interface because NoScript uses it to test for instanceof.
Attachment #774809 -
Flags: review?(peterv)
Reporter | ||
Comment 3•11 years ago
|
||
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #774949 -
Flags: review?(peterv)
Reporter | ||
Comment 5•11 years ago
|
||
Attachment #774950 -
Flags: review?(peterv)
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #774952 -
Flags: review?(peterv)
Reporter | ||
Comment 7•11 years ago
|
||
Attachment #774955 -
Flags: review?(peterv)
Reporter | ||
Comment 8•11 years ago
|
||
Attachment #775381 -
Flags: review?(peterv)
Reporter | ||
Comment 9•11 years ago
|
||
Attachment #775446 -
Flags: review?(peterv)
Reporter | ||
Comment 10•11 years ago
|
||
Attachment #775447 -
Flags: review?(peterv)
Reporter | ||
Comment 11•11 years ago
|
||
Attachment #775758 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 12•11 years ago
|
||
Attachment #776786 -
Flags: review?(peterv)
Reporter | ||
Comment 13•11 years ago
|
||
Attachment #776789 -
Flags: review?(peterv)
Reporter | ||
Comment 14•11 years ago
|
||
Attachment #776790 -
Flags: review?(peterv)
Comment 15•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #2) > Created attachment 774809 [details] [diff] [review] > HTMLTableElement > > We need the interface because NoScript uses it to test for instanceof. Please contact the NoScript author. NoScript is very actively maintained. If you leave the interface anyway, please bump the iid.
Updated•11 years ago
|
Keywords: addon-compat
Comment 16•11 years ago
|
||
Comment on attachment 775758 [details] [diff] [review] Clean up nsFormControlFrame Review of attachment 775758 [details] [diff] [review]: ----------------------------------------------------------------- There's no nsFormControlFrame here... As I mentioned on IRC, though, I've got a patch that does a lot of this; do you mind if we drop your patch for now?
Attachment #775758 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 17•11 years ago
|
||
(In reply to :Ms2ger from comment #16) > Comment on attachment 775758 [details] [diff] [review] > Clean up nsFormControlFrame > > Review of attachment 775758 [details] [diff] [review]: > ----------------------------------------------------------------- > > There's no nsFormControlFrame here... As I mentioned on IRC, though, I've > got a patch that does a lot of this; do you mind if we drop your patch for > now? Oops, I meant ListControlFrame. Sure. I can rebase this after you land.
Comment 18•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #15) > (In reply to David Zbarsky (:dzbarsky) from comment #2) > > Created attachment 774809 [details] [diff] [review] > > HTMLTableElement > > > > We need the interface because NoScript uses it to test for instanceof. > > Please contact the NoScript author. NoScript is very actively maintained. > If you leave the interface anyway, please bump the iid. So, let me understand: are all the nsIDOMHTML* interfaces going away? NoScript performs quite a lot of instanceof checks against them from a JavaScript-implemented XPCOM service (with generally no access to DOM window globals). Is there any safe replacement?
Comment 19•11 years ago
|
||
Also how can binary addons access HTML elements without nsIDOMHTML* interfaces?
Updated•11 years ago
|
Attachment #774805 -
Flags: review?(peterv) → review+
Comment 20•11 years ago
|
||
Comment on attachment 774805 [details] [diff] [review] HTMLTableCellElement Review of attachment 774805 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLTableCellElement.h @@ +13,5 @@ > > class HTMLTableElement; > > class HTMLTableCellElement MOZ_FINAL : public nsGenericHTMLElement, > + public nsIDOMHTMLElement Although given that we're not able to remove all nsIDOMHTML*Element interfaces it might be a good idea to have a base class that inherits from nsGenericHTMLElement and implements nsIDOMHTMLElement that these could inherit from.
Updated•11 years ago
|
Attachment #774809 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #774948 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #774949 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #774950 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #774952 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #774955 -
Flags: review?(peterv) → review+
Updated•11 years ago
|
Attachment #775381 -
Flags: review?(peterv) → review+
Reporter | ||
Comment 21•11 years ago
|
||
(In reply to Giorgio Maone from comment #18) > (In reply to Masatoshi Kimura [:emk] from comment #15) > > (In reply to David Zbarsky (:dzbarsky) from comment #2) > > > Created attachment 774809 [details] [diff] [review] > > > HTMLTableElement > > > > > > We need the interface because NoScript uses it to test for instanceof. > > > > Please contact the NoScript author. NoScript is very actively maintained. > > If you leave the interface anyway, please bump the iid. > > So, let me understand: are all the nsIDOMHTML* interfaces going away? > NoScript performs quite a lot of instanceof checks against them from a > JavaScript-implemented XPCOM service (with generally no access to DOM window > globals). Is there any safe replacement? I'll only get rid of the ones that aren't being used.(In reply to Masatoshi Kimura [:emk] from comment #19) > Also how can binary addons access HTML elements without nsIDOMHTML* > interfaces? I think we can codegen some wrapper classes that expose all the functions as virtual so binary addons can call them.(In reply to Peter Van der Beken [:peterv] - gone till Aug 10th 2013 from comment #20) > Comment on attachment 774805 [details] [diff] [review] > HTMLTableCellElement > > Although given that we're not able to remove all nsIDOMHTML*Element > interfaces it might be a good idea to have a base class that inherits from > nsGenericHTMLElement and implements nsIDOMHTMLElement that these could > inherit from. Good idea. I'll do that in a followup.
Reporter | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfdebcc8ce6 https://hg.mozilla.org/integration/mozilla-inbound/rev/6515e55d4fb9 https://hg.mozilla.org/integration/mozilla-inbound/rev/05ae078ca073 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1675c08598 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea5fd72aa59 https://hg.mozilla.org/integration/mozilla-inbound/rev/12a2fa9940f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b779d67a022
Reporter | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3772e15f1b45
Comment 24•11 years ago
|
||
Backed out for breaking mochitests: https://tbpl.mozilla.org/php/getParsedLog.php?id=25468172&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/82387fac7a8e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb31f6d254f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3de2ae192d9c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2d343242d7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e008e4a9f54a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1770236344ad remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c1f540425c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/470f3ad550f1
Comment 25•11 years ago
|
||
(In reply to Giorgio Maone from comment #18) > So, let me understand: are all the nsIDOMHTML* interfaces going away? > NoScript performs quite a lot of instanceof checks against them from a > JavaScript-implemented XPCOM service (with generally no access to DOM window > globals). Is there any safe replacement? Namespace and tag check should be just as reliable. (In reply to Masatoshi Kimura [:emk] from comment #19) > Also how can binary addons access HTML elements without nsIDOMHTML* > interfaces? Most of it is syntatic sugar and should be available through nsIDOMHTMLElement/Element/Node. We could do the codegen idea that David mentioned, but in general we'd like to stop supporting things that only binary addons use and that can be replaced by something equivalent.
Comment 26•11 years ago
|
||
David, given comment 24 are you still looking for review on those remaining 5 patches?
Flags: needinfo?(dzbarsky)
Reporter | ||
Comment 27•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #26) > David, given comment 24 are you still looking for review on those remaining > 5 patches? The tests it failed were broken. I just haven't had a chance to fix yet. I think we should land the rest of these patches.
Flags: needinfo?(dzbarsky)
Comment 28•11 years ago
|
||
Comment on attachment 775446 [details] [diff] [review] HTMLDataListElement r=me
Attachment #775446 -
Flags: review?(peterv) → review+
Comment 29•11 years ago
|
||
Comment on attachment 775447 [details] [diff] [review] HTMLOutputElement >+ GetAttr(kNameSpaceID_None, nsGkAtoms::name, aValue); GetHTMLAttr(nsGkAtoms::name, aValue). And you could probably inline this... > // nsIConstraintValidation::SetCustomValidity() is fine. It's clearly not, though, since you had to override it! Please remove that comment. Might be worth making sure we have tests that fail if the override is removed, too. r=me with the above
Attachment #775447 -
Flags: review?(peterv) → review+
Comment 30•11 years ago
|
||
Comment on attachment 776786 [details] [diff] [review] HTMLDirectoryElement r=me but add a comment in the IDL that says it's only there so it'll show up in Components.interfaces?
Attachment #776786 -
Flags: review?(peterv) → review+
Comment 31•11 years ago
|
||
Comment on attachment 776789 [details] [diff] [review] HTMLModElement >+ GetAttr(kNameSpaceID_None, nsGkAtoms::datetime, aDateTime); GetHTMLAttr, please. r=me
Attachment #776789 -
Flags: review?(peterv) → review+
Comment 32•11 years ago
|
||
Comment on attachment 776790 [details] [diff] [review] HTMLHeadingElement >+ GetAttr(kNameSpaceID_None, nsGkAtoms::align, aAlign); GetHTMLAttr. r=me
Attachment #776790 -
Flags: review?(peterv) → review+
Reporter | ||
Comment 33•11 years ago
|
||
Attachment #782934 -
Flags: review?(bzbarsky)
Comment 34•11 years ago
|
||
Comment on attachment 782934 [details] [diff] [review] Remove nsIDOMHTMLLegendElement r=bz The QI bits are a bit weird. Can we not just use an interface map throughout if that's what we want, since now there is no interface table involved at all? r=me with that fixed or a followup filed for better macros if we need them.
Attachment #782934 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 35•11 years ago
|
||
Attachment #782985 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 36•11 years ago
|
||
Attachment #782987 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 37•11 years ago
|
||
Attachment #782988 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 38•11 years ago
|
||
Attachment #782989 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 39•11 years ago
|
||
Attachment #782990 -
Flags: review?(bzbarsky)
Comment 40•11 years ago
|
||
Comment on attachment 782985 [details] [diff] [review] Remove nsIDOMHTMLFontElement r=bz r=me, again modulo the QI weirdness.
Attachment #782985 -
Flags: review?(bzbarsky) → review+
Comment 41•11 years ago
|
||
Comment on attachment 782987 [details] [diff] [review] Remove nsIDOMHTMLLabelElement r=bz r=me with the QI caveat.
Attachment #782987 -
Flags: review?(bzbarsky) → review+
Comment 42•11 years ago
|
||
Comment on attachment 782988 [details] [diff] [review] Remove nsIDOMHTMLParamElement r=bz r=me
Attachment #782988 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 43•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #34) > Comment on attachment 782934 [details] [diff] [review] > Remove nsIDOMHTMLLegendElement r=bz > > The QI bits are a bit weird. Can we not just use an interface map > throughout if that's what we want, since now there is no interface table > involved at all? > > r=me with that fixed or a followup filed for better macros if we need them. Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does and then get rid of the nsISupports declarations/implementations on these classes?
Comment 44•11 years ago
|
||
Comment on attachment 782989 [details] [diff] [review] Remove nsIDOMHTMLMeterElement r=bz Same thing with QI. Please file a followup to make SetDoubleAttr take an ErrorResult out param? > // NOTE: if we were allowed to static_cast to HTMLMeterElement we would be > // able to use nicer getters... That comment can go away now, right? r=me
Attachment #782989 -
Flags: review?(bzbarsky) → review+
Comment 45•11 years ago
|
||
> Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does
The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface.
If we got rid of all these nsIDOMHTMLElement subclasses, we could just implement nsIDOMHTMLElement and company on nsGenericHTMLElement, and things would just work...
Comment 46•11 years ago
|
||
Comment on attachment 782990 [details] [diff] [review] Remove nsIDOMHTMLDListElement r=bz r=me Note that this is an example of how to avoid having an interface table for an element class if you wanted one. ;)
Attachment #782990 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 47•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #45) > > Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does > > The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the > object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface. > > If we got rid of all these nsIDOMHTMLElement subclasses, we could just > implement nsIDOMHTMLElement and company on nsGenericHTMLElement, and things > would just work... Oh, right. We can just make the nsIDOMHTMLFooElement inherit from nsISupports and put nsIDOMHTMLElement on nsGenericHTMLElement.
Comment 48•11 years ago
|
||
We could, at a binary compat hit, yes.
Reporter | ||
Comment 49•11 years ago
|
||
OK, I will change the QI implementations to use maps and we can figure out what we want to do in a followup.
Comment 50•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #45) > > Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does > > The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the > object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface. Hmm, or we could use AsDOMNode()...
Comment 51•11 years ago
|
||
That's an extra virtual function call, right?
Comment 52•11 years ago
|
||
Yes, but you're QIing to nsIDOMHTMLElement, so you're getting a lot of virtual calls anyway if you want to do anything with it. I don't feel strongly about it, but I thought it was worth considering.
Reporter | ||
Comment 53•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #48) > We could, at a binary compat hit, yes. Authors will notice this because their code will no longer compile, and the solution is to stick in a QI call which is pretty easy. It will only break extensions that are no longer being updated.
Comment 54•11 years ago
|
||
> Yes, but you're QIing to nsIDOMHTMLElement True. This used to be more of an issue when we had bindings code doing that... > Authors will notice this because their code will no longer compile That's true. OK, I'm happy to try giving it a shot, now that the xpidl inheritance hierarchy doesn't affect JS proto chains. ;)
Reporter | ||
Comment 55•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4aa91081fb https://hg.mozilla.org/integration/mozilla-inbound/rev/928c54c1c5a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/606feefec994 https://hg.mozilla.org/integration/mozilla-inbound/rev/b458dd12993f https://hg.mozilla.org/integration/mozilla-inbound/rev/97b2b5990840 https://hg.mozilla.org/integration/mozilla-inbound/rev/da2f53bb684d https://hg.mozilla.org/integration/mozilla-inbound/rev/6106c5833bb1 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8589c5d7ca https://hg.mozilla.org/integration/mozilla-inbound/rev/c15f9ade4b80 https://hg.mozilla.org/integration/mozilla-inbound/rev/46425015b8cb https://hg.mozilla.org/integration/mozilla-inbound/rev/ce76e215e532 https://hg.mozilla.org/integration/mozilla-inbound/rev/a414040f5004 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5456df9ee6 https://hg.mozilla.org/integration/mozilla-inbound/rev/5a51269a168b https://hg.mozilla.org/integration/mozilla-inbound/rev/a97e4dc75260 https://hg.mozilla.org/integration/mozilla-inbound/rev/19e69f5e906c https://hg.mozilla.org/integration/mozilla-inbound/rev/6d686a8cf8f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4bd60528aef
Whiteboard: [leave open]
Comment 56•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0a4aa91081fb https://hg.mozilla.org/mozilla-central/rev/928c54c1c5a3 https://hg.mozilla.org/mozilla-central/rev/606feefec994 https://hg.mozilla.org/mozilla-central/rev/b458dd12993f https://hg.mozilla.org/mozilla-central/rev/97b2b5990840 https://hg.mozilla.org/mozilla-central/rev/da2f53bb684d https://hg.mozilla.org/mozilla-central/rev/6106c5833bb1 https://hg.mozilla.org/mozilla-central/rev/cd8589c5d7ca https://hg.mozilla.org/mozilla-central/rev/c15f9ade4b80 https://hg.mozilla.org/mozilla-central/rev/46425015b8cb https://hg.mozilla.org/mozilla-central/rev/ce76e215e532 https://hg.mozilla.org/mozilla-central/rev/a414040f5004 https://hg.mozilla.org/mozilla-central/rev/1d5456df9ee6 https://hg.mozilla.org/mozilla-central/rev/5a51269a168b https://hg.mozilla.org/mozilla-central/rev/a97e4dc75260 https://hg.mozilla.org/mozilla-central/rev/19e69f5e906c https://hg.mozilla.org/mozilla-central/rev/6d686a8cf8f9 https://hg.mozilla.org/mozilla-central/rev/d4bd60528aef
Comment 57•11 years ago
|
||
Backed out for Win8 mc bustage (see bug 895873) - https://tbpl.mozilla.org/?tree=Try&rev=b8ccf485ab7a https://hg.mozilla.org/integration/mozilla-inbound/rev/d27067519f79
Reporter | ||
Comment 58•11 years ago
|
||
Relanded the parts that didn't touch metro code: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8815f7b05c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5334fe747 https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7693e0366e https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b9d85ab129 https://hg.mozilla.org/integration/mozilla-inbound/rev/285270e7f0fa https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbe189bab20 https://hg.mozilla.org/integration/mozilla-inbound/rev/1ddf2f76d4df https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7bac477e24 https://hg.mozilla.org/integration/mozilla-inbound/rev/194aa4dcfb41 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a3a22bb3479 https://hg.mozilla.org/integration/mozilla-inbound/rev/21aa85792c76 https://hg.mozilla.org/integration/mozilla-inbound/rev/04889c58cda0 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c19a88c6f31 https://hg.mozilla.org/integration/mozilla-inbound/rev/db682c311745 https://hg.mozilla.org/integration/mozilla-inbound/rev/58860f0260d6
Reporter | ||
Comment 59•11 years ago
|
||
and backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b51833fb5854
Comment 60•11 years ago
|
||
Comment on attachment 775758 [details] [diff] [review] Clean up nsFormControlFrame This is mostly fixed in bug 899931.
Attachment #775758 -
Attachment is obsolete: true
Reporter | ||
Comment 61•11 years ago
|
||
Looks green on try https://tbpl.mozilla.org/?tree=Try&rev=746fe812f586 Relanded with a clobber: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8b3436b414 https://hg.mozilla.org/integration/mozilla-inbound/rev/723ce11ef380 https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa6ef73932 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc32ed7cd105 https://hg.mozilla.org/integration/mozilla-inbound/rev/db6daaa146a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae490cb6c7f https://hg.mozilla.org/integration/mozilla-inbound/rev/78af3e75e425 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3f4ebff615 https://hg.mozilla.org/integration/mozilla-inbound/rev/15d38d8c9263 https://hg.mozilla.org/integration/mozilla-inbound/rev/c00202cb97a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/11d7091b3bf4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea6c2e6a468 https://hg.mozilla.org/integration/mozilla-inbound/rev/a944279cd8f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1fcab5ffdf https://hg.mozilla.org/integration/mozilla-inbound/rev/d433739f49d6
Comment 62•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8a8b3436b414 https://hg.mozilla.org/mozilla-central/rev/723ce11ef380 https://hg.mozilla.org/mozilla-central/rev/ebfa6ef73932 https://hg.mozilla.org/mozilla-central/rev/cc32ed7cd105 https://hg.mozilla.org/mozilla-central/rev/db6daaa146a1 https://hg.mozilla.org/mozilla-central/rev/2ae490cb6c7f https://hg.mozilla.org/mozilla-central/rev/78af3e75e425 https://hg.mozilla.org/mozilla-central/rev/5b3f4ebff615 https://hg.mozilla.org/mozilla-central/rev/15d38d8c9263 https://hg.mozilla.org/mozilla-central/rev/c00202cb97a7 https://hg.mozilla.org/mozilla-central/rev/11d7091b3bf4 https://hg.mozilla.org/mozilla-central/rev/1ea6c2e6a468 https://hg.mozilla.org/mozilla-central/rev/a944279cd8f7 https://hg.mozilla.org/mozilla-central/rev/8e1fcab5ffdf https://hg.mozilla.org/mozilla-central/rev/d433739f49d6
Reporter | ||
Comment 63•11 years ago
|
||
Attachment #792451 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 64•11 years ago
|
||
Attachment #792452 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 65•11 years ago
|
||
Attachment #792453 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 66•11 years ago
|
||
Attachment #792454 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 67•11 years ago
|
||
Attachment #792455 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 68•11 years ago
|
||
Attachment #792456 -
Flags: review?(bzbarsky)
Comment 69•11 years ago
|
||
Comment on attachment 792451 [details] [diff] [review] Don't use nsIDOMHTMLFoo interfaces in metro r=bz This seems fine to me, but needs review from someone who knows whether all the relevant code is running in a window scope.
Attachment #792451 -
Flags: review?(bzbarsky) → feedback+
Comment 70•11 years ago
|
||
Comment on attachment 792452 [details] [diff] [review] Don't use nsIDOMHTMLFoo interfaces in mobile/ r=bz Again, seems fine but needs review from someone familiar with this code.
Attachment #792452 -
Flags: review?(bzbarsky) → feedback+
Comment 71•11 years ago
|
||
Comment on attachment 792453 [details] [diff] [review] Remove nsIDOMHTMLDivElement r=bz Given the shim bit, why did we need the earlier removals? What's the reason for the test changes here?
Comment 72•11 years ago
|
||
Comment on attachment 792454 [details] [diff] [review] Remove nsIDOMHTMLParagraphElement r=bz Have the getter take a DOMString? r=me with that
Attachment #792454 -
Flags: review?(bzbarsky) → review+
Comment 73•11 years ago
|
||
Comment on attachment 792455 [details] [diff] [review] Remove nsIDOMHTMLTableCellElement r=bz r=me
Attachment #792455 -
Flags: review?(bzbarsky) → review+
Comment 74•11 years ago
|
||
Comment on attachment 792456 [details] [diff] [review] Remove nsIDOMHTMLAnchorElement r=bz >+++ b/editor/libeditor/html/nsHTMLEditor.cpp >+ nsRefPtr<HTMLAnchorElement> anchor = HTMLAnchorElement::FromContent(content); FromContentOrNull, I think. r=me with that
Attachment #792456 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 75•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #71) > Comment on attachment 792453 [details] [diff] [review] > Remove nsIDOMHTMLDivElement r=bz > > Given the shim bit, why did we need the earlier removals? > They're not necessary. It's just nicer to not use the Ci stuff and it passes tests. > What's the reason for the test changes here? The test is making sure that we only add interfaces to the shim if they exist on the real Ci object, which is no longer the case.
Comment 76•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #75) > The test is making sure that we only add interfaces to the shim if they > exist on the real Ci object, which is no longer the case. What exact interface is present only on the shim? You should remove it from the shim instead.
Reporter | ||
Comment 77•11 years ago
|
||
Hmm, I think the shim isn't quite what we want here. The shim is exposed to web content, so instead we want to define nsIDOMHTMLDivElement only on chrome globals, or something.
Comment 78•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #77) > Hmm, I think the shim isn't quite what we want here. The shim is exposed to > web content, so instead we want to define nsIDOMHTMLDivElement only on > chrome globals, or something. On globals? not on Ci? If you want to expose nsIDOMHTMLDivElement on chrome, why are you going to remove the interface in the first place? If you keep nsIDOMHTMLDivElement intact and remove it from the shim, it will be exposed only on the real Ci (a.k.a. chrome). What's wrong with that?
Reporter | ||
Comment 79•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #78) > (In reply to David Zbarsky (:dzbarsky) from comment #77) > > Hmm, I think the shim isn't quite what we want here. The shim is exposed to > > web content, so instead we want to define nsIDOMHTMLDivElement only on > > chrome globals, or something. > > On globals? not on Ci? No I meant we want to define it on Ci, but only on chrome globals. > If you want to expose nsIDOMHTMLDivElement on chrome, why are you going to > remove the interface in the first place? > If you keep nsIDOMHTMLDivElement intact and remove it from the shim, it will > be exposed only on the real Ci (a.k.a. chrome). What's wrong with that? We'd like to remove nsIDOMHTMLDivElement because it's not needed and lets us shrink elements by a pointer. The only thing that needs to work is (element instanceof Ci.nsIDOMHTMLDivElement) because addons do that. Something like the shim lets us effectively map that to (element instanceof HTMLDivElement) which still works.
Reporter | ||
Comment 80•11 years ago
|
||
Attachment #792453 -
Attachment is obsolete: true
Attachment #792453 -
Flags: review?(bzbarsky)
Attachment #794105 -
Flags: review?(bzbarsky)
Comment 81•11 years ago
|
||
Comment on attachment 794105 [details] [diff] [review] Remove nsIDOMHTMLDivElement r=bz r=me. I assume you'll make some of this other stuff chromeonly too, right?
Attachment #794105 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 82•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #81) > Comment on attachment 794105 [details] [diff] [review] > Remove nsIDOMHTMLDivElement r=bz > > r=me. > > I assume you'll make some of this other stuff chromeonly too, right? Yeah, I'll move the entries for the rest of the elements to the end
Comment 83•11 years ago
|
||
Comment on attachment 794105 [details] [diff] [review] Remove nsIDOMHTMLDivElement r=bz ::: dom/base/nsDOMClassInfo.cpp > + bool globalIsChrome = xpc::AccessCheck::isChrome(global); > for (uint32_t i = 0; i < ArrayLength(kInterfaceShimMap); ++i) { > > + // Skip ChromeOnly interface shim entries on content globals. > + if (kInterfaceShimMap[i].isChromeOnly && !globalIsChrome) { > + continue; > + } > + DefineComponentShim will not be even called on chrome at all. I don't understand why this helps for add-ons.
Comment 85•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #83) > Comment on attachment 794105 [details] [diff] [review] > Remove nsIDOMHTMLDivElement r=bz > > ::: dom/base/nsDOMClassInfo.cpp > > + bool globalIsChrome = xpc::AccessCheck::isChrome(global); > > for (uint32_t i = 0; i < ArrayLength(kInterfaceShimMap); ++i) { > > > > + // Skip ChromeOnly interface shim entries on content globals. > > + if (kInterfaceShimMap[i].isChromeOnly && !globalIsChrome) { > > + continue; > > + } > > + > > DefineComponentShim will not be even called on chrome at all. I don't > understand why this helps for add-ons. Well, theoretically if chrome wanted to access someContentWindow.Components, it would get the Components shim (given how nsWindowSH::NewResolve works). But yes, putting something on the shim and not on the real Ci won't help addons that try to do "foopy instanceof Ci.nsIFoo".
Flags: needinfo?(bobbyholley+bmo)
Reporter | ||
Comment 86•11 years ago
|
||
Bobby, how/where do we set up the real Ci object?
Flags: needinfo?(bobbyholley+bmo)
Comment 87•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #86) > Bobby, how/where do we set up the real Ci object? See this stuff: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#250
Flags: needinfo?(bobbyholley+bmo)
Comment 88•11 years ago
|
||
It seems that parts of this bug have been landing already, and I learned about them from an add-ons developer whose add-on was broken. Since we look for compatibility issues by milestone, for resolved bugs, this bug hasn't been on our radar. Could bugs like this be split into dependencies so the various parts that land have their own milestone and compatibility flags? Is this something that needs to be discussed in the newsgroups to adjust the workflow?
Comment 89•11 years ago
|
||
Hmm. We should in fact split this up, but which addon was broken and how? We aimed to not remove anything that the addons mxr showed was being used....
Comment 90•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #89) > Hmm. We should in fact split this up, but which addon was broken and how? > We aimed to not remove anything that the addons mxr showed was being used.... I am the add-on developer who brought this to Jorge's attention. Unfortunately, the add-on that broke is not hosted on AMO (we created it for another company). It contained an instanceof Components.interfaces.nsIDOMHTMLParamElement check. There are a lot of add-ons that are not on AMO ;)
Comment 91•11 years ago
|
||
I'm glad to hear that the MXR checks were done for these changes. That at least minimizes the potential impact. However, we do post a detailed update in the Add-ons Blog for every Firefox version, and we include changes in interfaces that don't necessarily show up in MXR queries.
Comment 92•11 years ago
|
||
Mark, sorry this caused problems for you guys. :( Jorge, in the future we'll make sure to resolve bugs that make changes like this and open new ones for continuing work!
Comment 93•10 years ago
|
||
Our add-on was broken too :( why was this defect opened ? what is the trigger for deleting all those nsIDOMHTMLFooElement ? What do you guys suggest that I do in my code where I use some of the elements that were deleted (nsIDOMHTMLTableRowElement & nsIDOMHTMLLegendElement) ? Thanks in advance for your help !
Comment 94•10 years ago
|
||
> what is the trigger for deleting all those nsIDOMHTMLFooElement ? It reduces the memory consumption of element classes, as well as reducing the on-disk and memory footprint of the binary (because of the removed code) and making the code simpler and hence more maintainable. > What do you guys suggest that I do in my code where I use some of the elements that were > deleted (nsIDOMHTMLTableRowElement & nsIDOMHTMLLegendElement) ? Is this C++ code or JS code? How exactly were you using theseinterfaces? Was it for instanceof tests like comment 90, or something else?
Comment 95•10 years ago
|
||
C++ code, and I use these interfaces to get information on the different DOM elements. For example, following is how I use nsIDOMHTMLTableRowElement (a code that doesn't compile anymore because I don't have the needed methods & interfaces anymore). Is there a workaround for this ? nsCOMPtr<nsIDOMHTMLCollection> rows; m_tableElem->GetRows(getter_AddRefs(rows)); // m_tableElem is of type nsIDOMHTMLTableElement for (unsigned int i = 0; i < rowsCount; i++) { pRows->Item(i, getter_AddRefs(pNode)); if (pNode == NULL) return E_FAIL; nsCOMPtr<nsIDOMHTMLTableRowElement> pRowElement(do_QueryInterface(pNode)); if (FAILED(pRowElement->GetCells(getter_AddRefs(pCells))) || pCells==NULL) return E_FAIL; // Some logic here } Thanks a lot for this.
Comment 96•10 years ago
|
||
The C++ story is not great. :( I assume just QI to mozilla::dom::Element and then casting to HTMLTableRowElement won't work, because HTMLTableRowElement::Cells is not virtual and presumably not exported. The simplest solution for this particular case on your side is probably to just walk through the kids the the Element and check for the ones that are IsHTML() and have the right localName ("td" or th"). But in general we need to figure what our C++ API story is here...
Flags: needinfo?(peterv)
Comment 97•10 years ago
|
||
I agree with you on that. But maybe until then you guys should stop working on this "defect" because it's causing C++ Add-on developers some serious headache :) ! PS Will C++ Add-ons become obsolete over time ?
Comment 98•10 years ago
|
||
I don't think there are any current plans to remove more of the nsIDOM* APIs for the moment.
> PS Will C++ Add-ons become obsolete over time ?
In the grand scheme of things, yes (e.g. they obviously can't work with Servo).
Comment 99•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #96) > But in general we need to figure what our C++ API story is here... I was under the impression that the solution going forward for interacting with the DOM in addons was to do it in JS.
Comment 100•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #99) > (In reply to Boris Zbarsky [:bz] from comment #96) > > But in general we need to figure what our C++ API story is here... > > I was under the impression that the solution going forward for interacting > with the DOM in addons was to do it in JS. What about existing add-ons ?
Comment 101•10 years ago
|
||
(In reply to George Haddad [:georgehdd] from comment #100) > (In reply to Bobby Holley (:bholley) from comment #99) > What about existing add-ons ? They should hoist their DOM manipulation into JS (writing a JS-implementated component that touches the DOM, and using that component from C++). The DOM specs (and consequently our implementation of them) are just too volatile to expose a useful C++ API to third-party code.
Comment 102•10 years ago
|
||
Also, the DOM specs are more and more tied to JS. There are some parts of them that can't even be expressed in C++ terms without involving the JS engine.
Comment 103•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #101) > (In reply to George Haddad [:georgehdd] from comment #100) > > (In reply to Bobby Holley (:bholley) from comment #99) > > What about existing add-ons ? > > They should hoist their DOM manipulation into JS (writing a JS-implementated > component that touches the DOM, and using that component from C++). The DOM > specs (and consequently our implementation of them) are just too volatile to > expose a useful C++ API to third-party code. Do you have an example of how to do that ?
Comment 104•10 years ago
|
||
This might get you started: https://developer.mozilla.org/en-US/docs/How_to_Build_an_XPCOM_Component_in_Javascript The simplest in-tree example of a js-implemented XPCOM component is in the XPConnect test suite - see js/xpconnect/tests/components/js.
Comment 105•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #96) > The simplest solution for this particular case on your side is probably to > just walk through the kids the the Element and check for the ones that are > IsHTML() and have the right localName ("td" or th"). > > But in general we need to figure what our C++ API story is here... I'm not sure there's much of a story here. We could add helper functions somewhere that emulate some of the member functions that we removed, but make them take an Element. That at least wouldn't cost us any performance, but I'm not too fond of it.
Flags: needinfo?(peterv)
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Comment 106•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: dzbarsky → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•