Closed Bug 833412 Opened 9 years ago Closed 9 years ago

Remove XBL binding layering (addBinding)

Categories

(Core :: XBL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files)

Right now, privileged callers can do |document.addBinding(elem, bindingURI)| to dynamically add bindings to elements. If these elements have existing bindings, the new binding is inserted at the bottom of the prototype chain. Given how binding inheritance works, the same binding can actually end up on the prototype chain twice, with all kinds of wacky effects.

I've recently discovered a major bug in addBindings, which is that it doesn't unbind |elem| before applying the new bindings, leading to even more wacky behavior. But all this machinery appears to be unused (even on AMO mxr) except for a trivial case in nsXMLPrettyPrinter, so bz and I decided we should just remove it.
Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.
(In reply to Olli Pettay [:smaug] from comment #1)
> Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.

You are correct.
Green. Uploading and flagging bz for review.
Attachment #705236 - Flags: review?(bzbarsky)
Comment on attachment 705236 [details] [diff] [review]
Part 1 - Fix tests that call addBinding. v1

> +++ b/content/base/test/test_bug375314.html

Can you please replace the addBinding thing by a style rule that applies a binding to the <div> and then a style flush to cause it to take effect, instead of just losing XBL test coverage?

>+++ b/content/xbl/test/test_bug398492.xul

Likewise.

r=me with those
Attachment #705236 - Flags: review?(bzbarsky) → review+
Comment on attachment 705237 [details] [diff] [review]
Part 2 - Make nsXMLPrettyPrinter use nsXBLService/BindingManger to load/remove bindings. v1

OK.  So you don't need the LoadBindingDocument call because this is a chrome:// URI so will get sync-loaded no matter what, right?  Might be worth commenting that...

This is not running the constructor of the binding.  That might be ok in this case because the binding has no constructor, but please add a comment to the binding XML saying that if a constructor is added this code needs changing.

Or change this code to actually run the constructor, of course.

>+    // Apply the prettprint XBL binding.

"prettyprint"


>     nsCOMPtr<nsIDOMDocument> document = do_QueryInterface(mDocument);
>     nsCOMPtr<nsIDOMElement> rootElem;
>     document->GetDocumentElement(getter_AddRefs(rootElem));
>+    nsCOMPtr<nsIContent> rootContent = do_QueryInterface(rootElem);

How about replacing all that with:

  nsCOMPtr<Element> element = mDocument->GetDocumentElement();

?

r=me with that.
Attachment #705237 - Flags: review?(bzbarsky) → review+
Comment on attachment 705238 [details] [diff] [review]
Part 3 - Remove AddBinding/RemoveBinding and remove dead code. v1

>+++ b/content/xbl/src/nsXBLService.cpp
>-  if (aAugmentFlag) {
...
>-  }
>   else {

That's not good.  That else is now being treated as part of the previous if.  It happens to work out, but that's an accident.

Please remove the else and curlies and outdent the body?

r=me with that
Attachment #705238 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/616244739680
https://hg.mozilla.org/mozilla-central/rev/6dd652b1e555
https://hg.mozilla.org/mozilla-central/rev/fb3098dbf4eb
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 890193
You need to log in before you can comment on or make changes to this bug.