Closed Bug 818225 Opened 7 years ago Closed 6 years ago

PageThumbs should have an easy way to check to see if a page has a thumbnail

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: mkaply, Assigned: brennan.brisad)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 4 obsolete files)

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.
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
If anyone is interested in picking up that bug, I can mentor it.

Otherwise, I will get around to it whenever I can.
Whiteboard: [mentor=Yoric][lang=js]
Actually, PageThumbsStorage.getFilePathForURL is probably a good enough API.

Tim, what do you think?
Flags: needinfo?(ttaubert)
(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?
Flags: needinfo?(ttaubert)
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.
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.
Attachment #760262 - Flags: review?(dteller)
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.
Attachment #760262 - Flags: review?(dteller) → feedback+
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?
Attachment #760262 - Attachment is obsolete: true
Attachment #761332 - Flags: feedback?(dteller)
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.
Attachment #761332 - Flags: feedback?(dteller) → feedback+
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.
Attachment #761332 - Attachment is obsolete: true
Attachment #763301 - Flags: review?(dteller)
Blocks: 884458
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 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.
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
Attachment #763301 - Flags: review?(ttaubert)
Attachment #763301 - Flags: review?(dteller)
Attachment #763301 - Flags: feedback+
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.
Attachment #763301 - Flags: review?(ttaubert) → feedback+
Assignee: nobody → brennan.brisad
Thanks!  I've attached a new patch with the modifications.
Attachment #763301 - Attachment is obsolete: true
Attachment #769484 - Flags: review?(dteller)
Attachment #769484 - Flags: review?(dteller) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Added r=yoric to end of patch summary
Attachment #769484 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2734bd00f3ff
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2734bd00f3ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][fixed-in-fx-team] → [mentor=Yoric][lang=js]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.