Last Comment Bug 725268 - [Page Thumbnails] Reduce API provided by PageThumbs.jsm
: [Page Thumbnails] Reduce API provided by PageThumbs.jsm
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]
:
: Dão Gottwald [:dao]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-08 04:37 PST by Tim Taubert [:ttaubert]
Modified: 2012-02-10 07:26 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (6.93 KB, patch)
2012-02-08 05:03 PST, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-02-08 04:37:50 PST
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] 2012-02-08 05:03:35 PST
Created attachment 595382 [details] [diff] [review]
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) 2012-02-08 12:09:48 PST
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] 2012-02-09 02:41:44 PST
https://hg.mozilla.org/integration/fx-team/rev/bcd726a4a258
Comment 4 Tim Taubert [:ttaubert] 2012-02-09 02:52:18 PST
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] 2012-02-10 07:25:26 PST
https://hg.mozilla.org/mozilla-central/rev/bcd726a4a258

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