Closed Bug 794804 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(2 files)

Let's take the patch from bug 775540 and fix it in this bug.
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?
Attachment #665325 - Flags: review?(dteller) → review+
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)
Attachment #665349 - Flags: review?(dteller) → review+
(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.
(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 :)
https://hg.mozilla.org/mozilla-central/rev/da72409eda0e
https://hg.mozilla.org/mozilla-central/rev/dd150b8dc4dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.