cc: Remove PicturePileBase

cc: Remove PicturePileBase

- remove required contents of PicturePileBase into PicturePile
- move as many test functions into TextPicturePile as posible
  and moved into separate file to share the class

Committed: https://crrev.com/312ee8acb412671d98f3c1e7c0fc0304dc9b7fea
Cr-Commit-Position: refs/heads/master@{#303919}

[hendrikw, 2014-11-11 23:43:44]

hendrikw@chromium.org changed reviewers:
+ enne@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-11-11 23:45:36]

hendrikw@chromium.org changed reviewers:
+ danakj@chromium.org
- enne@chromium.org

[hendrikw, 2014-11-11 23:46:44]

2nd part - remove picturepilebase

Sorry Dana, I know I said I wouldn't send you the next review, but my change
leaked into ui/compositor and Enne isn't an owner there.

[vmpstr, 2014-11-11 23:57:08]

https://codereview.chromium.org/714203006/diff/20001/cc/cc.gyp
File cc/cc.gyp (left):

https://codereview.chromium.org/714203006/diff/20001/cc/cc.gyp#oldcode385
cc/cc.gyp:385: 'resources/picture_pile_base.cc',
.gyp changes should have corresponding BUILD.gn changes.

https://codereview.chromium.org/714203006/diff/20001/cc/debug/picture_record_...
File cc/debug/picture_record_benchmark.cc (right):

https://codereview.chromium.org/714203006/diff/20001/cc/debug/picture_record_...
cc/debug/picture_record_benchmark.cc:16: #include
"third_party/skia/include/core/skBBHFactory.h"
why this include?

https://codereview.chromium.org/714203006/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/714203006/diff/20001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:12: #include "base/lazy_instance.h"
I'm not sure why this is needed? Was there some sequence of includes that is no
longer here that caused this to be included?

[hendrikw, 2014-11-12 00:02:34]

PTAL

https://codereview.chromium.org/714203006/diff/20001/cc/cc.gyp
File cc/cc.gyp (left):

https://codereview.chromium.org/714203006/diff/20001/cc/cc.gyp#oldcode385
cc/cc.gyp:385: 'resources/picture_pile_base.cc',
On 2014/11/11 23:57:07, vmpstr wrote:
> .gyp changes should have corresponding BUILD.gn changes.

Acknowledged.

https://codereview.chromium.org/714203006/diff/20001/cc/debug/picture_record_...
File cc/debug/picture_record_benchmark.cc (right):

https://codereview.chromium.org/714203006/diff/20001/cc/debug/picture_record_...
cc/debug/picture_record_benchmark.cc:16: #include
"third_party/skia/include/core/skBBHFactory.h"
On 2014/11/11 23:57:08, vmpstr wrote:
> why this include?

Since I removed includes in other locations, I need includes in new locations

https://codereview.chromium.org/714203006/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/714203006/diff/20001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:12: #include "base/lazy_instance.h"
On 2014/11/11 23:57:08, vmpstr wrote:
> I'm not sure why this is needed? Was there some sequence of includes that is
no
> longer here that caused this to be included?

Same as previous comment.

[vmpstr, 2014-11-12 19:19:00]

https://codereview.chromium.org/714203006/diff/40001/cc/debug/picture_record_...
File cc/debug/picture_record_benchmark.cc (right):

https://codereview.chromium.org/714203006/diff/40001/cc/debug/picture_record_...
cc/debug/picture_record_benchmark.cc:16: #include
"third_party/skia/include/core/skBBHFactory.h"
This file doesn't exist (capitalization and all that :) )

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pile.h
File cc/resources/picture_pile.h (left):

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.h:18: class CC_EXPORT PicturePile : public
PicturePileBase, public RecordingSource {
\o/

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pile.h
File cc/resources/picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.h:32: gfx::Size GetSize() const final;
Why final?

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.h:33: void SetEmptyBounds() override;
Can you group all of the overrides/finals together and put a comment saying
"RecordingSource overrides" for easy identification.

https://codereview.chromium.org/714203006/diff/40001/cc/test/test_picture_pile.h
File cc/test/test_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/40001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:51: }
  // namespace cc

[hendrikw, 2014-11-12 19:37:33]

PTAL

https://codereview.chromium.org/714203006/diff/40001/cc/debug/picture_record_...
File cc/debug/picture_record_benchmark.cc (right):

https://codereview.chromium.org/714203006/diff/40001/cc/debug/picture_record_...
cc/debug/picture_record_benchmark.cc:16: #include
"third_party/skia/include/core/skBBHFactory.h"
On 2014/11/12 19:18:57, vmpstr wrote:
> This file doesn't exist (capitalization and all that :) )

Oh windows, why do you have to be so lenient :(

Thanks!  fixed.

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pile.h
File cc/resources/picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.h:32: gfx::Size GetSize() const final;
On 2014/11/12 19:18:59, vmpstr wrote:
> Why final?

Early optimization :)  It really doesn't matter, but we call this a bunch of
times within the class, I can remove it, was playing around.

https://codereview.chromium.org/714203006/diff/40001/cc/resources/picture_pil...
cc/resources/picture_pile.h:33: void SetEmptyBounds() override;
On 2014/11/12 19:18:59, vmpstr wrote:
> Can you group all of the overrides/finals together and put a comment saying
> "RecordingSource overrides" for easy identification.

Yes!

https://codereview.chromium.org/714203006/diff/40001/cc/test/test_picture_pile.h
File cc/test/test_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/40001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:51: }
On 2014/11/12 19:18:59, vmpstr wrote:
>   // namespace cc

Done. windows presubmit fail!

[vmpstr, 2014-11-12 19:41:31]

lgtm % danakj

[danakj, 2014-11-12 19:56:39]

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pile.h
File cc/resources/picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile.h:22: // RecordingSource overrides
add a period

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.h (left):

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.h:28: static scoped_refptr<PicturePileImpl>
CreateFromOther(
Can we be consistent about using Create() vs constructors to make this class.
Please keep this or remove both Create functions (probably keep this for this
CL).

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pile.h
File cc/test/test_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:12: class TestPicturePile : public PicturePile {
I know this class already existed (and that we have some TestFoo classes
already, regrettably) but since it's being promoted to top-level class here, can
you please rename this to FakePicturePile? "Test" as a prefix is not meaningful,
and we're trying to do better than that now. Fake refers to an implementation
that can override things or change behaviours etc.

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:45: void SetPixelRecordDistanceForTesting(int d) {
pixel_record_distance_ = d; }
remove ForTesting in a test class

https://codereview.chromium.org/714203006/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/714203006/diff/60001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:12: #include "base/lazy_instance.h"
why is this part of this CL?

[hendrikw, 2014-11-12 20:25:42]

PTAL

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pile.h
File cc/resources/picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile.h:22: // RecordingSource overrides
On 2014/11/12 19:56:38, danakj wrote:
> add a period

Acknowledged.

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.h (left):

https://codereview.chromium.org/714203006/diff/60001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.h:28: static scoped_refptr<PicturePileImpl>
CreateFromOther(
On 2014/11/12 19:56:38, danakj wrote:
> Can we be consistent about using Create() vs constructors to make this class.
> Please keep this or remove both Create functions (probably keep this for this
> CL).

Acknowledged.

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pile.h
File cc/test/test_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:12: class TestPicturePile : public PicturePile {
On 2014/11/12 19:56:38, danakj wrote:
> I know this class already existed (and that we have some TestFoo classes
> already, regrettably) but since it's being promoted to top-level class here,
can
> you please rename this to FakePicturePile? "Test" as a prefix is not
meaningful,
> and we're trying to do better than that now. Fake refers to an implementation
> that can override things or change behaviours etc.

Acknowledged.

https://codereview.chromium.org/714203006/diff/60001/cc/test/test_picture_pil...
cc/test/test_picture_pile.h:45: void SetPixelRecordDistanceForTesting(int d) {
pixel_record_distance_ = d; }
On 2014/11/12 19:56:38, danakj wrote:
> remove ForTesting in a test class

Acknowledged.

https://codereview.chromium.org/714203006/diff/60001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/714203006/diff/60001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:12: #include "base/lazy_instance.h"
On 2014/11/12 19:56:38, danakj wrote:
> why is this part of this CL?

this includes a file that includes a file that includes lazy_instance.h, and I
removed an include from one of those intermediate includes

[danakj, 2014-11-12 20:33:37]

LGTM

https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile.cc (right):

https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile.cc:638: return
scoped_refptr<RasterSource>(PicturePileImpl::CreateFromOther(this));
I don't think that you need to cast it like this?

class A : public base::RefCounted<A> {
 protected:
  friend class base::RefCounted<A>;
  virtual ~A() {}
};
class B : public A {
 protected:
  ~B() override {}
};

scoped_refptr<A> CreateA() {
  return scoped_refptr<B>(new B);
}

This compiles for me.

[vmpstr, 2014-11-12 20:36:34]

https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
cc/resources/picture_pile_impl.h:27: static scoped_refptr<PicturePileImpl>
CreateFromOther(
Since we're going with a Create* function, can you change the name to something
else? It's not clear what "other" refers to. Maybe CreateFromPicturePile?

[hendrikw, 2014-11-12 21:10:55]

On 2014/11/12 20:36:34, vmpstr wrote:
>
https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
> File cc/resources/picture_pile_impl.h (right):
> 
>
https://codereview.chromium.org/714203006/diff/100001/cc/resources/picture_pi...
> cc/resources/picture_pile_impl.h:27: static scoped_refptr<PicturePileImpl>
> CreateFromOther(
> Since we're going with a Create* function, can you change the name to
something
> else? It's not clear what "other" refers to. Maybe CreateFromPicturePile?

Agreed, done!

[hendrikw, 2014-11-12 21:11:13]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-11-12 21:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714203006/120001

[commit-bot: I haz the power, 2014-11-12 21:18:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 21:18:15]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2014-11-12 21:30:49]

https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
btw 2014

[danakj, 2014-11-12 21:31:16]

https://codereview.chromium.org/714203006/diff/120001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/714203006/diff/120001/cc/BUILD.gn#newcode544
cc/BUILD.gn:544: "test/fake_picture_pile.h"
missing a ,

[danakj, 2014-11-12 21:32:19]

https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:5: #ifndef CC_FAKE_PICTURE_PILE_H_
oh and CC_TEST_FAKE_PICTURE_PILE_H

you want the full path in there.

seeing all the diffs between your version and mine of this file when i rebase
onto this :)

[hendrikw, 2014-11-12 21:53:12]

On 2014/11/12 21:32:19, danakj wrote:
>
https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
> File cc/test/fake_picture_pile.h (right):
> 
>
https://codereview.chromium.org/714203006/diff/120001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:5: #ifndef CC_FAKE_PICTURE_PILE_H_
> oh and CC_TEST_FAKE_PICTURE_PILE_H
> 
> you want the full path in there.
> 
> seeing all the diffs between your version and mine of this file when i rebase
> onto this :)

Ouch.  Good thing I missed the comma.

[hendrikw, 2014-11-12 21:53:48]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-11-12 21:54:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714203006/140001

[commit-bot: I haz the power, 2014-11-12 23:24:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-12 23:25:27]

Patchset 8 (id:??) landed as
https://crrev.com/312ee8acb412671d98f3c1e7c0fc0304dc9b7fea
Cr-Commit-Position: refs/heads/master@{#303919}

[tfarina, 2014-11-13 15:33:43]

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:30: bool HasRecordings() const { return
has_any_recordings_; }
bool has_any_recordings() const { .. }
+
void set_has_any_recordings() { ... }

?

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:32: void SetRecordedViewport(const gfx::Rect&
viewport) {
set_recorded_viewport?

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:42: bool is_solid_color() const { return
is_solid_color_; }
in c++11 you don't need to initialize this member variable in the constructor
anymore? Does it get set to false automatically? Sorry, I don't remember.
Haven't written C++ for some time now.

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:45: void SetPixelRecordDistance(int d) {
pixel_record_distance_ = d; }
set_ ....?

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:47: typedef PicturePile::PictureInfo PictureInfo;
these typedefs should be before the constructor, all above.

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:50: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:51: }  // namespace cc
add empty line above?

[tfarina, 2014-11-13 15:35:19]

Also, you could have assigned this to BUG=430260 in the CL description. ;)

[hendrikw, 2014-11-13 16:37:02]

On 2014/11/13 15:33:43, tfarina wrote:
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> File cc/test/fake_picture_pile.h (right):
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:30: bool HasRecordings() const { return
> has_any_recordings_; }
> bool has_any_recordings() const { .. }
> +
> void set_has_any_recordings() { ... }
> 
> ?
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:32: void SetRecordedViewport(const gfx::Rect&
> viewport) {
> set_recorded_viewport?
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:42: bool is_solid_color() const { return
> is_solid_color_; }
> in c++11 you don't need to initialize this member variable in the constructor
> anymore? Does it get set to false automatically? Sorry, I don't remember.
> Haven't written C++ for some time now.
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:45: void SetPixelRecordDistance(int d) {
> pixel_record_distance_ = d; }
> set_ ....?
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:47: typedef PicturePile::PictureInfo PictureInfo;
> these typedefs should be before the constructor, all above.
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:50: };
> DISALLOW_COPY_AND_ASSIGN?
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:51: }  // namespace cc
> add empty line above?

I suppose I could have cleaned up the class, that class was moved, I didn't
write it.

Yes, you must still initialize members in c++11 :)  Though that member comes
from the base class.

[danakj, 2014-11-13 16:59:13]

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
File cc/test/fake_picture_pile.h (right):

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:30: bool HasRecordings() const { return
has_any_recordings_; }
On 2014/11/13 15:33:42, tfarina wrote:
> bool has_any_recordings() const { .. }
> +
> void set_has_any_recordings() { ... }
> 
> ?

Meh... they could be formatted that way, but a) this is legal and b) this is
test code. So I don't feel strongly either way.

https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
cc/test/fake_picture_pile.h:42: bool is_solid_color() const { return
is_solid_color_; }
On 2014/11/13 15:33:42, tfarina wrote:
> in c++11 you don't need to initialize this member variable in the constructor
> anymore? Does it get set to false automatically? Sorry, I don't remember.
> Haven't written C++ for some time now.

You do need to initialize, looks like that is missing from the base class.

[hendrikw, 2014-11-13 17:36:30]

On 2014/11/13 16:59:13, danakj wrote:
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> File cc/test/fake_picture_pile.h (right):
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:30: bool HasRecordings() const { return
> has_any_recordings_; }
> On 2014/11/13 15:33:42, tfarina wrote:
> > bool has_any_recordings() const { .. }
> > +
> > void set_has_any_recordings() { ... }
> > 
> > ?
> 
> Meh... they could be formatted that way, but a) this is legal and b) this is
> test code. So I don't feel strongly either way.
> 
>
https://codereview.chromium.org/714203006/diff/140001/cc/test/fake_picture_pi...
> cc/test/fake_picture_pile.h:42: bool is_solid_color() const { return
> is_solid_color_; }
> On 2014/11/13 15:33:42, tfarina wrote:
> > in c++11 you don't need to initialize this member variable in the
constructor
> > anymore? Does it get set to false automatically? Sorry, I don't remember.
> > Haven't written C++ for some time now.
> 
> You do need to initialize, looks like that is missing from the base class.

PicturePile initializes is_solid_color_ and solid_color_.

background_color_ was always missing, but that's set in my other cl.