Last Comment Bug 720838 - [Page Thumbnails] Use canvas.mozFetchAsStream()
: [Page Thumbnails] Use canvas.mozFetchAsStream()
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on: 720697
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-24 14:13 PST by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2012-02-10 07:24 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (12.21 KB, patch)
2012-01-25 05:35 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v2 (8.21 KB, patch)
2012-02-03 05:51 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dietrich: review+
Details | Diff | Splinter Review
patch v3 (8.34 KB, patch)
2012-02-08 05:00 PST, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-01-24 14:13:29 PST
Use canvas.mozFetchAsStream() as soon as bug 720697 landed.
Comment 1 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-01-25 05:35:36 PST
Created attachment 591433 [details] [diff] [review]
patch v1
Comment 2 Dão Gottwald [:dao] 2012-01-25 05:38:50 PST
Comment on attachment 591433 [details] [diff] [review]
patch v1

It looks like 'capture' could take a browser and call 'store' automatically.
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-03 05:51:39 PST
Created attachment 594149 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #2)
> It looks like 'capture' could take a browser and call 'store' automatically.

It could but now that 'capture' feeds the callback with a nsIInputStream containing the image data I'd rather let the API be flexible so that callers can decide how to capture and store images and pick their own keys for example.
Comment 4 Dão Gottwald [:dao] 2012-02-03 06:01:32 PST
Why should anyone call 'capture' but not 'store'?
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-03 06:11:25 PST
I suspect that some JavaScript code might want to get a thumbnail as a nsIInputStream and examine or modify it rather than using a canvas.
Comment 6 Dão Gottwald [:dao] 2012-02-03 06:15:56 PST
Is this a guess or really a well-founded suspicion?
Can we start with the simpler API and extend it when somebody asks for it?
Comment 7 Dietrich Ayala (:dietrich) 2012-02-04 06:05:05 PST
Comment on attachment 594149 [details] [diff] [review]
patch v2

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

thanks, r=me.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +137,3 @@
>  
> +      // Write the image data to the cache entry.
> +      NetUtil.asyncCopy(aData, outputStream, function (aResult) {

please rename aData to indicate that it's a stream.
Comment 8 Dietrich Ayala (:dietrich) 2012-02-04 06:07:37 PST
(In reply to Dão Gottwald [:dao] from comment #6)
> Is this a guess or really a well-founded suspicion?
> Can we start with the simpler API and extend it when somebody asks for it?

this is neither - it's something we thought we might want to do with this api in the future. specifically, in jetpack.

also, that question is not relevant to this bug - that api was not added here. please file a different bug if you think this is a serious enough issue.
Comment 9 Dão Gottwald [:dao] 2012-02-04 06:35:48 PST
Comment on attachment 594149 [details] [diff] [review]
patch v2

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Is this a guess or really a well-founded suspicion?
> > Can we start with the simpler API and extend it when somebody asks for it?
> 
> this is neither - it's something we thought we might want to do with this
> api in the future. specifically, in jetpack.

File a bug when you actually need it? Seriously, API creep isn't fun, especially if it makes the API more cumbersome to use (e.g. having to add a callback in order to call 'store').

> also, that question is not relevant to this bug - that api was not added
> here. please file a different bug if you think this is a serious enough
> issue.

The callback is added here.
Comment 10 Dietrich Ayala (:dietrich) 2012-02-04 08:34:18 PST
(In reply to Dão Gottwald [:dao] from comment #9)
> File a bug when you actually need it? Seriously, API creep isn't fun,
> especially if it makes the API more cumbersome to use (e.g. having to add a
> callback in order to call 'store').

This is not API creep. It's for a use that we very likely want in Jetpack. 

Also, specializing this API like you suggest results in more code jammed into overly large methods, which are difficult to debug and more error prone than separating it into pieces anyway.

Can you explain exactly why this particular change is so complicated to you? We have APIs throughout the codebase that are far more complicated than this. It's a simple get and store.
Comment 11 Dão Gottwald [:dao] 2012-02-04 10:41:59 PST
I think we should have a simple method for what browser-thumbnails.js wants to do: tell PageThumbs to take and store a screenshot. It doesn't need to know nothing about the data stream for this. I wouldn't object to another method with the purpose of getting the data stream, although I'd prefer this to be postponed to when you *really* need it, to avoid creating potentially unused interfaces.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-06 11:03:36 PST
I agree that adding a captureAndStore(aBrowser) method on PageThumbs would be useful, to reduce unnecessary complexity for the primary use of this API (browser-thumbnails.js).

Exposing store() as part of the PageThumbs API doesn't seem justified at the moment. Who would want to use an arbitrary key or save data from another source? This could actually lead to trouble if addons end up improperly storing data. I can see capture()'s potentially utility, though, and keeping it doesn't seem harmful.

Neither of these suggested API changes are directly related to this bug, though (apart from this bug also changing the existing API slightly). I don't think we need to block the improvement of using mozFetchAsStream() on the API changes.
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-08 04:40:14 PST
Filed bug 725268 to track the API changes.
Comment 14 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-08 05:00:52 PST
Created attachment 595381 [details] [diff] [review]
patch v3

v2 with some little corrections. The overall API change proposed by Dão and Gavin will be implemented in bug 725268.
Comment 15 Dietrich Ayala (:dietrich) 2012-02-08 12:05:24 PST
Comment on attachment 595381 [details] [diff] [review]
patch v3

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

::: browser/base/content/browser-thumbnails.js
@@ +79,5 @@
>    _capture: function Thumbnails_capture(aBrowser) {
>      if (this._shouldCapture(aBrowser)) {
> +      this._pageThumbs.capture(aBrowser.contentWindow, function (aData) {
> +        this._pageThumbs.store(aBrowser.currentURI.spec, aData);
> +      }.bind(this));

rename aData to indicate it's a stream.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +111,4 @@
>     * @param aCallback The function to be called when the canvas data has been
>     *                  stored (optional).
>     */
> +  store: function PageThumbs_store(aKey, aData, aCallback) {

rename aData to indicate it's a stream.
Comment 16 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-09 02:41:19 PST
https://hg.mozilla.org/integration/fx-team/rev/c5290a826c32
Comment 17 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-02-10 07:24:33 PST
https://hg.mozilla.org/mozilla-central/rev/c5290a826c32

Note You need to log in before you can comment on or make changes to this bug.