Closed
Bug 556334
Opened 15 years ago
Closed 14 years ago
Allow LightweightThemeManager to accept file URIs
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: rdoherty, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
8.03 KB,
patch
|
mossop
:
review+
beltzner
:
approval1.9.2.5+
|
Details | Diff | Splinter Review |
[spin-off of bug 554220]
The Personas Plus add-on has a 'custom persona' feature where users can select images from their computer to use. Previous to 3.6.2 the LightweightThemeManager allowed local file urls. A change in 3.6.2 now does not allow this, which breaks this functionality for many users.
STR:
1) Firefox 3.6.2 + Personas Plus 1.5.2
2) Tools => Personas => Preferences => Check 'Show Custom Persona in Menu'
3) Tools => Personas => Custom Persona => Edit
4) Select header and footer images from computer
5) See preview
6) Close tab
7) Custom persona is removed
Expected results:
Custom persona stays applied.
Comment 1•15 years ago
|
||
the same is true of a clean install of Firefox 3.6.3 and Personas 1.5.2 on a slackware 13 machine.
Comment 2•15 years ago
|
||
This is still an issue for Firefox 3.6.3, Personas 1.5.2 on Win7x64.
Comment 3•15 years ago
|
||
Dao: do we know what caused this? It would be something in this list:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20status1.9.2:.2-fixed
Keywords: regression
Comment 4•15 years ago
|
||
(In reply to comment #3)
> Dao: do we know what caused this? It would be something in this list:
>
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20status1.9.2:.2-fixed
It was bug 544753 (the one in the blockers for this bug)
Assignee | ||
Comment 5•15 years ago
|
||
We probably don't want to allow file URIs generally, at least not on a stable branch, and doing it only for the currentTheme setter seems strange, so I suggest we make it opt-in.
I renamed callback to successCallback because I erroneously thought it should be called in any case.
Assignee | ||
Updated•15 years ago
|
Version: unspecified → 1.9.2 Branch
Assignee | ||
Updated•15 years ago
|
blocking1.9.2: --- → ?
status1.9.2:
--- → ?
Comment 6•15 years ago
|
||
Comment on attachment 437276 [details] [diff] [review]
patch
I don't think this is the right way to go. As soon as the personas extension allows local resources webpages will be able to use file urls too which we don't want. I actually think that allowing file uri's through directly setting currentTheme isn't all that strange, it just means webpages have more restrictions than local extensions do which is pretty normal.
How about in _sanitizeTheme if aBaseURI is http/https then we restrict to http/https, otherwise we allow file too. That should fix this without the personas extension having to change anything. It would also allow webpages accessed via a file:/// url to use local files but that seems reasonable to me.
Or if we wanted to go the whole hog and use the content policy service to tell us if a particular URI is allowed to use the images it requests but I don't think that is needed for this bug.
Attachment #437276 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6)
> (From update of attachment 437276 [details] [diff] [review])
> I don't think this is the right way to go. As soon as the personas extension
> allows local resources webpages will be able to use file urls too which we
> don't want.
No, they won't, as it's only allowed once, i.e. personas should use it when setting the custom/local persona.
> I actually think that allowing file uri's through directly setting
> currentTheme isn't all that strange, it just means webpages have more
> restrictions than local extensions do which is pretty normal.
It would actually lead to the problem you mentioned above when extensions use currentTheme to install extensions from the web (e.g. through the personas menu).
Comment 8•15 years ago
|
||
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 437276 [details] [diff] [review] [details])
> > I don't think this is the right way to go. As soon as the personas extension
> > allows local resources webpages will be able to use file urls too which we
> > don't want.
>
> No, they won't, as it's only allowed once, i.e. personas should use it when
> setting the custom/local persona.
Oh I see, but no setting a property that modifies the behavior of currentTheme for one time is not the right thing to do here.
> > I actually think that allowing file uri's through directly setting
> > currentTheme isn't all that strange, it just means webpages have more
> > restrictions than local extensions do which is pretty normal.
>
> It would actually lead to the problem you mentioned above when extensions use
> currentTheme to install extensions from the web (e.g. through the personas
> menu).
That's a good point, but extensions could call parseTheme first including a base URI to verify the theme is valid.
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #8)
> > No, they won't, as it's only allowed once, i.e. personas should use it when
> > setting the custom/local persona.
>
> Oh I see, but no setting a property that modifies the behavior of currentTheme
> for one time is not the right thing to do here.
Would a dedicated setter or method be better?
> > It would actually lead to the problem you mentioned above when extensions use
> > currentTheme to install extensions from the web (e.g. through the personas
> > menu).
>
> That's a good point, but extensions could call parseTheme first including a
> base URI to verify the theme is valid.
They could, but I'm not convinced they will... They might as well use JSON.parse and currentTheme. I still find it hard to follow why parsing a string would verify its integrity but setting a theme wouldn't.
Comment 10•15 years ago
|
||
(In reply to comment #9)
> (In reply to comment #8)
> > > No, they won't, as it's only allowed once, i.e. personas should use it when
> > > setting the custom/local persona.
> >
> > Oh I see, but no setting a property that modifies the behavior of currentTheme
> > for one time is not the right thing to do here.
>
> Would a dedicated setter or method be better?
Yeah something dedicated would seem much better to me.
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #437276 -
Attachment is obsolete: true
Attachment #437356 -
Flags: review?(dtownsend)
Comment 12•15 years ago
|
||
Please nominate for approval once it's reviewed and baked.
blocking1.9.2: ? → needed
Assignee | ||
Comment 13•15 years ago
|
||
Dave, ping?
Comment 14•15 years ago
|
||
Comment on attachment 437356 [details] [diff] [review]
patch v2
Sorry this slipped off my list.
Setting a variable to change the behaviour of sanitizeTheme is still not the right way to go. Instead I think separate out the bulk of the currentTheme setter into a private function and then have currentTheme and currentThemeWithLocalResources call sanitizeTheme with an argument saying whether to allow resources and then call the new private function.
Not entirely sure I'm sold on currentThemeWithLocalResources as a name either, what do you think about just setLocalTheme? It should be a method rather than a setter with no matching getter I think.
Attachment #437356 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 15•15 years ago
|
||
(In reply to comment #14)
> Setting a variable to change the behaviour of sanitizeTheme is still not the
> right way to go.
I don't understand why that's a problem, as the variable can't be accessed from the outside.
Comment 16•15 years ago
|
||
(In reply to comment #15)
> (In reply to comment #14)
> > Setting a variable to change the behaviour of sanitizeTheme is still not the
> > right way to go.
>
> I don't understand why that's a problem, as the variable can't be accessed from
> the outside.
Using global variables to alter a function's behaviour makes sense when they are essentially caching preferences but for one time changes it is bad design.
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #437356 -
Attachment is obsolete: true
Attachment #441449 -
Flags: review?(dtownsend)
Updated•15 years ago
|
Attachment #441449 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 18•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Assignee | ||
Updated•15 years ago
|
Attachment #441449 -
Flags: approval1.9.2.5?
Reporter | ||
Comment 19•15 years ago
|
||
What version of Firefox will this be a part of?
Updated•15 years ago
|
Attachment #441449 -
Flags: approval1.9.2.5? → approval1.9.2.5+
Comment 20•15 years ago
|
||
(In reply to comment #19)
> What version of Firefox will this be a part of?
We'll get it landed for Firefox 3.6.5
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 21•15 years ago
|
||
Landed on branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a17a4f056c49
Keywords: checkin-needed
Comment 22•14 years ago
|
||
This bug seems to have reappeared in Firefox 3.6.8.
When the Personas add-on tries to use the LightweightThemeManager.setLocalTheme method, the following error is reported:
"Error: uncaught exception: 2147942487"
The persona object being passed is the following:
{"id":0,
"name":"My Custom Persona",
"headerURL":"file:///Users/josenriq/Pictures/rayTrace1.jpg?298",
"footerURL":"file:///Users/josenriq/Pictures/rayTrace3.jpg?6901",
"custom":true,
"textcolor":"#FFCC00",
"accentcolor":"#006600"}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 23•14 years ago
|
||
(In reply to comment #22)
> This bug seems to have reappeared in Firefox 3.6.8.
It's probably better to open new bugs when finding problems that are believed to be fixed.
In this case the problem is that the id property is expected to be a string and not a plain integer as you've used. Changing it to a string makes it work correctly.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•