720838 - [Page Thumbnails] Use canvas.mozFetchAsStream()

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Use canvas.mozFetchAsStream() as soon as bug 720697 landed.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Dão Gottwald [:dao]]

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] (inactive)]

Attachment: 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]]

Why should anyone call 'capture' but not 'store'?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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]]

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)]

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)]

(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]]

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)]

(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]]

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: [e-mail]]]

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] (inactive)]

Filed bug 725268 to track the API changes.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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)]

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] (inactive)]

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

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c5290a826c32