725268 - [Page Thumbnails] Reduce API provided by PageThumbs.jsm

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

From bug 720838 comment #12:

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Implements the proposed API.

1) capture() remains untouched.
2) captureAndStore(aBrowser[, aCallback]) was added
3) store() got removed

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 595382 [details] [diff] [review]
patch v1

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

::: browser/components/thumbnails/PageThumbs.jsm
@@ +110,3 @@
>     */
> +  captureAndStore: function PageThumbs_captureAndStore(aBrowser, aCallback) {
> +    this.capture(aBrowser.contentWindow, function (aData) {

aData too ambiguous, rename to reflect actual type.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Backed out because I accidentally slipped in changes from another patch:

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

Pushed again:

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/363923689d48
https://hg.mozilla.org/mozilla-central/rev/113832a73df9