Closed Bug 571598 Opened 10 years ago Closed 10 years ago

Allow XUL documents loaded in about urls to persist attributes


(Core :: XUL, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: mossop, Assigned: mossop)



(Whiteboard: [AddonsRewriteTestday][AddonsRewrite])


(1 file, 1 obsolete file)

Bug 329677 made it so only documents loaded in URLs can persist attributes, but about:addons is a URI only so persistence fails. I have a probably silly patch that just tacks the element ID on the end of the URI.spec and then pulls it off again when asked. I suspect that approach is a little naive though so I'd appreciate some feedback.
Attached patch naive patch (obsolete) — Splinter Review
Attachment #450749 - Flags: feedback?
Whiteboard: [AddonsRewriteTestday][AddonsRewrite]
It'd be really nice if nsIURIs just supported refs...

In any case, that patch fails if an ID contains the '#' character, no?  Worth adding a test for that.
(In reply to comment #2)
> It'd be really nice if nsIURIs just supported refs...
> In any case, that patch fails if an ID contains the '#' character, no?  Worth
> adding a test for that.

Yeah dolske pointed that out too. I think the choices would either be to do some escaping in that case, or just make it fail for IDs with #'s, seems like they should be rare enough so I'm not sure it is worth the energy to support it.
blocking2.0: --- → ?
Comment on attachment 450749 [details] [diff] [review]
naive patch

Having had to unite the internal and external string APIs for bug 377319 I can't seem to look at a patch without commenting on its string API usage ;-)

>+        aURI.AppendLiteral("#");
.Append works on single chars too.

>+        const char* start = aURI.BeginReading();
>+        const char* end = aURI.EndReading();
>+        const char* chr = end;
>+        while (--chr >= start) {
>+            if (*chr == '#') {
(R)FindChar, perhaps? (R if you want to support base URIs containing #s)
Hmm, at some point about:addons must know it's loaded from extensions.xul so maybe we can save that somewhere and use that as the base for persistence.
The xul document knows about two URIs, in theory: the original URI of its channel, and the URI of its channel.

In this case those would be the about:addons URI and a jar:file:// URI, I'd think.
Blocks: 577990
--> betaN, but I bet dtownsend has a better idea of when this should actually be pulled in.
blocking2.0: beta2+ → betaN+
Assignee: nobody → dtownsend
Attached patch patch rev 1Splinter Review
This is the patch I have here. It works mostly as before, appending a ref to the spec of uris that don't already support it and stripping them after. However I've added some encoding to the ref to escape out any # characters (I'm not sure I have chosen the best escape options but it certainly escapes #).

Testing this directly is tricky however I've added a test for bug 567137 which gives us some basic coverage here, I'm open to other suggestions for how this might be tested.
Attachment #450749 - Attachment is obsolete: true
Attachment #461000 - Flags: review?(bzbarsky)
Attachment #450749 - Flags: feedback?
Comment on attachment 461000 [details] [diff] [review]
patch rev 1

Attachment #461000 - Flags: review?(bzbarsky) → review+
Landed and tested by virtue of the test for bug 567137.
Closed: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Verified fixed by checking the persistence of the new add-ons manager categories with Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100802 Minefield/4.0b3pre
Depends on: 584693
You need to log in before you can comment on or make changes to this bug.