Closed Bug 1180871 Opened 4 years ago Closed 4 years ago

Since Bug 1170200 the API for nsIPermissionManager::Remove() accepts a URI instead of a string.

Categories

(SeaMonkey :: Passwords & Permissions, defect)

SeaMonkey 2.39 Branch
defect
Not set

Tracking

(seamonkey2.39+ fixed, seamonkey2.40 affected, seamonkey2.41 fixed)

RESOLVED FIXED
seamonkey2.41
Tracking Status
seamonkey2.39 + fixed
seamonkey2.40 --- affected
seamonkey2.41 --- fixed

People

(Reporter: philip.chee, Assigned: neil)

References

Details

User Story

Change all callers in /suite/ of nsIPermissionManager::Remove() to use a URI instead of a string.

Also change call callers in /suite/ of nsIPermission::GetHost() to use the principal.

Attachments

(2 files)

No description provided.
User Story: (updated)
Depends on: 1173523, 817007, 1165263
User Story: (updated)
User Story: (updated)
Attached patch Proposed patchSplinter Review
This may fix some other bugs such as the Data Manager one, but right now these are just mechanical conversions so I may have some typos or omissions.
Attachment #8672112 - Flags: review?(philip.chee)
Attachment #8672112 - Flags: review?(iann_bugzilla)
Comment on attachment 8672112 [details] [diff] [review]
Proposed patch

> +++ b/suite/browser/pageinfo/pageInfo.js
 In Firefox I see:

 function onLoadPermission()
 .....
-    hostText.value = gPermURI.host;
+    hostText.value = gPermURI.prePath;

https://bugzilla.mozilla.org/show_bug.cgi?id=1173523#c24

> +++ b/suite/common/dataman/dataman.xml

> -              Services.perms.remove(this.host, this.type);
> +              Services.perms.remove(Services.io.newURI("http://" + this.host, null, null), this.type);
What happens to https?

> +++ b/suite/common/pref/pref-offlineapps.js

> -    let usage = _getOfflineAppUsage(perm.host);
> +    let usage = _getOfflineAppUsage(perm);
>      let row = document.createElement("listitem");
> -    row.setAttribute("host", perm.host);
> +    row.setAttribute("host", perm.principal.URI.host);

That reminds me, we should port Bug 1173523 - Part 7: Update advanced.js to use new API for nsIPermission
https://hg.mozilla.org/mozilla-central/rev/88daaf531d7c#l1.40
https://hg.mozilla.org/mozilla-central/rev/88daaf531d7c#l1.99
Attachment #8672112 - Flags: review?(philip.chee) → review+
See Also: → 1188348
> @@ -1225,21 +1225,21 @@ var gPerms = {
>      this.addHost = document.getElementById("permHost");
>      this.addType = document.getElementById("permType");
>      this.addButton = document.getElementById("permAddButton");
 
>      let enumerator = Services.perms.enumerator;
>      while (enumerator.hasMoreElements()) {
>        let nextPermission = enumerator.getNext();
>        nextPermission = nextPermission.QueryInterface(Components.interfaces.nsIPermission);
> -      let rawHost = nextPermission.host.replace(/^\./, "");
> +      let rawHost = nextPermission.principal.URI.host.replace(/^\./, "");
>        if (gDomains.hostMatchesSelected(rawHost)) {
>          let permElem = document.createElement("richlistitem");
>          permElem.setAttribute("type", nextPermission.type);


> -        permElem.setAttribute("host", nextPermission.host);
> +        permElem.setAttribute("host", nextPermission.principal.URI.host);
Frank-Rainer Grahl has this change in his patch over at Bug 1188348
Assignee: nobody → neil
Status: NEW → ASSIGNED
Neil,

could you look over my patch in Bug 1188348 and integrate it in yours. It also covers the old cookieviewer. 

I did the remove in dataviewer.js a little differently. The removeFromPrincipal seemed to be looking a little better in my eyes.

Your missed one host assignment in yours:  

>> -        permElem.setAttribute("host", nextPermission.host);
>> +        permElem.setAttribute("host", nextPermission.principal.URI.host);

FRG
Comment on attachment 8672112 [details] [diff] [review]
Proposed patch

>+++ b/suite/common/dataman/dataman.js
>@@ -1225,17 +1225,17 @@ var gPerms = {
>     while (enumerator.hasMoreElements()) {
>       let nextPermission = enumerator.getNext();
>       nextPermission = nextPermission.QueryInterface(Components.interfaces.nsIPermission);
>-      let rawHost = nextPermission.host.replace(/^\./, "");
>+      let rawHost = nextPermission.principal.URI.host.replace(/^\./, "");
>       if (gDomains.hostMatchesSelected(rawHost)) {
>         let permElem = document.createElement("richlistitem");
>         permElem.setAttribute("type", nextPermission.type);
>         permElem.setAttribute("host", nextPermission.host);
As mentioned by FRG, this will need fixing.

As mentioned by Ratty:
That reminds me, we should port Bug 1173523 - Part 7: Update advanced.js to use new API for nsIPermission
https://hg.mozilla.org/mozilla-central/rev/88daaf531d7c#l1.40
https://hg.mozilla.org/mozilla-central/rev/88daaf531d7c#l1.99

f+ as I would like to see the new patch.
Attachment #8672112 - Flags: review?(iann_bugzilla) → feedback+
(In reply to Ian Neal from comment #5)
> As mentioned by FRG, this will need fixing.
I won't land the datamanager changes; neither set is ready anyway.

> As mentioned by Ratty:
> That reminds me, we should port Bug 1173523 - Part 7: Update advanced.js to
> use new API for nsIPermission
We need to implement the nsILoadContextInfo changes first anyway. I think it would be better to land and uplift this work done so far, and then fix the other bustage.
Pushed comm-central changeset 553176734705.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.41
Comment on attachment 8672112 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Regression caused by (bug #): 1170200
User impact if declined: Various functions broken
String changes made by this patch: None
Attachment #8672112 - Flags: approval-comm-release?
Attachment #8672112 - Flags: approval-comm-beta?
Attachment #8672112 - Flags: approval-comm-aurora?
Attachment #8672112 - Flags: approval-comm-release?
Attachment #8672112 - Flags: approval-comm-release+
Attachment #8672112 - Flags: approval-comm-beta?
Attachment #8672112 - Flags: approval-comm-beta+
Attachment #8672112 - Flags: approval-comm-aurora?
Attachment #8672112 - Flags: approval-comm-aurora+
From the recent merge, c-a is not needed since it's been uplifted from
c-c.  So it's only c-b and c-r that needs the patch.
[Triage Comment]

This was for the c-r checkin. (Removed the dataman code).
Attachment #8682396 - Flags: approval-comm-release+
Blocks: 1232855
You need to log in before you can comment on or make changes to this bug.