364111 - Don't allow colons in custom toolbar names

Status: NEW

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Phil Ringnalda (:philor)]

Because XUL hates us (bug 251273, bug 215489, bug 220701), we persist the currentset for customized toolbars as attributes on a toolbarset, so that if the toolbar named "foopy" has one element named "goats" on it, we persist |toolbar12345="foopy:goats"|, and then in the toolbar binding we recreate the toolbar with .split(":") and call [0] the name, and [1] the currentset.

That's fine until someone creates a toolbar named "foopy:goats", and then puts the perfectly legally named "<toolbarbutton id="foopy:goats">" button on it, when we'll persist |toolbar1234="foopy:goats:foopy:goats"| and then try to recreate a toolbar named "foopy" and put the element with id="goats" on it. If not for id="foopy:goats", we could .split(":"), pop(), join(":"), to call everything after the last colon the currentset, and everything before the name, but since we can't obscurely break perfectly legal XML, we need to tell the user "No! That's a bad, bad name."

Comment 1

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Comment 2

[Phil Ringnalda (:philor)]

Gavin: do you remember anything we said, when we talked about this in Decemberish? I vaguely remember "the horror, the horror" and not having the feeling that it would be cheap to migrate to something nicer, but that's about it.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't remember anything specific other than the general ickiness I still feel now as I read the patch. Can we just silently strip out colons?

Comment 4

[Phil Ringnalda (:philor)]

Attachment: Fix v.2

Sure, silently stripping them is much easier. I'll get around to looking at how to testcase this, erm, sometime.

Comment 5

[Phil Ringnalda (:philor)]

Though I notice looking at it now, that our behavior when you have an existing toolbar named Foobar, and you try to create Foo:bar, would be exactly the sort of thing that reasonably enrages users.