cc: Remove tile ref counting in tile manager.

cc/resources/tile_manager.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CC_RESOURCES_TILE_MANAGER_H_
 #define CC_RESOURCES_TILE_MANAGER_H_
 
 #include <queue>
 #include <set>
 #include <vector>
@@ skipping 93 matching lines @@
   void UnregisterTile(Tile* tile);
 
   // Overriden from RasterWorkerPoolClient:
   virtual bool ShouldForceTasksRequiredForActivationToComplete() const
       OVERRIDE;
 
   // Virtual for test
   virtual void ScheduleTasks();
 
  private:
+  typedef uint64 ManagedTileId;

[reveman, 2013/07/03 00:32:47]

can you move this to the Tile class instead? Tile::TileId.

[vmpstr, 2013/07/03 19:08:04]

I added a todo here. Right now doing this would cause a circular dependency
between tile_manager.h and tile.h. This should be fixed shortly, and then I will
do what you're suggesting.

[reveman, 2013/07/03 20:20:06]

Ah, lets just make sure this is fixed before landing this patch.

[vmpstr, 2013/07/03 23:47:51]

Agreed.

   void OnImageDecodeTaskCompleted(
       int layer_id,
       skia::LazyPixelRef* pixel_ref,
       bool was_canceled);
   void OnRasterTaskCompleted(
-      scoped_refptr<Tile> tile,
+      ManagedTileId tile,
       scoped_ptr<ResourcePool::Resource> resource,
       RasterMode raster_mode,
       const PicturePileImpl::Analysis& analysis,
       bool was_canceled);
 
   void AssignBinsToTiles();
   void SortTiles();
   RasterMode DetermineRasterMode(const Tile* tile) const;
   void CleanUpUnusedImageDecodeTasks();
   void AssignGpuMemoryToTiles();
@@ skipping 10 matching lines @@
                             WhichTree tree);
   scoped_ptr<base::Value> GetMemoryRequirementsAsValue() const;
   void AddRequiredTileForActivation(Tile* tile);
 
   TileManagerClient* client_;
   scoped_ptr<ResourcePool> resource_pool_;
   scoped_ptr<RasterWorkerPool> raster_worker_pool_;
   GlobalStateThatImpactsTilePriority global_state_;
 
   typedef std::vector<Tile*> TileVector;
   TileVector tiles_;

[reveman, 2013/07/03 00:32:47]

I think we can remove |tiles_|. It's just local to ManageTiles().

[vmpstr, 2013/07/03 19:08:04]

It's also used in DispatchMoreTasks (this avoids an extra sort by priority). I
agree that it should be possible to remove this, but I think this requires a bit
of thought and code rework not to regress on efficiency. Are you ok with this
being a follow-up?

[reveman, 2013/07/03 20:20:06]

I think we need include some of it in this patch. How about we keep this vector
but instead of erasing elements from it we just mark it as invalid in
Register/UnregisterTile?

[vmpstr, 2013/07/03 23:47:51]

Done.

   TileVector tiles_that_need_to_be_rasterized_;

[reveman, 2013/07/03 00:32:47]

|tiles_that_need_to_be_rasterized_| definitely doesn't have to be a member
variable. Do you mind fixing that as part of this patch?

[vmpstr, 2013/07/03 19:08:04]

Done.

   typedef std::set<Tile*> TileSet;
   TileSet tiles_that_need_to_be_initialized_for_activation_;

[reveman, 2013/07/03 00:32:47]

fyi, I think we can remove this once my "ready to activate" patch lands.

In general, we shouldn't store any "Tile*" after we add this TileMap. Preferably
existing "Tile*" containers are removed. The ones that can't be removed should
store Tile::TileId instead. 

[vmpstr, 2013/07/03 19:08:04]

Sounds good. I think we would have to rethink the flow of how we throttle things
based on available memory, but I think this is work worth doing

[reveman, 2013/07/03 20:20:06]

I think it's trivial to replace
|tiles_that_need_to_be_initialized_for_activation_| with a simple "bool
tiles_need_to_be_initialized_for_activation_" now that my patch has landed. We
can do that as part of this patch or land it in a separate patch before this.

[vmpstr, 2013/07/03 23:47:51]

I'd rather do this separately. Do you mind putting up a patch, if you have some
time?

 
   bool ever_exceeded_memory_budget_;
   MemoryHistory::Entry memory_stats_from_last_assign_;
 
   RenderingStatsInstrumentation* rendering_stats_instrumentation_;
 
   bool did_initialize_visible_tile_;
 
   GLenum texture_format_;
 
   typedef base::hash_map<uint32_t, RasterWorkerPool::Task> PixelRefTaskMap;
   typedef base::hash_map<int, PixelRefTaskMap> LayerPixelRefTaskMap;
   LayerPixelRefTaskMap image_decode_tasks_;
 
+  typedef base::hash_map<ManagedTileId, Tile*> ManagedTileMap;

[reveman, 2013/07/03 00:32:47]

I like naming these based on the value type. How about just TileMap?

[vmpstr, 2013/07/03 19:08:04]

Done.

+  ManagedTileMap managed_tile_map_;

[reveman, 2013/07/03 00:32:47]

we usually don't use the type in the name for these containers. How about
"TileMap managed_tiles_"? Or maybe just |tiles_| after removing the old vector
with this name.

[vmpstr, 2013/07/03 19:08:04]

Named it managed_tiles_ for now.

+
   DISALLOW_COPY_AND_ASSIGN(TileManager);
 };
 
 }  // namespace cc
 
 #endif  // CC_RESOURCES_TILE_MANAGER_H_