Allow XUL documents loaded in about urls to persist attributes

VERIFIED FIXED in mozilla2.0b3

Status

()

Core
XUL
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla2.0b3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [AddonsRewriteTestday][AddonsRewrite])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 450749 [details] [diff] [review]
naive patch
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.
(Assignee)

Comment 3

8 years ago
(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 4

8 years ago
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)

Comment 5

8 years ago
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.
blocking2.0: ? → beta2+

Updated

8 years ago
Blocks: 577990
--> betaN, but I bet dtownsend has a better idea of when this should actually be pulled in.
blocking2.0: beta2+ → betaN+
(Assignee)

Updated

8 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 8

8 years ago
Created attachment 461000 [details] [diff] [review]
patch rev 1

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?
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Comment on attachment 461000 [details] [diff] [review]
patch rev 1

r=bzbarsky
Attachment #461000 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

8 years ago
Landed and tested by virtue of the test for bug 567137.
http://hg.mozilla.org/mozilla-central/rev/92c3a3a02405
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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
Status: RESOLVED → VERIFIED

Updated

8 years ago
Depends on: 584693
You need to log in before you can comment on or make changes to this bug.