Closed
Bug 833412
Opened 12 years ago
Closed 12 years ago
Remove XBL binding layering (addBinding)
Categories
(Core :: XBL, defect)
Core
XBL
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(3 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #1)
> Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.
You are correct.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Green. Uploading and flagging bz for review.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #705236 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #705237 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #705238 -
Flags: review?(bzbarsky)
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
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: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•