Mike Kaply [:mkaply] Reporter
	Description • 12 years ago

Right now, to detect if a given URL has a thumbnail, you have to call into PageThumbsStorage directly using getFileForURL on FF17 or pre FF17 check the PageThumbsCache for a read entry.

It would be nice if PageThumbs had a simple function that given a URL whether or not it has a thumbnail.

This would avoid reliance on the page thumbs storage model.

	Ben Bucksch (:BenB)
	Comment 1 • 11 years ago

There are other cases where the actual thumb image would be needed by an extension, so an API for that is needed anyway. Once we have that, it's easy to test that function for a null result, so I don't think we need a separate function for existence. Or do I miss something here?

xref bug 857201

	David Teller [:Yoric] - still alive but not very active
	Comment 2 • 11 years ago

If anyone is interested in picking up that bug, I can mentor it.

Otherwise, I will get around to it whenever I can.

	David Teller [:Yoric] - still alive but not very active
	Comment 3 • 11 years ago

Actually, PageThumbsStorage.getFilePathForURL is probably a good enough API.

Tim, what do you think?

	Tim Taubert [:ttaubert] (inactive)
	Comment 4 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> Actually, PageThumbsStorage.getFilePathForURL is probably a good enough API.

PageThumbsStorage is not meant to be public although it currently kind of is - that's only because the channel needs it (that itself is part of the implementation).

I'd rather have people only use the PageThumbs module so that they don't really need to know about the underlying thumbnail storage. If we return a file name as string they'll probably just end up using nsIFile.exists(). So how about a dedicated method that returns a promise that resolves to a boolean?

	Ben Bucksch (:BenB)
	Comment 5 • 11 years ago

Tim, as mentioned in comment 1, there are cases where only the existence of the thumb is important, but there are just as many use cases where the actual thumb image is needed. Given that the latter implies the former, I think some kind of reference to the thumb is better as API than a boolean.

	Michael Hoffmann Assignee
	Comment 6 • 11 years ago

Hi, I wrote a patch that simply adds a getThumbnail method to PageThumbs. It returns a promise that resolves to a nsILocalFile if it exists, otherwise null.

	David Teller [:Yoric] - still alive but not very active
	Comment 7 • 11 years ago

Comment on attachment 760262 [details] [diff] [review]
Add getThumbnails method to PageThumbs

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

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +189,5 @@
> +   */
> +  getThumbnail: function PageThumbs_getThumbnail(aUrl) {
> +    let deferred = Promise.defer();
> +
> +    TaskUtils.spawn((function task() {

You are actually not using this TaskUtils.spawn.

@@ +199,5 @@
> +          deferred.resolve(file.exists() ? file : null);
> +        } catch (ex) {
> +          deferred.reject(ex);
> +        }
> +    }).bind(this));

Let's return a |path| instead of a |nsIFile|.

::: toolkit/components/thumbnails/test/browser_thumbnails_bug818225.js
@@ +10,5 @@
> +  yield testGetThumbnail(URL, function onSuccess(file) {
> +    is(file, null, "Thumbnail doesn't exist");
> +    executeSoon(next);
> +  });
> +                        

Nit: trailing whitespace.

@@ +17,5 @@
> +  yield testGetThumbnail(URL, function onSuccess(file) {
> +    let path = PageThumbsStorage.getFilePathForURL(URL);
> +    is(file.path, path, "Thumbnail file has correct path");
> +    executeSoon(next);
> +  });

You probably want to also check whether the file exists.

	Michael Hoffmann Assignee
	Comment 8 • 11 years ago

Thanks for the feedback!  I'm attaching a new patch with modifications according to your comments.

I'm not sure about the last yield in runTests(), it's quite nested.  Is that OK, or am I doing it the wrong way?

	David Teller [:Yoric] - still alive but not very active
	Comment 9 • 11 years ago

Comment on attachment 761332 [details] [diff] [review]
Add getThumbnail method to PageThumbs v2

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

I believe that this will work, but you should, of course, test it :)

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +186,5 @@
> +   *
> +   * @param aUrl The web page's url.
> +   * @return {Promise} A promise resolving to the path or null if the
> +   * file does not exist.
> +   */

I wonder if it wouldn't be as easy and a little more generic to return just the |thumbPath| and let the clients decide what they want to do with it.

	Michael Hoffmann Assignee
	Comment 10 • 11 years ago

Ok, I went with your last note and keep it simple instead.  I changed the patch to only return the path given by PageThumbsStorage.getFilePathForURL.  I have tested it on try.

	Mike Kaply [:mkaply] Reporter
	Comment 11 • 11 years ago

What would someone do with the path except create an nsIFile?

Shouldn't we save the caller the trouble? 

I've seen this trend with other APIs and I don't understand it. In the Mozilla world, if you have an nsIFile, you can get anything you need...

	David Teller [:Yoric] - still alive but not very active
	Comment 12 • 11 years ago

(In reply to Michael Kaply (mkaply) from comment #11)
> What would someone do with the path except create an nsIFile?
> 
> Shouldn't we save the caller the trouble? 
> 
> I've seen this trend with other APIs and I don't understand it. In the
> Mozilla world, if you have an nsIFile, you can get anything you need...

In JavaScript, we are deprecating nsIFile (which is bad for performance) in favor of OS.File. So we want a OS.File friendly structure, i.e. a string.

	David Teller [:Yoric] - still alive but not very active
	Comment 13 • 11 years ago

Comment on attachment 763301 [details] [diff] [review]
Add getThumbnail method to PageThumbs v3

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

I'll let ttaubert chime in, as he's the owner of that code, but this looks good to me.

::: toolkit/components/thumbnails/test/browser_thumbnails_bug818225.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/" +
> +            "test/thumbnail_sample.html";

Nit: To avoid accidents, I would rename the file/url to thumbnail_sample_bug818225.html

	Tim Taubert [:ttaubert] (inactive)
	Comment 14 • 11 years ago

Comment on attachment 763301 [details] [diff] [review]
Add getThumbnail method to PageThumbs v3

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

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +187,5 @@
> +   *
> +   * @param aUrl The web page's url.
> +   * @return The path of the thumbnail file.
> +   */
> +  getThumbnail: function PageThumbs_getThumbnail(aUrl) {

Shouldn't this rather be 'getThumbnailPath'?

::: toolkit/components/thumbnails/test/browser_thumbnails_bug818225.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/" +
> +            "test/thumbnail_sample.html";

Why don't you just use toolkit/components/thumbnails/test/background_red.html? I think you could append some query string [e.g. Date.now()] so that we're sure there's no thumbnail, yet.

	Michael Hoffmann Assignee
	Comment 15 • 11 years ago

Thanks!  I've attached a new patch with the modifications.

	Michael Hoffmann Assignee
	Comment 16 • 11 years ago

Added r=yoric to end of patch summary

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/2734bd00f3ff

	Tim Taubert [:ttaubert] (inactive)
	Comment 18 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2734bd00f3ff