Move PixelRefMap and its Iterator out from Picture class

Move PixelRefMap and its Iterator out from Picture class

Factor out PixelRefMap and the values used with it out from Picture
class. Also move the PixelRefIterator on Picture level out from Picture
class. TileGridSize is now only used to decide cell size when gather
pixel refs.

R=vmpstr, ajuma
BUG=440468
Committed: https://crrev.com/7e508ddb57205b19b3d28f1e23b6883caa804c66
Cr-Commit-Position: refs/heads/master@{#319909}

[ajuma, 2015-02-11 15:29:04]

lgtm if vmpstr's happy with it too.

https://codereview.chromium.org/909353002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/909353002/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:178: // TODO(mtklein): If SkRTree sticks, clean up
tile_grid_size.  skbug.com/3085
Can this TODO be deleted now that TileGridInfo no longer exists?

https://codereview.chromium.org/909353002/diff/1/cc/resources/pixel_ref_map.h
File cc/resources/pixel_ref_map.h (right):

https://codereview.chromium.org/909353002/diff/1/cc/resources/pixel_ref_map.h...
cc/resources/pixel_ref_map.h:29: // refs which would happen after record. It
takes in |tile_grid_size| to decide
s/"tile_grid_size"/"cell_size"?

https://codereview.chromium.org/909353002/diff/1/cc/resources/pixel_ref_map.h...
cc/resources/pixel_ref_map.h:30: // how big each grid should be.
s/"each grid"/"each grid cell"?

[weiliangc, 2015-02-18 16:50:53]

New patchsets have been uploaded after l-g-t-m from ajuma@chromium.org

[weiliangc, 2015-03-05 16:40:10]

vmpstr@ ping

[vmpstr, 2015-03-05 19:31:44]

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
File cc/resources/pixel_ref_map.h (right):

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:31: class CC_EXPORT PixelRefMap : public
base::hash_map<PixelRefMapKey, PixelRefs> {
I find this kinda iffy. Is it difficult to have a private hash map instead and
provide some accessor functions that base::hash_map would provide? I don't think
we use too many of those, do we? 

The problem is that it would be possible to implicitly convert this to a
base::hash_map pointer (suppose someone assumes incorrectly that PixelRefMap is
a typedef to base::hash_map), and then destruction would yield UD, since I don't
think base::hash_map has a virtual dtor.

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:48: class CC_EXPORT PixelRefMapIterator {
This can also be PixelRefMap::Iterator, which might be more explicit in what it
iterates? Up to you though.

[weiliangc, 2015-03-09 18:50:21]

Updated according to review comments. PTAL.

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
File cc/resources/pixel_ref_map.h (right):

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:31: class CC_EXPORT PixelRefMap : public
base::hash_map<PixelRefMapKey, PixelRefs> {
On 2015/03/05 at 19:31:44, vmpstr wrote:
> I find this kinda iffy. Is it difficult to have a private hash map instead and
provide some accessor functions that base::hash_map would provide? I don't think
we use too many of those, do we? 
> 
> The problem is that it would be possible to implicitly convert this to a
base::hash_map pointer (suppose someone assumes incorrectly that PixelRefMap is
a typedef to base::hash_map), and then destruction would yield UD, since I don't
think base::hash_map has a virtual dtor.

Accessing hash_map is way less often than I expected. Thanks for the point about
desctructor!

Done.

https://codereview.chromium.org/909353002/diff/40001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:48: class CC_EXPORT PixelRefMapIterator {
On 2015/03/05 at 19:31:44, vmpstr wrote:
> This can also be PixelRefMap::Iterator, which might be more explicit in what
it iterates? Up to you though.

Moved to PixelRefMap::Iterator.

Before when PixelRefMap inherits hash_map compiler would complain about
::Iterator. So it works out.

[vmpstr, 2015-03-09 19:16:11]

lgtm with a couple of naming nits

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
File cc/resources/pixel_ref_map.h (right):

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:68: const PixelRefMap* map_;
nit: Can you rename this to something more descriptive?

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:85: PixelRefHashmap map_;
nit: Same here.

[weiliangc, 2015-03-10 14:26:40]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-03-10 14:26:41]

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
File cc/resources/pixel_ref_map.h (right):

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:68: const PixelRefMap* map_;
On 2015/03/09 at 19:16:11, vmpstr wrote:
> nit: Can you rename this to something more descriptive?

Done.

https://codereview.chromium.org/909353002/diff/60001/cc/resources/pixel_ref_m...
cc/resources/pixel_ref_map.h:85: PixelRefHashmap map_;
On 2015/03/09 at 19:16:11, vmpstr wrote:
> nit: Same here.

Done.

[weiliangc, 2015-03-10 14:26:41]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/909353002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-10 14:27:04]

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

[commit-bot: I haz the power, 2015-03-10 17:15:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-10 17:18:06]

Patchset 7 (id:??) landed as
https://crrev.com/7e508ddb57205b19b3d28f1e23b6883caa804c66
Cr-Commit-Position: refs/heads/master@{#319909}