Rollup patch: Add preload content policy types for images (Bug 1048048) Use the loading document's principal to populate loadInfo for Favicons instead of using systemPrincipal (Bug 1119386) nsITaskbarPreview::Invalidate no longer throws when not visible (

RESOLVED FIXED in seamonkey2.45

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.45

SeaMonkey Tracking Flags

(seamonkey2.46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Bug to port the following to SeaMonkey:
Bug 1048048 - add preload content policy types for images
Bug 1119386 - Use the loading document's principal to populate loadInfo for Favicons instead of using systemPrincipal

Bug 1127577 - nsITaskbarPreview::Invalidate no longer throws when not visible
Bug 1087726: Make JS callers of ios.newChannel call ios.newChannel2
(Assignee)

Comment 1

2 years ago
Created attachment 8702902 [details] [diff] [review]
Patch v1 proposed fix.

Untested patch. May contain typo's
Attachment #8702902 - Flags: feedback?(stefanh)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8702902 [details] [diff] [review]
Patch v1 proposed fix.

Looks like it's working.
Attachment #8702902 - Flags: review?(neil)

Comment 3

2 years ago
Comment on attachment 8702902 [details] [diff] [review]
Patch v1 proposed fix.

f+ because I don't crash @ SetAndFetchFaviconForPage with my trunk debug build anymore.
Attachment #8702902 - Flags: feedback?(stefanh) → feedback+

Comment 4

2 years ago
Comment on attachment 8702902 [details] [diff] [review]
Patch v1 proposed fix.

>-                  this.mTabBrowser.setIcon(this.mTab, "");
>+                  this.mTabBrowser.setIcon(this.mTab, "", null);
[We don't use aLoadingPrincipal if aURI is empty, so strictly speaking, this change is unnecessary. Then again, the "" is also no longer necessary, but it just didn't get removed when setIcon was updated.]

>+              var loadingPrincipal = aLoadingPrincipal ||
>+                                     this.mSecMan.getSystemPrincipal();
[I guess this is for backwards compatibility if someone else calls setIcon with only two parameters?]

>+               this.setIcon(this.tabs[index], iconUri,
>+                            targetDoc.nodePrincipal);
link.nodePrincipal perhaps?

>+    this._faviconService.setAndFetchFaviconForPage(
>+      readerURI, faviconURI, false, flags,
>       function(aURI, aDataLen, aData, aMimeType) {
>         if (aDataLen > 0) {
>           let dataURL = "data:" + aMimeType + ";base64," +
>                         btoa(String.fromCharCode.apply(null, aData));
>           aMenuItem.setAttribute("image", dataURL);
>         }
>-      });
>+      }, nullPrincipal);
Why not this._document.nodePrincipal?

>+                  nsIContentPolicy.TYPE_INTERNAL_IMAGE
>+                );
On previous line please.

>+      });
[But not this one because it's a }.]

>+      aBrowser.contentWindow.document,
Nit: contentDocument (also somewhere else I forget where)
(Assignee)

Comment 5

2 years ago
Created attachment 8724108 [details] [diff] [review]
Patch v2.0 fix review issues.

>>-                  this.mTabBrowser.setIcon(this.mTab, "");
>>+                  this.mTabBrowser.setIcon(this.mTab, "", null);
> [We don't use aLoadingPrincipal if aURI is empty, so strictly speaking, 
> this change is unnecessary. Then again, the "" is also no longer
> necessary, but it just didn't get removed when setIcon was updated.]
Fixed

>>+              var loadingPrincipal = aLoadingPrincipal ||
>>+                                     this.mSecMan.getSystemPrincipal();
> [I guess this is for backwards compatibility if someone else calls
> setIcon with only two parameters?]
That's one reason. The other is:
This is from the Firefox tabbrowser:
(Our SessionStore.jsm doesn't do that yet)
> // We do not serialize the principal from within SessionStore.jsm,
> // hence if aLoadingPrincipal is null we default to the
> // systemPrincipal which will allow the favicon to load.

>>+               this.setIcon(this.tabs[index], iconUri,
>>+                            targetDoc.nodePrincipal);
> link.nodePrincipal perhaps?
Fixed.

>>+    this._faviconService.setAndFetchFaviconForPage(
>>+      readerURI, faviconURI, false, flags,
>>       function(aURI, aDataLen, aData, aMimeType) {
>>         if (aDataLen > 0) {
>>           let dataURL = "data:" + aMimeType + ";base64," +
>>                         btoa(String.fromCharCode.apply(null, aData));
>>           aMenuItem.setAttribute("image", dataURL);
>>         }
>>-      });
>>+      }, nullPrincipal);
> Why not this._document.nodePrincipal?

From Bug 1119386:
> Marco pointed out that this is not going to work correctly; we're the people 
> asking for this icon, not the feed, as it's the icon of the feed reader. 
> There should now be a separate bug on file to just rip out this code.
Also:
> Mak, I forgot to mention that since we are going to eliminate code within
>> browser/components/feeds/FeedWriter.js
> we could use the nullPrincipal as an interim solution.

>>+                  nsIContentPolicy.TYPE_INTERNAL_IMAGE
>>+                );
> On previous line please.
Fixed.

>>+      });
> [But not this one because it's a }.]
Fixed x2

>>+      aBrowser.contentWindow.document,
> Nit: contentDocument (also somewhere else I forget where)
Fixed x2
Attachment #8702902 - Attachment is obsolete: true
Attachment #8702902 - Flags: review?(neil)
Attachment #8724108 - Flags: review?(neil)
Created attachment 8730119 [details] [diff] [review]
1235805-channel2-mailnews.patch

Philip,

the now removed newChannelFromURI is also used in mailnews. You might want to integrate this one into your patch. It's untested.
Attachment #8730119 - Flags: feedback?(philip.chee)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8730119 [details] [diff] [review]
1235805-channel2-mailnews.patch

r=me thanks.
Attachment #8730119 - Flags: feedback?(philip.chee) → review+
Created attachment 8730644 [details] [diff] [review]
1235805-channel2-mailnews-V2.patch

I goofed. Ci. was undefined. 

Review+ from Philip Chee carried forward.
Attachment #8730644 - Flags: review+
Attachment #8730119 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8724108 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8730644 - Flags: review+

Comment 9

2 years ago
Comment on attachment 8724108 [details] [diff] [review]
Patch v2.0 fix review issues.

r=me though Neil needs crediting too
Attachment #8724108 - Flags: review?(iann_bugzilla) → review+

Updated

2 years ago
Component: General → Tabbed Browser
(Assignee)

Comment 10

2 years ago
http://hg.mozilla.org/comm-central/rev/273839906290
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → ---
status-seamonkey2.46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.45
Attachment #8724108 - Flags: review?(neil)
You need to log in before you can comment on or make changes to this bug.