Issue 11985003: Refactored-out the code of thumbnaling algorithm from thumbnail_tab_helper.

Issue 11985003: Refactored-out the code of thumbnaling algorithm from thumbnail_tab_helper. (Closed)

Created:
7 years, 11 months ago by motek.

Modified:
7 years, 11 months ago

Reviewers:
mazda, sky

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Refactored-out the code of thumbnaling algorithm from thumbnail_tab_helper. The purpose is that we will be trying out (Finching?) a new experimental thumbnailing algorithm. This CL is split from the 'main thing' to make reviewers' life a bit easier. BUG=155269 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178131

Patch Set 1 #

Patch Set 2 : Added a unit test. #

Total comments: 12

Patch Set 3 : Addressed reviewer's remarks. #

Patch Set 4 : Updated some comments. #

Patch Set 5 : #

Total comments: 24

Patch Set 6 : Addressed owner's comments. #

Patch Set 7 : Compliation problem (win). #

Patch Set 8 : Another OS_WIN thing. #

Patch Set 9 : Updated a comment. #

Total comments: 4

Patch Set 10 : Another round of updates from OWNER's review. #

Created: 7 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+757 lines, -529 lines)			Patch
A	chrome/browser/thumbnails/simple_thumbnail_crop.h	View	1 2 3 4 5 6 7	1 chunk	+69 lines, -0 lines	0 comments	Download
A	chrome/browser/thumbnails/simple_thumbnail_crop.cc	View	1 2 3 4 5 6 7	1 chunk	+246 lines, -0 lines	0 comments	Download
A	chrome/browser/thumbnails/simple_thumbnail_crop_unittest.cc	View	1 2 3 4 5	1 chunk	+198 lines, -0 lines	0 comments	Download
M	chrome/browser/thumbnails/thumbnail_service.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+12 lines, -3 lines	0 comments	Download
M	chrome/browser/thumbnails/thumbnail_service_impl.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/thumbnails/thumbnail_service_impl.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+18 lines, -4 lines	0 comments	Download
M	chrome/browser/thumbnails/thumbnail_tab_helper.h	View	1 2	3 chunks	+1 line, -79 lines	0 comments	Download
M	chrome/browser/thumbnails/thumbnail_tab_helper.cc	View	1 2 3 4 5 6 7 8 9	4 chunks	+73 lines, -297 lines	0 comments	Download
D	chrome/browser/thumbnails/thumbnail_tab_helper_unittest.cc	View		1 chunk	+0 lines, -142 lines	0 comments	Download
A	chrome/browser/thumbnails/thumbnailing_algorithm.h	View	1 2 3 4 5 6 7	1 chunk	+55 lines, -0 lines	0 comments	Download
A	chrome/browser/thumbnails/thumbnailing_context.h	View	1 2 3 4 5	1 chunk	+51 lines, -0 lines	0 comments	Download
A	chrome/browser/thumbnails/thumbnailing_context.cc	View	1 2 3 4 5	1 chunk	+25 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8	2 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests_unit.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

mazda

https://codereview.chromium.org/11985003/diff/2001/chrome/browser/thumbnails/simple_thumbnail_crop.cc File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right): https://codereview.chromium.org/11985003/diff/2001/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode26 chrome/browser/thumbnails/simple_thumbnail_crop.cc:26: nit: remove an extra line. https://codereview.chromium.org/11985003/diff/2001/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode180 chrome/browser/thumbnails/simple_thumbnail_crop.cc:180: nit: remove ...

7 years, 11 months ago (2013-01-17 18:08:43 UTC) #1

motek.

https://chromiumcodereview.appspot.com/11985003/diff/2001/chrome/browser/thumbnails/simple_thumbnail_crop.cc File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right): https://chromiumcodereview.appspot.com/11985003/diff/2001/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode26 chrome/browser/thumbnails/simple_thumbnail_crop.cc:26: On 2013/01/17 18:08:43, mazda wrote: > nit: remove an ...

7 years, 11 months ago (2013-01-17 18:39:42 UTC) #2

sky

https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/simple_thumbnail_crop.cc File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right): https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode24 chrome/browser/thumbnails/simple_thumbnail_crop.cc:24: } DCHECK size not empty https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode26 chrome/browser/thumbnails/simple_thumbnail_crop.cc:26: SimpleThumbnailCrop::~SimpleThumbnailCrop() { ...

7 years, 11 months ago (2013-01-17 20:18:35 UTC) #4

motek.

Addressed all comments. https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/simple_thumbnail_crop.cc File chrome/browser/thumbnails/simple_thumbnail_crop.cc (right): https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/simple_thumbnail_crop.cc#newcode24 chrome/browser/thumbnails/simple_thumbnail_crop.cc:24: } On 2013/01/17 20:18:35, sky wrote: ...

7 years, 11 months ago (2013-01-18 18:38:41 UTC) #5

sky

https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/thumbnailing_algorithm.h File chrome/browser/thumbnails/thumbnailing_algorithm.h (right): https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/thumbnailing_algorithm.h#newcode21 chrome/browser/thumbnails/thumbnailing_algorithm.h:21: : public base::RefCountedThreadSafe<ThumbnailingAlgorithm> { On 2013/01/18 18:38:41, motek. wrote: ...

7 years, 11 months ago (2013-01-18 21:43:26 UTC) #6

motek.

7 years, 11 months ago (2013-01-18 22:56:52 UTC) #7

sky

On Fri, Jan 18, 2013 at 2:56 PM, <motek@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thumbnails/thumbnailing_algorithm.h > File ...

7 years, 11 months ago (2013-01-18 23:36:21 UTC) #8

On Fri, Jan 18, 2013 at 2:56 PM,  <motek@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thu...
> File chrome/browser/thumbnails/thumbnailing_algorithm.h (right):
>
>
https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thu...
> chrome/browser/thumbnails/thumbnailing_algorithm.h:21: : public
> base::RefCountedThreadSafe<ThumbnailingAlgorithm> {
> On 2013/01/18 21:43:26, sky wrote:
>>
>> On 2013/01/18 18:38:41, motek. wrote:
>> > On 2013/01/17 20:18:36, sky wrote:
>> > > Why is this ref counted thread safe?
>> > The new algorithm(s) will be more costly than the current one. The
>
> idea is to
>>
>> > have the option to shunt parts of thumbnail cropping to another
>
> thread.
>
>> Why does that mean they need to be ref counted? Isn't there a single
>
> clean owner
>>
>> here?
>
>
> Ah, I see what you mean. 'Need' is not the best word here. It clearly
> can be either owned or one-time use. However, I still believe we would
> be better off having ThumbnailingAlgorithm as a disposable refcounted
> entity. Here are my reasons:
>
> 1. I think the only reasonable owner would be ThumbnailService.
> 2. ThumbnailService itself is refcounted so in principle it could go
> away. Thus, passing methods of an entity *it* owns bound as callbacks
> would be as problematic. I would like to be able to do it, it generally
> results in less boilerplate.
> 3. For implementing the new algo (GaussianGradientThumbnailCrop, in
> works) I would like to keep intermediate data in the instance. To this
> end, having a single use entity would be very convenient. If I make it
> 'owned', it will have to be stateless with all methods const-ed.

Ah, this is the real issue. I was assuming ThumbnailingAlgorithms were
light weight stateless objects:

// Create and return a new instance of the algorithm. These objects are
 34   // lightweight and are expected to self-destruct. They
encapsulate the process
 35   // of creating a thumbnail from tab contents.
 36   virtual ThumbnailingAlgorithm* CreateThumbnailingAlgorithm() const = 0;

If that isn't the case, update this description appropriately.

  -Scott

> In summary, I think that such design gives me considerable flexibility
> (up to employing more then one algorithm if we need different thumbnails
> for different purposes) at no cost.
>
> https://chromiumcodereview.appspot.com/11985003/

motek.

I have fixed up the comment in thumbnail_service.h to clarify this matter. Would that work? ...

7 years, 11 months ago (2013-01-21 19:03:09 UTC) #9

I have fixed up the comment in thumbnail_service.h to clarify this matter. Would
that work?
On 2013/01/18 23:36:21, sky wrote:
> On Fri, Jan 18, 2013 at 2:56 PM,  <mailto:motek@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thu...
> > File chrome/browser/thumbnails/thumbnailing_algorithm.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/11985003/diff/11002/chrome/browser/thu...
> > chrome/browser/thumbnails/thumbnailing_algorithm.h:21: : public
> > base::RefCountedThreadSafe<ThumbnailingAlgorithm> {
> > On 2013/01/18 21:43:26, sky wrote:
> >>
> >> On 2013/01/18 18:38:41, motek. wrote:
> >> > On 2013/01/17 20:18:36, sky wrote:
> >> > > Why is this ref counted thread safe?
> >> > The new algorithm(s) will be more costly than the current one. The
> >
> > idea is to
> >>
> >> > have the option to shunt parts of thumbnail cropping to another
> >
> > thread.
> >
> >> Why does that mean they need to be ref counted? Isn't there a single
> >
> > clean owner
> >>
> >> here?
> >
> >
> > Ah, I see what you mean. 'Need' is not the best word here. It clearly
> > can be either owned or one-time use. However, I still believe we would
> > be better off having ThumbnailingAlgorithm as a disposable refcounted
> > entity. Here are my reasons:
> >
> > 1. I think the only reasonable owner would be ThumbnailService.
> > 2. ThumbnailService itself is refcounted so in principle it could go
> > away. Thus, passing methods of an entity *it* owns bound as callbacks
> > would be as problematic. I would like to be able to do it, it generally
> > results in less boilerplate.
> > 3. For implementing the new algo (GaussianGradientThumbnailCrop, in
> > works) I would like to keep intermediate data in the instance. To this
> > end, having a single use entity would be very convenient. If I make it
> > 'owned', it will have to be stateless with all methods const-ed.
> 
> Ah, this is the real issue. I was assuming ThumbnailingAlgorithms were
> light weight stateless objects:
> 
> // Create and return a new instance of the algorithm. These objects are
>  34   // lightweight and are expected to self-destruct. They
> encapsulate the process
>  35   // of creating a thumbnail from tab contents.
>  36   virtual ThumbnailingAlgorithm* CreateThumbnailingAlgorithm() const = 0;
> 
> If that isn't the case, update this description appropriately.
> 
>   -Scott
> 
> > In summary, I think that such design gives me considerable flexibility
> > (up to employing more then one algorithm if we need different thumbnails
> > for different purposes) at no cost.
> >
> > https://chromiumcodereview.appspot.com/11985003/

sky

LGTM with the following changes https://chromiumcodereview.appspot.com/11985003/diff/39001/chrome/browser/thumbnails/thumbnail_service.h File chrome/browser/thumbnails/thumbnail_service.h (right): https://chromiumcodereview.appspot.com/11985003/diff/39001/chrome/browser/thumbnails/thumbnail_service.h#newcode33 chrome/browser/thumbnails/thumbnail_service.h:33: // Create and return ...

7 years, 11 months ago (2013-01-22 16:10:28 UTC) #10

motek.

All fixed as suggested. About to commit. https://chromiumcodereview.appspot.com/11985003/diff/39001/chrome/browser/thumbnails/thumbnail_service.h File chrome/browser/thumbnails/thumbnail_service.h (right): https://chromiumcodereview.appspot.com/11985003/diff/39001/chrome/browser/thumbnails/thumbnail_service.h#newcode33 chrome/browser/thumbnails/thumbnail_service.h:33: // Create ...

7 years, 11 months ago (2013-01-22 16:56:19 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/11985003/42002

7 years, 11 months ago (2013-01-22 20:37:19 UTC) #12

Message was sent while issue was closed.

Change committed as 178131

Expand Messages | Collapse Messages