1647486 - Remove Blob URL screenshot support from Activity Stream code

Status: NEW

(Firefox :: New Tab Page, task, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Blobs and Blob URLs were originally used for screenshots to avoid sending huge data URLs over IPC. Now that moz-page-thumb support exists in the privileged about content process, we've been using that, and the old Blob and Blob URL support is probably not needed anymore.

This bug is for removing that stuff, which would probably simplify a few things.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Mardak,

I'm trying to eliminate a flash where a TopSite from a cached about:home starts with a screenshot, then transitions to having no screenshot, and then back to having a screenshot again. It seems like the flash is coming from here:

https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/browser/components/newtab/content-src/components/TopSites/TopSite.jsx#113

which calls this function:

https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/browser/components/newtab/content-src/components/TopSites/TopSite.jsx#92-99

And since the state for the cached about:home document doesn't have a screenshotImage, I think it sets it to null (which is the default set in the constructor), and then sets it again to the proper "local image" object here: https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/browser/components/newtab/content-src/components/TopSites/TopSite.jsx#104-106

So, a few things:

1. Now that we don't use Blob URLs for thumbnails, are there parts here that can be simplified?
2. What's the best way for a TopSite link to default to using a non-null screenshotImage when it's being restored from the startup cache?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Okay, I've dug into this more, and I don't actually think the Blob URL stuff is what's causing the flash. It would probably be good to get rid of the Blob URL support at some point to reduce complexity, anyhow.

Comment 3

[Ed Lee :Mardak]

Bug 1436615 added blobs for and bug 1467485 handled blob state changes and those all should be removable with bug 1184701 assuming privileged content process. Yeah a lot of this screenshot stuff could be simplified when browser.tabs.remote.separatePrivilegedContentProcess goes away / is always true.

Comment 4

[Hector Zhao [:hectorz]]

Hi, Hector from Beijing office here.

In case you're not aware, we're shipping a web hosted fork of activity-stream with a privileged signed extension as the newtab experience in Fx China repack.

We're actually flipping the browser.tabs.remote.separatePrivilegedContentProcess pref to keep the screenshots working in our fork. I guess I'm not always using the code as originally intended, but would you please give me some advice on how best to mitigate this removal, other than moving all those logic into our extension?

Thanks!

Comment 5

[Hector Zhao [:hectorz]]

(In reply to Hector Zhao [:hectorz] from comment #4)

We're actually flipping the browser.tabs.remote.separatePrivilegedContentProcess pref [3] to keep the screenshots working in our fork. I guess I'm not always using the code as originally intended, but would you please give me some advice on how best to mitigate this removal, other than moving all those logic into our extension?

With bug 1513680 landed in Fx 86, we can no longer flip the pref. I'm working on a fix/workaround in our extension.