1235805 - 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 (

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Philip Chee]

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

Comment 1

[Philip Chee]

Attachment: Patch v1 proposed fix.

Untested patch. May contain typo's

Comment 2

[Philip Chee]

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

Looks like it's working.

Comment 3

[Stefan [:stefanh]]

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

f+ because I don't crash @ SetAndFetchFaviconForPage with my trunk debug build anymore.

Comment 4

[[e-mail]]

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)

Comment 5

[Philip Chee]

Attachment: 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

Comment 6

[Frank-Rainer Grahl (:frg)]

Attachment: 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.

Comment 7

[Philip Chee]

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

r=me thanks.

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: 1235805-channel2-mailnews-V2.patch

I goofed. Ci. was undefined. 

Review+ from Philip Chee carried forward.

Comment 9

[Ian Neal]

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

r=me though Neil needs crediting too

Comment 10

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/273839906290

Comment 11

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/a06de81ee211