Closed Bug 405044 Opened 17 years ago Closed 17 years ago

clean up updates.xml

Categories

(Toolkit :: Application Update, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file)

updates.xml has two "update" bindings and something strange near
the end of file. See lines 10,340,390
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/update/content/updates.xml&rev=1.33&mark=10,340,390
Can't fix bug 401907 before this.
Is the "update" binging even used anywhere?
(So great that updates.xml is probably mostly unreviewed code and blame doesn't
 point to any bugs.)
Anyone familiar with updater code?
It would sure be easier to know if the original bugs had patches, let alone were reviewed :(

It seems to be used by: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/update/content/history.js&rev=1.7&mark=64#64
Attached patch possible patchSplinter Review
This might be the right patch, but I don't really know how to test this.
Comment on attachment 289937 [details] [diff] [review]
possible patch

Tested using updates.xml.
With the patch UI looks the same. If the second binding is removed and first one kept, UI changes.
Attachment #289937 - Flags: review?(gavin.sharp)
Attachment #289937 - Flags: review?(gavin.sharp) → review+
Comment on attachment 289937 [details] [diff] [review]
possible patch

Bug 401907 needs this one fixed.
Attachment #289937 - Flags: approval1.9?
Comment on attachment 289937 [details] [diff] [review]
possible patch

a=beltzner for drivers
Attachment #289937 - Flags: approval1.9? → approval1.9+
Assignee: nobody → Olli.Pettay
> If the second binding is removed and first one kept, UI changes.

questions:

should we be concerned that the "last binding wins"?  (then again, if you are able to provide an xbl binding, the horse is out of the barn.)

Would it be worth logging a xbl bug about checking if we have an existing binding to ignore any other bindings (and assert?)

Could someone write a theme and and override an existing binding?  (I forget what themes can and can't do.)

do we (or extension authors) rely on "last binding wins" to purposefully override bindings?

forgive the private comment.  if this isn't legitimately security sensitive, we can mark it un-private and reveal my ignorance!
(In reply to comment #7)
> > If the second binding is removed and first one kept, UI changes.
> 
> questions:
> 
> should we be concerned that the "last binding wins"?  (then again, if you are
> able to provide an xbl binding, the horse is out of the barn.)

I can't think of a case where this would be a problem, other than that it could be a source of bugs like any somewhat unexpected/ambiguous behavior in a programming language.

> Would it be worth logging a xbl bug about checking if we have an existing
> binding to ignore any other bindings (and assert?)

Yes, I think it would be a good idea to assert.

In general we shouldn't assert on errors that end-users can cause. However the XBL code is full of such assertions. Ideally we should change these to assert only for chrome-xbl, and just log to the error console for http-xbl. But that's a separate bug.

> Could someone write a theme and and override an existing binding?  (I forget
> what themes can and can't do.)

Yes. You can simply write a new rule in one of the stylesheets and override the -moz-binding CSS property. This is just how XBL1 works unfortunately.

> do we (or extension authors) rely on "last binding wins" to purposefully
> override bindings?

The "last binding wins" rule is only within a single XBL file. So I don't think this is a problem in any way.

> forgive the private comment.  if this isn't legitimately security sensitive, 
> we can mark it un-private and reveal my ignorance!

I think we can unmark it
jonas, thanks for answering all my questions (and more).

> I think it would be a good idea to assert.

I've logged bug #405563 

Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: