Data Manager enhancements for non domain permissions and preferences

RESOLVED FIXED in seamonkey2.45

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

unspecified
seamonkey2.45
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
The Data Manager will currently not display and process all permissions and preferences correctly. Only permissions with a common scheme and preferences for a valid host are supported. Others like chrome://, file:// and for internal pages like about: will either be displayed as a blank domain or with an arbitrary host. 

This needs to be fixed.
Assignee

Comment 1

3 years ago
Moves all non standard hosts and schemes to the * global data domain for permissions and preferences.

Tested with various schemes eg. chrome:// and files:// and internal pages like setting zoom factor for about:blank
Attachment #8747527 - Flags: review?(philip.chee)
Assignee

Updated

3 years ago
Depends on: 1188348

Comment 2

3 years ago
Comment on attachment 8747527 [details] [diff] [review]
1269195-DataManagerEnhancement.patch

r=me with the following fixed:

In the "*" domain I have file:/// mailbox:/// about:
In the ":" domain I have jar:/ <- Shouldn't this go under "*"

Did you test for resource: and gopher: ?

> -        while (statement.executeStep())
> -          gDomains.addDomainOrFlag(statement.row["host"], "hasPreferences");
> +        while (statement.executeStep()) {
> +          gDataman.debugMsg("Found pref: " + statement.row["host"]);
> +          var prefHost = gDomains.getDomainFromHostWithCheck(statement.row["host"]);
To be consistent. use "let" in scoped blocks if/do/while/try/catch

> +  getDomainFromHostWithCheck: function domain_getDomainFromHostWithCheck(aHost)  {
> +    let host = gDomains.getDomainFromHost(aHost);
Maybe: let host = gDomains.getDomainFromHost(aHost).trim();

> +    if ((host.trim().length == 0) ||
> +        (aHost.slice(0,6) == "about:"))
Is this a string? If so you can use aHost.startsWith("about:");

Due to operator precedence you don't need the internal ()'s so just use:
if (!host.length || aHost.startsWith("about:"))

> +  // Used for checking if * global data domain should be used.
> +  commonScheme: function domain_commonScheme(aScheme) {
> +    if (aScheme != "http" &&
> +        aScheme != "https" &&
> +        aScheme != "ftp") {
> +      return false;
>      }
> -    return this.xlcache[aHostname];
> +
> +    return true;
>    },
Just use (SeaMonkey coding style):
return /^(https?|ftp):/i.test(aScheme);
Attachment #8747527 - Flags: review?(philip.chee) → review+
Assignee

Comment 3

3 years ago
>> In the ":" domain I have jar:/ <- Shouldn't this go under "*"

You found another case where the prefs api did put the full url into the group pref as a host. This needs the same static treatment as about: or should I try to parse the host?

>> Did you test for resource: and gopher: ?

Now I did:) resource seems to work ok. I didn't think the gopher api was still supported by the permission manager but with the overbite add-on a valid permission can be added. Didn't use gopher in ages. I need to put gopher in the commonScheme function or the pref host ends up in the domains and the permission in *. There are maybe others but I can't think fo them right now.

Will fix the other remarks. Ok with these additional changes or should I set the review again when I am finished?
Flags: needinfo?(philip.chee)

Comment 4

3 years ago
(In reply to Frank-Rainer Grahl from comment #3)
> >> In the ":" domain I have jar:/ <- Shouldn't this go under "*"
> 
> You found another case where the prefs api did put the full url into the
> group pref as a host.

jar: can point to a local file. It can also point to a remote http://
There are a couple of prefs you need to set.

network.jar.block-remote-files;false
network.jar.open-unsafe-types;true

> This needs the same static treatment as about: or
> should I try to parse the host?

I don't think the jar: protocol has the concept of "host".
jar:file:///tmp/simple.blah!/
jar:http://www.skierpage.com/moz_bugs/simple.myarchive!/

> Will fix the other remarks. Ok with these additional changes or should I set
> the review again when I am finished?
I would like a quick peek at your additional changes. Thanks.
Flags: needinfo?(philip.chee)

Comment 5

3 years ago
> return /^(https?|ftp):/i.test(aScheme);
Of course, that should be without the ":". Lower-casing might not be necessary either.
return /^(https?|ftp)/i.test(aScheme);
Assignee

Comment 6

3 years ago
Hopefullly I have covered everything for now. Remote xul also tested.
Attachment #8747527 - Attachment is obsolete: true
Attachment #8750108 - Flags: review?(philip.chee)

Comment 7

3 years ago
Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

r=me
Attachment #8750108 - Flags: review?(philip.chee) → review+
Assignee

Comment 9

3 years ago
Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Domain view faulty. Some permissions and preferences will not be shown under the correct domain or not at all.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): None. Tested
String changes made by this patch: none
Attachment #8750108 - Flags: approval-comm-aurora?

Comment 10

3 years ago
Comment on attachment 8750108 [details] [diff] [review]
1269195-DataManagerEnhancement-V2.patch

a=me for comm-aurora
Attachment #8750108 - Flags: approval-comm-aurora? → approval-comm-aurora+
Assignee

Comment 11

3 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/814f87ba88b5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.