Closed Bug 1235805 Opened 4 years ago Closed 4 years ago

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 (

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set

Tracking

(seamonkey2.46 fixed)

RESOLVED FIXED
seamonkey2.45
Tracking Status
seamonkey2.46 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

Attachments

(2 files, 2 obsolete files)

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
Attached patch Patch v1 proposed fix. (obsolete) — Splinter Review
Untested patch. May contain typo's
Attachment #8702902 - Flags: feedback?(stefanh)
Comment on attachment 8702902 [details] [diff] [review]
Patch v1 proposed fix.

Looks like it's working.
Attachment #8702902 - Flags: review?(neil)
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 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)
>>-                  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)
Attached patch 1235805-channel2-mailnews.patch (obsolete) — Splinter Review
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)
Comment on attachment 8730119 [details] [diff] [review]
1235805-channel2-mailnews.patch

r=me thanks.
Attachment #8730119 - Flags: feedback?(philip.chee) → review+
I goofed. Ci. was undefined. 

Review+ from Philip Chee carried forward.
Attachment #8730644 - Flags: review+
Attachment #8730119 - Attachment is obsolete: true
Attachment #8724108 - Flags: review?(iann_bugzilla)
Attachment #8730644 - Flags: review+
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+
Component: General → Tabbed Browser
http://hg.mozilla.org/comm-central/rev/273839906290
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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.