Closed
Bug 818225
Opened 12 years ago
Closed 11 years ago
PageThumbs should have an easy way to check to see if a page has a thumbnail
Categories
(Firefox :: Tabbed Browser, enhancement)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 26
People
(Reporter: mkaply, Assigned: brennan.brisad)
Details
(Whiteboard: [mentor=Yoric][lang=js])
Attachments
(1 file, 4 obsolete files)
3.80 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•12 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
Comment 2•12 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.
Whiteboard: [mentor=Yoric][lang=js]
Comment 3•12 years ago
|
||
Actually, PageThumbsStorage.getFilePathForURL is probably a good enough API.
Tim, what do you think?
Flags: needinfo?(ttaubert)
Comment 4•12 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?
Flags: needinfo?(ttaubert)
Comment 5•12 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.
Assignee | ||
Comment 6•12 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.
Attachment #760262 -
Flags: review?(dteller)
Comment 7•12 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.
Attachment #760262 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 8•12 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?
Attachment #760262 -
Attachment is obsolete: true
Attachment #761332 -
Flags: feedback?(dteller)
Comment 9•12 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.
Attachment #761332 -
Flags: feedback?(dteller) → feedback+
Assignee | ||
Comment 10•12 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.
Attachment #761332 -
Attachment is obsolete: true
Attachment #763301 -
Flags: review?(dteller)
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...
(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 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.
Attachment #763301 -
Flags: review?(ttaubert) → feedback+
Updated•11 years ago
|
Assignee: nobody → brennan.brisad
Assignee | ||
Comment 15•11 years ago
|
||
Thanks! I've attached a new patch with the modifications.
Attachment #763301 -
Attachment is obsolete: true
Attachment #769484 -
Flags: review?(dteller)
Updated•11 years ago
|
Attachment #769484 -
Flags: review?(dteller) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•11 years ago
|
||
Added r=yoric to end of patch summary
Attachment #769484 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 17•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][fixed-in-fx-team]
Comment 18•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 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.
Description
•