794804 - [Page Thumbnails] minimize chrome/worker communication when expiring thumbnails

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Let's take the patch from bug 775540 and fix it in this bug.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - minimize chrome/worker communication when expiring thumbnails

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - add tests for thumbnails expiration

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 665325 [details] [diff] [review]
part 1 - minimize chrome/worker communication when expiring thumbnails

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

Looks good.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +449,5 @@
> +    PageThumbsWorker.postMessage({
> +      type: "expireFilesInDirectory",
> +      minChunkSize: EXPIRATION_MIN_CHUNK_SIZE,
> +      path: PageThumbsStorage.getDirectory().path,
> +      filesToKeep: [PageThumbsStorage.getLeafNameForURL(url) for (url of aURLsToKeep)]

By the way, if we want to minimize even further, note that we only need to send |minChunkSize| and path the first time and that we only need to send |filesToKeep| if it changes.

Maybe in a followup bug? Or do you think this is overoptimization?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 665349 [details] [diff] [review]
part 2 - add tests for thumbnails expiration

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

Looks good.
In the future, you may wish to take a look at bug 763311, which might be well suited for your testsuite.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +450,5 @@
>        type: "expireFilesInDirectory",
>        minChunkSize: EXPIRATION_MIN_CHUNK_SIZE,
>        path: PageThumbsStorage.getDirectory().path,
>        filesToKeep: [PageThumbsStorage.getLeafNameForURL(url) for (url of aURLsToKeep)]
> +    }, aCallback);

I am a bit lost here. Why is this change in the testsuite and not in the main patch?

::: browser/components/thumbnails/test/browser_thumbnails_expiration.js
@@ +68,5 @@
> +
> +  let data = "dummy";
> +  fos.write(data, data.length);
> +  FileUtils.closeSafeFileOutputStream(fos);
> +}

Cough! cough!
(sorry, just a reflex when seeing this sync I/O, never mind me)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> By the way, if we want to minimize even further, note that we only need to
> send |minChunkSize| and path the first time and that we only need to send
> |filesToKeep| if it changes.
> 
> Maybe in a followup bug? Or do you think this is overoptimization?

That's true but I think this would indeed be a little over-optimized. The code would be a little harder to read and the caller would have to know about the worker's state which is not too ideal I think.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> In the future, you may wish to take a look at bug 763311, which might be
> well suited for your testsuite.

Yeah, we should do that. The new tab page and the thumbnail services are using test runners and we should sooner or later switch to Task.jsm.

> ::: browser/components/thumbnails/PageThumbs.jsm
> @@ +450,5 @@
> >        type: "expireFilesInDirectory",
> >        minChunkSize: EXPIRATION_MIN_CHUNK_SIZE,
> >        path: PageThumbsStorage.getDirectory().path,
> >        filesToKeep: [PageThumbsStorage.getLeafNameForURL(url) for (url of aURLsToKeep)]
> > +    }, aCallback);
> 
> I am a bit lost here. Why is this change in the testsuite and not in the
> main patch?

That's just a minor change to make the test suite know the expiration has been finished. I thought it would rather belong to the test suite patch as that's the reason for this change.

> ::: browser/components/thumbnails/test/browser_thumbnails_expiration.js
> @@ +68,5 @@
> > +
> > +  let data = "dummy";
> > +  fos.write(data, data.length);
> > +  FileUtils.closeSafeFileOutputStream(fos);
> > +}
> 
> Cough! cough!
> (sorry, just a reflex when seeing this sync I/O, never mind me)

Makes tests like this a little easier to write :)

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/da72409eda0e
https://hg.mozilla.org/integration/fx-team/rev/dd150b8dc4dc

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/da72409eda0e
https://hg.mozilla.org/mozilla-central/rev/dd150b8dc4dc