Use BackgroundPageThumbs to capture user top sites that we don't currently capture

RESOLVED FIXED in Firefox 25

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: adw, Assigned: markh)

Tracking

(Depends on 4 bugs, {feature})

Trunk
Firefox 25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 disabled)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
We should determine when we want to use BackgroundPageThumbs and then do it.  There's the larger question, When do we want to capture thumbnails?  And a specific question with that larger one, When do we want to use BackgroundPageThumbs vs. PageThumbs?
(Reporter)

Updated

6 years ago
Summary: Formulate and implement policy for using BackgroundPageThumbs → Formulate and implement policy for using BackgroundPageThumbs in desktop Firefox and/or Toolkit
(In reply to Drew Willcoxon :adw from comment #0)
> We should determine when we want to use BackgroundPageThumbs and then do it.
> There's the larger question, When do we want to capture thumbnails?  And a
> specific question with that larger one, When do we want to use
> BackgroundPageThumbs vs. PageThumbs?

While a larger policy may be needed in time, can we begin by capturing the sites with the highest frecency score - say, the top 50?  This is particularly relevant for New Tab (Bug 756881).
(In reply to Drew Willcoxon :adw from comment #0)
> We should determine when we want to use BackgroundPageThumbs and then do it.
> There's the larger question, When do we want to capture thumbnails?  And a
> specific question with that larger one, When do we want to use
> BackgroundPageThumbs vs. PageThumbs?

Adw, what kind of policy is needed here?  I've pinged some usual suspects in privacy & product and am being met with shrugs mostly.  If this is the bug blocking thumbnails from displaying, it's P1 easily (as I just marked).  How do we move this forward?
Flags: needinfo?(adw)
Priority: -- → P1
(Reporter)

Comment 3

6 years ago
We (Gavin, Tim, Mark, and I) just haven't sat down yet and decided how/when we want to use the background service.  That's all I meant by "policy."  I didn't mean to imply any formal security, privacy, or product review.  (Although someone flagged the original bug 841495 for security review.  I don't know what that's about.)

There are several bugs I filed blocking the original that need to be fixed before the service is suitable for production use.  We can decide how/when we want to use it at the same time we're fixing those bugs.

Gavin wants me to set up a meeting between the four of us to talk about it, so I need to do that soon.  Would you like to be there?

(In reply to Drew Willcoxon :adw from comment #0)
> And a specific question with that larger one, When do we want to use
> BackgroundPageThumbs vs. PageThumbs?

Oops, I meant "within," not "with."
Flags: needinfo?(adw)
(In reply to Drew Willcoxon :adw from comment #3)
> We (Gavin, Tim, Mark, and I) just haven't sat down yet and decided how/when
> we want to use the background service.  That's all I meant by "policy."  I
> didn't mean to imply any formal security, privacy, or product review. 
> (Although someone flagged the original bug 841495 for security review.  I
> don't know what that's about.)
> 
> There are several bugs I filed blocking the original that need to be fixed
> before the service is suitable for production use.  We can decide how/when
> we want to use it at the same time we're fixing those bugs.
> 
> Gavin wants me to set up a meeting between the four of us to talk about it,
> so I need to do that soon.  Would you like to be there?
> 
> (In reply to Drew Willcoxon :adw from comment #0)
> > And a specific question with that larger one, When do we want to use
> > BackgroundPageThumbs vs. PageThumbs?
> 
> Oops, I meant "within," not "with."

I'd love to be, thanks!  Please feel free to add me to a meeting via Zimbra.
(Reporter)

Comment 5

6 years ago
Boriss, Mark, Gavin and I met today, and we decided that there are a few reasonable ways to start using the service.  In order from smallest rollout to biggest:

(1) Use it for the user's top sites that we currently decline to capture (due to cache-control: no-store (any other reasons?))
(2) Use it for all sites we currently decline to capture
(3) Use it for all sites

We decided to start with 1, so I'm morphing the bug.  The point of restricting ourselves to top sites is to start small, gather data, see how it performs, and then go from there.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Summary: Formulate and implement policy for using BackgroundPageThumbs in desktop Firefox and/or Toolkit → Use BackgroundPageThumbs to capture user top sites that we don't currently capture
(Assignee)

Comment 6

6 years ago
I noticed that my laptop was missing almost all thumbnails - the problem there is I use Sync (so the top-sites are sync'd from my desktop) but as the laptop has never visited those sites the thumbnails are missing.  Same issue (I assume) on my phone.

Can we subtly change this bug into "Use BackgroundPageThumbs to capture user top sites with missing thumbnails"?  That way we kill multiple birds with one stone.
(In reply to Mark Hammond (:markh) from comment #6)
> Can we subtly change this bug into "Use BackgroundPageThumbs to capture user
> top sites with missing thumbnails"?  That way we kill multiple birds with
> one stone.

We can make that be step 2.5 (in the steps from comment 5), I suppose (or maybe just a replacement for step 3). It's a bit more complicated because it requires us deciding on a time to make those requests (do we just fire off these requests on startup?). I think we should keep this bug scoped to step 1 (and maybe step 2).
(Assignee)

Comment 8

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> (In reply to Mark Hammond (:markh) from comment #6)
> > Can we subtly change this bug into "Use BackgroundPageThumbs to capture user
> > top sites with missing thumbnails"?  That way we kill multiple birds with
> > one stone.
> 
> We can make that be step 2.5 (in the steps from comment 5), I suppose (or
> maybe just a replacement for step 3). It's a bit more complicated because it
> requires us deciding on a time to make those requests (do we just fire off
> these requests on startup?). I think we should keep this bug scoped to step
> 1 (and maybe step 2).

I guess it depends on how you approach it :)  Step 2 means we still need to decide when to examine the user's top sites to determine which ones are missing (do we fire that off on startup?) and we still might end up forcing thumbnails for sites we aren't actually ever going to use on about:newtab.

A patch is worth a thousand words - so this patch is what I was thinking:

* As about:newtab initializes, it adds an observer for a new notification about a thumbnail having been captured.  When it gets this notification it refreshes the displayed thumbnails.

* As about:newtab decides it wants to display the thumbnail for a site, it requests that the "freshness" of the thumbnail be checked - but still goes ahead and displays whatever thumbnail is currently available, if any.

* This freshness-checking function checks the modified time of the thumbnail file in the worker thread.  If it turns out the thumbnail doesn't exist or isn't fresh enough, it uses the b/g thumbnail capturer to make a new thumbnail.

* When this finally completes and after the thumbnail is written to disk, a notification is fired - which about:newtab notices and updates the page accordingly (see first bullet point).

In practice, this means you can clobber *all* your thumbnails, then open about:newtab and watch as thumbnails magically appear at the rate of about 1 every 1-2 seconds.  If a thumbnail is > 2 days old, you can also notice it update.  This happens without visiting the sites.

The patch is clearly just a WIP to help me understand how everything hangs together and to see if my idea was viable.  Obvious improvements would include:

* Have the b/g capture process only actually process when the idle service says we are idle.
* Have about:newtab only refresh the thumbnail that completes - currently it just updates the entire page every time a thumbnail appears (but this is actually quite seamless)
* Tests, etc.

Key things I like about this approach are that the patch is relatively small:

 toolkit/components/thumbnails/PageThumbs.jsm      |  28 +++++++++++++++++++++-
 toolkit/components/thumbnails/PageThumbsWorker.js |  14 +++++++++++
 toolkit/modules/NewTabUtils.jsm                   |  27 +++++++++++++++-----
 3 files changed, 61 insertions(+), 8 deletions(-)

and it doesn't need to make it's own decision about which top-sites need doing, doesn't need to know that a site failed to capture for any reason, and the impact on consumers of thumbnails is a single API function and single observer topic.

I hope I'm not stepping on Drew's toes here - it really was a fairly quick experiment to demonstrate what I had in mind.  I'll understand if you guys don't like the approach.
(Reporter)

Comment 9

6 years ago
Comment on attachment 763333 [details] [diff] [review]
Update state or missing thumbnail as they are requested.

This is great.

checkThumbnailForFreshness is interesting.  I'd call it captureIfStale or captureIfExpired, though.  (Although PageThumbs already uses the latter term elsewhere.)  I think it makes total sense to build the notion of thumbnail freshness into the service rather than making consumers keep track of that.  You could take it one step further.  Presumably every consumer wants to show an up-to-date thumbnail, at least eventually after immediately displaying whatever thumbnail is cached, right?  So instead of making every consumer call an update function as a matter of habit, why not make the mere act of requesting a thumbnail trigger a recapture using the background service if necessary?

However, and this is a tangent since it pertains to a detail and not the overall picture, I think we should use a response's cache-related headers when determining how fresh its thumbnail is, as described in HTTP 1.1: http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html

The problem with choosing a blanket, arbitrary freshness window is that it would unnecessarily fit many sites poorly.  Two days for a news site is a poor fit, for example.  It's also a poor fit for rarely updated sites.

Presumably a server uses cache headers to convey how fresh its content is, which is what we're trying to divine here.  Thumbnails are a kind of cache anyway.  We'd have to be careful to avoid infinite loops when thumbnails are set to expire immediately, though.  If I request a thumbnail, the service determines it's expired, fetches it, I'm notified, I ask for the thumbnail again, the service determines it's expired, ...

And I think the about:newtab change makes sense in this context.  All thumbnail consumers would work the same way.

> I hope I'm not stepping on Drew's toes here - it really was a fairly quick
> experiment to demonstrate what I had in mind.

Not at all!  This is better than what I probably would have come up with.
Attachment #763333 - Flags: feedback+
(Assignee)

Comment 10

6 years ago
(In reply to Drew Willcoxon :adw from comment #9)
> Comment on attachment 763333 [details] [diff] [review]
> Update state or missing thumbnail as they are requested.
> 
> This is great.

awesome :)

> So instead of making every consumer
> call an update function as a matter of habit, why not make the mere act of
> requesting a thumbnail trigger a recapture using the background service if
> necessary?

Sounds great!

> However, and this is a tangent since it pertains to a detail and not the
> overall picture, I think we should use a response's cache-related headers
> when determining how fresh its thumbnail is, as described in HTTP 1.1:
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html

This might end up tricky - the cache control headers are per request, whereas this is per "page" - IOW, the response for the top-level request might have a very long ttl, whereas sub-requests are very short (or vice-versa).  It's not clear that using the cache-control headers for that initial request is a suitable indication of the cacheability of the page as a whole.

Even ignoring that, what would the policy be for pages which imply they should not be cached?  Visiting google.com.au serves me the response header "Cache-Control:	private, max-age=0" (and no last-modified/etag) - which would imply we'd fire off a re-fetch request every time.  Of my personal top-two news sites, one has no last-modified or etag and no cache-control at all, which IIUC is effectively the same as google, while the other indicates the request is fresh for 2 minutes.  Even mxr.mozilla.org suggests its landing page should be revalidated every hour.  Finally, it's not uncommon for pages to include an etag/date-modified but a max-age of 0, which will (IIUC) generally cause the browser to perform a conditional request (and ideally get a 304, so the request is made but no response body is sent)

So my concern here is that the vast majority of sites that end up on about:newtab will imply we should re-fetch everytime we need the thumbnail.  If we decide that's not really such a big deal, then we could almost make the case to just do this for *every* thumbnail and avoid the additional complexity (eg, we don't currently have anywhere to store that expiry date.)  And if we do add "idle" detection to the queue, maybe that's quite reasonable (we can rely on the browser to cache whatever it can in those requests anyway...)

> The problem with choosing a blanket, arbitrary freshness window is that it
> would unnecessarily fit many sites poorly.  Two days for a news site is a
> poor fit, for example.  It's also a poor fit for rarely updated sites.

OTOH, I'd expect the browser cache would mean that capturing rarely updated sites isn't very expensive in terms of network.

> Presumably a server uses cache headers to convey how fresh its content is,
> which is what we're trying to divine here.  Thumbnails are a kind of cache
> anyway.  We'd have to be careful to avoid infinite loops when thumbnails are
> set to expire immediately, though.  If I request a thumbnail, the service
> determines it's expired, fetches it, I'm notified, I ask for the thumbnail
> again, the service determines it's expired, ...

Maybe just a blanket (say) 30 minutes would be a reasonable first step, especially if it does transpire that most pages that end up on about:home have aggressive "don't cache me" semantics (which seems quite likely to me).  If the requests for new thumbnails are only fired (a) when a new tab page is opened, (b) on idle and (c) when no other request for the same URL is queued, then it might be fine for a first cut?  Especially if we roll some telemetry in so we can start to measure the actual impact...

Thanks for the feedback - and I'm sure Gavin will also offer his thoughts when he has a few spare minutes...
(Assignee)

Comment 11

6 years ago
Chatting with Gavin about this, we agreed that a goal of this must be that we don't impact the performance of Fx when doing these captures.  For example, a worst-case scenario would be that we queue 9 thumbnail requests as soon as the user opens about:newtab - but the user then immediately selects one of the sites and is trying to interact with it just as we start doing these captures...

One way to mitigate this would be to (a) only start processing the queue when the user goes idle for some time (or investigate if we can set the priority low for the OS-process used by the capture) and (b) to ensure that the capture has a low network priority.

The fly in this ointment is that the "capture timeout" for the background queue starts immediately the item is placed in the queue, rather than as the queued item is processed.  That doesn't marry well with a queue that's only processed at a very low priority.

Drew - any insights or thoughts on this?  (eg, can we just change the timeout semantics or should we add a different "timeout" concept, etc)?  FWIW, I'm not going to have much time to do any real work on this before leaving for the work-week, so feel free to take your time on responding - we can always just chat about it in person next week...
Assignee: adw → mhammond
Flags: needinfo?(adw)
(Reporter)

Comment 12

6 years ago
(In reply to Mark Hammond (:markh) from comment #11)
> Chatting with Gavin about this, we agreed that a goal of this must be that
> we don't impact the performance of Fx when doing these captures.  For
> example, a worst-case scenario would be that we queue 9 thumbnail requests
> as soon as the user opens about:newtab - but the user then immediately
> selects one of the sites and is trying to interact with it just as we start
> doing these captures...

That's a proper goal, and I know we're just thinking out loud here, but what's the assumption represented by that ellipsis?

Unless we're talking about asymptotic complexity or something, we shouldn't just assume things about performance or responsiveness.  If we're worried about impacts to performance or responsiveness, then we should come up with some reasonable metrics by which they can be measured and then measure them.  A rough first metric would be, do I as a person sitting in front of the browser actually notice any problems during the scenario above?

> (or investigate if we can set the priority low for the OS-process used by
> the capture) and (b) to ensure that the capture has a low network priority.

These are definitely reasonable and probably worth doing even outside a context of trying to fix a specific performance problem.  Again though, let's think of ways to solve problems only after establishing that they actually exist.

> (eg, can we just change the timeout semantics or should we add a different
> "timeout" concept, etc)?

Sure, we can change the semantics, no problem.
Flags: needinfo?(adw)
(Assignee)

Comment 13

6 years ago
(In reply to Drew Willcoxon :adw from comment #12)

> That's a proper goal, and I know we're just thinking out loud here, but
> what's the assumption represented by that ellipsis?

It's just a feeling that *something* more should be said, but a complete lack of insight into what that is :)

> A rough first metric would be, do I as a person sitting in front of the
> browser actually notice any problems during the scenario above?
...
> These are definitely reasonable and probably worth doing even outside a
> context of trying to fix a specific performance problem.  Again though,
> let's think of ways to solve problems only after establishing that they
> actually exist.

Agreed - but I'm not sure how to measure this.  I'm confident that on my desktop machine I wouldn't notice background thumbnails even if we took no action in this regard.  On the other hand, a user running Windows XP on a memory-starved single-core machine and a relatively slow connection would probably have a significantly different experience.

Do you have any thoughts about what we should measure to meet this goal?  It seems the perfect answer would be to see how various performance metrics we already capture are impacted while a capture is ongoing, but I'm aware our telemetry infrastructure is setup for that.
(Assignee)

Comment 14

6 years ago
(In reply to Mark Hammond (:markh) from comment #13)
> but I'm aware our telemetry infrastructure is setup for that.

but I'm *not* aware our telemetry infrastructure is setup for that.
(Reporter)

Comment 15

6 years ago
Maybe we could run the Talos tests with queues of thumbnail captures to see how they're affected.  That seems like a good place to start.
(Assignee)

Comment 16

6 years ago
Taras seems the right person to loop in for feedback - see comment 11 and later...
(Assignee)

Comment 17

6 years ago
TIL that navigator.connection.metered is a thing (or more accurately - might one day be a useful thing in this context) - https://developer.mozilla.org/en-US/docs/Web/API/window.navigator.connection.metered - might be worth considering if we should factor that into when we decide it's OK to take these background snapshots.
(Assignee)

Comment 18

6 years ago
Joel has kindly offered to help us with (ab)using talos here...
(Assignee)

Updated

6 years ago
Depends on: 890133
(Assignee)

Updated

6 years ago
Depends on: 890130
(Assignee)

Updated

6 years ago
Depends on: 891640
(Assignee)

Comment 19

6 years ago
FTR, the talos experiments show that we suffer about 7-8% in Tp5 when thumbnails are being captured for the entire test run.  Given we will not be re-fetching thumbnails all the time (ie, worst-case is that we will queue up 9 requests as about:newtab is displayed), and that real-world network latency should mean even lower perceived slowness, we feel this is low enough to check it in on nightly with a conservative "staleness" value and see what feedback we get.
(Assignee)

Comment 20

6 years ago
This patch is similar to the previous one, but as suggested by Drew, the "staleness" value is baked into the service itself, and all requests for a page's thumbnail kicks off a freshness check.  There's also a test for the thumbnail side of the world.

Requesting feedback from Tim, mainly for the about:newtab changes.  This patch tries to only update individual thumbnails that have changed rather than causing a refresh of the entire about:newtab page.  This refresh is done simply by setting the backgroundImage - to the exact same value it was before!  This change is enough to cause a new copy of the thumbnail to be displayed.  It's not clear to me that this micro-optimization is worthwhile - the patch to newtab/ becomes quite a bit smaller if we just call gPage.update() as *any* new thumbnail comes in.  There's also no newtab/ specific test.
Attachment #763333 - Attachment is obsolete: true
Attachment #773126 - Flags: feedback?(ttaubert)
(Assignee)

Comment 21

6 years ago
This patch is to the thumbnails component.  It re-generates thumbnails on demand and includes a new test.  This is the same basic patch as previously, just split for easier review.
Attachment #773126 - Attachment is obsolete: true
Attachment #773126 - Flags: feedback?(ttaubert)
Attachment #773836 - Flags: review?(adw)
(Assignee)

Comment 22

6 years ago
This is part 2 and only touches about:newtab.  It is very similar to the last patch, although (a) split for easier review and (b) try tells me that the new loop in site.js could end up with a null site - so the loop now looks like:

        for (let site of gGrid.sites) {
          if (site && site.url === aData) {
            site.refreshThumbnail();

Note the check of |site| in the middle line.  I'd also still like feedback on whether this should just be |this.update()|
Attachment #773837 - Flags: review?(ttaubert)
Comment on attachment 773836 [details] [diff] [review]
part 1 - regenerate stale page thumbnails on demand, and send notification when done.

Review of attachment 773836 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +19,5 @@
>  
> +// If a request for a thumbnail comes in and we find one that is "stale"
> +// (or don't find one at all) we automatically queue a request to generate a
> +// new one.
> +const MAX_THUMBNAIL_AGE = 172800; // 60 * 60 * 24 * 2 == 172800

MAX_THUMBNAIL_AGE_SECS would be a better name to signal that it's a number of seconds. Please also add a comment that mentions the timespan in words.

@@ +183,5 @@
>    getThumbnailURL: function PageThumbs_getThumbnailURL(aUrl) {
> +    // Kick off an async task to check the freshness and possibly generate
> +    // a new one.
> +    let filePath = PageThumbsStorage.getFilePathForURL(aUrl);
> +    PageThumbsWorker.post("isFileRecent", [filePath, MAX_THUMBNAIL_AGE]

I don't think that PageThumbs.getThumbnailURL() should cause a background capture. We should be doing this in the PageThumbsProtocol when the image is actually being requested. Proto.newChannel() could send an "isFileRecent" message.
Comment on attachment 773837 [details] [diff] [review]
part 2 - have about:newtab update thumbnails when it observes the notification from the thumbnail component.

Review of attachment 773837 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/page.js
@@ +44,5 @@
> +        this._init();
> +      } else {
> +        gUndoDialog.hide();
> +      }
> +      return;

Please don't return here, we can use 'else if'

@@ +49,3 @@
>      }
> +    if (aTopic == "page-thumbnail:create") {
> +      if (gGrid.ready) {

We can include 'gGrid.ready' in the condition above.

@@ +54,5 @@
> +            site.refreshThumbnail();
> +          }
> +        }
> +      }
> +      return;

(see above)

::: browser/base/content/newtab/sites.js
@@ +130,5 @@
>      this._querySelector(".newtab-title").textContent = title;
>  
>      if (this.isPinned())
>        this._updateAttributes(true);
> +    this.refreshThumbnail();

I wonder, does this actually work to refresh the thumbnail? We actually don't change the .style.backgroundImage string so does this really request the image again? If not just clearing the value before that would probably do.
Attachment #773837 - Flags: review?(ttaubert) → review+
(In reply to Mark Hammond (:markh) from comment #22)
> I'd also still like feedback
> on whether this should just be |this.update()|

This is definitely much better than a brute-force update of the whole page. +1 for selective updates :)
(Reporter)

Comment 26

6 years ago
Comment on attachment 773836 [details] [diff] [review]
part 1 - regenerate stale page thumbnails on demand, and send notification when done.

Review of attachment 773836 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Tim, especially about doing the background request in the protocol handler, like I suggested earlier.

::: toolkit/components/thumbnails/test/browser_thumbnails_update.js
@@ +6,5 @@
> + */
> +
> +let numNotifications = 0;
> +
> +function observe() {

Please verify that the data is the expected URL.
Attachment #773836 - Flags: review?(adw) → review-
(Assignee)

Comment 27

6 years ago
Drew and I were discussing this over IRC.  A problem with the "implicitly check if the thumbnail is stale" approach is that it risks becoming circular.  ie:

1) about:newtab fetches thumbnail, causing "staleness" check to be triggered.
2) thumbnail is updated, causing observer notification, causing about:newtab to fetch thumbnail.
3) goto (1)

The saving grace is that the loop should execute fast enough that the thumbnail is never stale the second time around.  There's also probably an implementation accident saving us - we send the notification while the URL is still in the queue, so we would probably refuse to re-queue that URL again while the notification is being sent.

FWIW, this problem exists both with my patch and with the requested change that the protocol handler trigger the check.  The key difference is that with my patch it's *possible* to avoid it, but its impossible to avoid if the protocol handler does it.  But that's cold comfort and fragile - the patch as it stands does have this issue.

I see 2 options:

* Formalize this "implementation accident" in a test and rely on it.  Then hope code doesn't get refactored so the observer notification just causes an async refresh, which would cause the problem to return.

* Move right away from an implicit check - move back to an explicit method to check the freshness.  This would then make it much more obvious that if you are explicitly calling this method in response to a notification the thumbnail has changed then you are creating a cycle.
(Assignee)

Updated

6 years ago
Depends on: 892875
(Reporter)

Comment 28

6 years ago
(In reply to Mark Hammond (:markh) from comment #27)
> * Move right away from an implicit check - move back to an explicit method
> to check the freshness.  This would then make it much more obvious that if
> you are explicitly calling this method in response to a notification the
> thumbnail has changed then you are creating a cycle.

Let's do that then.  We can implement a cleverer mechanism later if we think of one.
(Assignee)

Comment 29

6 years ago
Based on earlier feedback from Drew, the new "explicit" method is called captureIfStale() - there is now no implicit staleness checking.  Also implemented the feedback from Tim.
Attachment #773836 - Attachment is obsolete: true
Attachment #775079 - Flags: review?(adw)
(Assignee)

Comment 30

6 years ago
Comment on attachment 775079 [details] [diff] [review]
part 1 - regenerate stale page thumbnails via new captureIfStale() method, and send notification when done.

bugger - forgot to do one of the test tweaks requested by Drew.
Attachment #775079 - Attachment is obsolete: true
Attachment #775079 - Flags: review?(adw)
(Assignee)

Comment 32

6 years ago
FTR, putting part2 up here with Tim's review comments addressed.  Carrying r+ forward.

(In reply to Tim Taubert [:ttaubert] from comment #24)
> > +    this.refreshThumbnail();
> 
> I wonder, does this actually work to refresh the thumbnail? We actually
> don't change the .style.backgroundImage string so does this really request
> the image again? If not just clearing the value before that would probably
> do.

It's actually a little strange - I had to actually move the mouse (!) before I saw the thumbnail update.  Given that, I took your suggestion and now style.removeProperty(...) before setting it and my 1 little test shows it updating immediately.
Attachment #773837 - Attachment is obsolete: true
Attachment #775082 - Flags: review+
(Reporter)

Updated

6 years ago
Attachment #775081 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/d493a47e087a
https://hg.mozilla.org/mozilla-central/rev/1accaa61b257
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
FWIW, I've asked the QA community to keep an eye out and report any issues they encouter potentially related to this bug.
RyanVM pointed out that this was causing some Reftest/Crashtest failures, so I pushed a followup to flip the pref there as well:

https://hg.mozilla.org/integration/mozilla-inbound/rev/de9d4c0eddb8
Worked like a charm, thanks.
Comment on attachment 775082 [details] [diff] [review]
part 2 - review comments addressed.

>+  refreshThumbnail: function Site_refreshThumbnail() {
>     let thumbnailURL = PageThumbs.getThumbnailURL(this.url);
>     let thumbnail = this._querySelector(".newtab-thumbnail");
>+    // if this is being called due to the thumbnail being updated we will
>+    // be setting it to the same value it had before.  To be confident the
>+    // change wont be optimized away we remove the property first.
>+    thumbnail.style.removeProperty("backgroundImage");

I bet this doesn't work, because it should be "background-image". Maybe this line isn't needed after all? Or it's needed and the effect you tried to prevent is still there...
Depends on: 897408
Now how to turn this off ?

Pref: browser.pagethumbnails.capturing_disabled set to true stopped all display of thumbs, now with this patch their back being displayed.  

I don't want to see thumbs ever... even if I choose the option to show the thumbs panel, if its clicked I want just titles, blank squares not thumbs being shown.
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #40)
> Pref: browser.pagethumbnails.capturing_disabled set to true stopped all
> display of thumbs, now with this patch their back being displayed.  

We should probably fix the prefs to be consistent. Can you file a new bug?

Updated

6 years ago
Depends on: 897794
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #41)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #40)
> > Pref: browser.pagethumbnails.capturing_disabled set to true stopped all
> > display of thumbs, now with this patch their back being displayed.  
> 
> We should probably fix the prefs to be consistent. Can you file a new bug?

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=897811
Depends on: 898598

Updated

6 years ago
Depends on: 898825
No longer depends on: 898156, 898825

Updated

6 years ago
Depends on: 901294

Updated

6 years ago
Duplicate of this bug: 752343
Depends on: 903949
(Assignee)

Updated

6 years ago
Depends on: 904341
Depends on: 897503
No longer depends on: 897503
On a clean profile, visit some sites, restart browser -> the thumbnails are generated live one at a time in the newtab page. But no thumbnails are displayed before the restart. Is this ok ?
Flags: needinfo?
(Reporter)

Comment 45

6 years ago
(In reply to Paul Silaghi [QA] from comment #44)
> On a clean profile, visit some sites, restart browser -> the thumbnails are
> generated live one at a time in the newtab page. But no thumbnails are
> displayed before the restart. Is this ok ?

Thanks for mentioning this.  I filed bug 911307 for it.
Flags: needinfo?
Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.
Keywords: feature
Depends on: 903990
(Reporter)

Updated

6 years ago
Depends on: 912763
(Reporter)

Comment 47

6 years ago
This bug was later disabled on Beta (and release) in bug 912763, but it has relnote-firefox=25+, so it ended up included in the Beta 25 release notes [1].  So I'm clearing that flag now.  I don't know if that's kosher or what, and we don't know when we'll enable it on Beta, but it shouldn't be listed in release notes until it's turned on.  (I filed bug 920676 to update [1].)

[1] http://www.mozilla.org/en-US/firefox/25.0beta/releasenotes/
(Reporter)

Updated

6 years ago
Blocks: 927688
Depends on: 911307
(Reporter)

Updated

6 years ago
No longer depends on: 911307

Updated

6 years ago
Depends on: 934256
(Reporter)

Updated

6 years ago
No longer depends on: 948413
Depends on: 985521
(Reporter)

Updated

5 years ago
No longer depends on: 985521

Updated

2 years ago
Depends on: 1384094

Updated

6 months ago
Depends on: 1509084
You need to log in before you can comment on or make changes to this bug.