cc: Move UI Resource management out of LayerTreeHost.

cc: Move UI Resource management out of LayerTreeHost.

Moves out the UIResource request tracking into its own class composed
into the LayerTreeHost. The code is pretty isolated and doesn't need
to live in the host.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8ec0740f51368ce84fd4ad5cd5ba9947f47c96cf
Cr-Commit-Position: refs/heads/master@{#417812}

[Khushal, 2016-09-08 20:08:56]

Description was changed from

==========
cc: Move UI Resource management out of LayerTreeHost.

Moves out the UIResource request tracking into its own class composed
into the LayerTreeHost. The code is pretty isolated and doesn't need
to live in the host.

BUG=625283
==========

to

==========
cc: Move UI Resource management out of LayerTreeHost.

Moves out the UIResource request tracking into its own class composed
into the LayerTreeHost. The code is pretty isolated and doesn't need
to live in the host.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-09-08 20:11:00]

khushalsagar@chromium.org changed reviewers:
+ aelias@chromium.org

[Khushal, 2016-09-08 20:11:01]

I still have to update the ResourceManagerImplTest but otherwise this is done.
PTAL.

[danakj, 2016-09-08 20:57:35]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-09-08 20:57:36]

Hey, I have a couple drive-by comments just on the interface additions to
LayerTree, which I want to avoid if we can. Oh and one C++11 thing I randomly
saw.

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer.cc:173: if (GetLayerTree()
Similarly, it could save this when SetLayerTreeHost happens to avoid adding
GetSettings()?

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:127: GetLayerTree()->GetUIResourceManager(),
bitmap_);
How about storing the UIResourceManager* in SetLayerTreeHost? Then you don't
need this accessor on LayerTree?

https://codereview.chromium.org/2322943003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:476:
sync_tree->set_ui_resource_request_queue(ui_resource_request_queue);
Why not just move the queue here? In fact why isnt this then just

sync_tree->set_ui_resource_request_queue(ui_resource_manager_->TakeUIResourcesRequests());
? And change it to receive by value and move, then we're not copying vectors
anywhere.

[danakj, 2016-09-08 20:58:46]

One more :)

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/composito...
ui/compositor/compositor.h:419: std::unique_ptr<cc::LayerTreeHostInterface>
host_;
It's always an inprocess LTH for ui::Compositor, so this probably doesn't need
to point at the abstract thing?

[Khushal, 2016-09-08 22:05:24]

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:127: GetLayerTree()->GetUIResourceManager(),
bitmap_);
On 2016/09/08 20:57:35, danakj wrote:
> How about storing the UIResourceManager* in SetLayerTreeHost? Then you don't
> need this accessor on LayerTree?

Finally the SetLayerTreeHost will change to SetLayerTree so Layers just talk to
the tree, keeping it somewhat consistent with the impl side where LayerImpls go
through the LayerTreeImpl for things they need from the host. Also helped me
refactor the code clearly so the use of LayerTreeHost is restricted to fewer
places, its much easier to see when something gets added whether it actually
needs to go on the host or the tree. WDYT?

https://codereview.chromium.org/2322943003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:476:
sync_tree->set_ui_resource_request_queue(ui_resource_request_queue);
On 2016/09/08 20:57:35, danakj wrote:
> Why not just move the queue here? In fact why isnt this then just
> 
>
sync_tree->set_ui_resource_request_queue(ui_resource_manager_->TakeUIResourcesRequests());
> ? And change it to receive by value and move, then we're not copying vectors
> anywhere.

Much cooler. Done.

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/composito...
ui/compositor/compositor.h:419: std::unique_ptr<cc::LayerTreeHostInterface>
host_;
On 2016/09/08 20:58:46, danakj wrote:
> It's always an inprocess LTH for ui::Compositor, so this probably doesn't need
> to point at the abstract thing?

This is probably not decided at all, but what's going to happen with MUS? Would
that have the UI using a LayerTreeHostMUS? Just curious.

Also, is there a reason that embedders should see the impl class? I just think
it helps keep things clean because what's internal to cc remains internal, if
someone wants to do something then they have to add it to the interface and its
obvious.

[danakj, 2016-09-08 22:37:13]

So ya, seems okay overall. aelias@ can review the details, thanks for explaining
for me.

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:127: GetLayerTree()->GetUIResourceManager(),
bitmap_);
On 2016/09/08 22:05:24, Khushal wrote:
> On 2016/09/08 20:57:35, danakj wrote:
> > How about storing the UIResourceManager* in SetLayerTreeHost? Then you don't
> > need this accessor on LayerTree?
> 
> Finally the SetLayerTreeHost will change to SetLayerTree so Layers just talk
to
> the tree, keeping it somewhat consistent with the impl side where LayerImpls
go
> through the LayerTreeImpl for things they need from the host. Also helped me
> refactor the code clearly so the use of LayerTreeHost is restricted to fewer
> places, its much easier to see when something gets added whether it actually
> needs to go on the host or the tree. WDYT?

I see, and since layers can change trees it wont make sense for the layer to be
given the uiresourcemanager at creation time. Or would it make sense to have the
UIResourceManager owned by the embedder and shared between all LayerTreeHosts?

I was a bit :/ about this new category of "pass thru" methods on LayerTree, but
actually reading the impl of LayerTree there are a lot of methods that are using
LTH already, like SetNeedsCommit for example. I think if you add new methods to
LayerTree they should just be part of the LayerTree API and not pointed out as
"pass thru" in comments, that's an implementation detail of how LayerTree works.

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/composito...
ui/compositor/compositor.h:419: std::unique_ptr<cc::LayerTreeHostInterface>
host_;
On 2016/09/08 22:05:24, Khushal wrote:
> On 2016/09/08 20:58:46, danakj wrote:
> > It's always an inprocess LTH for ui::Compositor, so this probably doesn't
need
> > to point at the abstract thing?
> 
> This is probably not decided at all, but what's going to happen with MUS?
Would
> that have the UI using a LayerTreeHostMUS? Just curious.

If MUS was just the DisplayCompositor, it would remain an inprocess LTH, and the
OutputSurface would go OOP like in the renderer.

If MUS was putting the whole impl-side out of process, I think we'd do it at the
proxy (thats how the prototype did it for MUS again). It's possible we could
replace the LTH too, but in the end it would become concretely the other type
then, I would say it's not clear and don't worry about planning around it in
this way.

> Also, is there a reason that embedders should see the impl class? I just think
> it helps keep things clean because what's internal to cc remains internal, if
> someone wants to do something then they have to add it to the interface and
its
> obvious.

Ya I think there is. Sometimes embedders need information/things that are
specific to them, but not part of the general API, and this allows it without
casting or something. Example:
https://cs.chromium.org/chromium/src/content/common/gpu/client/context_provid...
this doesn't need to be part of the general API since all users of it know the
concrete type.

[Khushal, 2016-09-08 23:56:06]

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
File cc/layers/ui_resource_layer.cc (right):

https://codereview.chromium.org/2322943003/diff/20001/cc/layers/ui_resource_l...
cc/layers/ui_resource_layer.cc:127: GetLayerTree()->GetUIResourceManager(),
bitmap_);
On 2016/09/08 22:37:12, danakj wrote:
> On 2016/09/08 22:05:24, Khushal wrote:
> > On 2016/09/08 20:57:35, danakj wrote:
> > > How about storing the UIResourceManager* in SetLayerTreeHost? Then you
don't
> > > need this accessor on LayerTree?
> > 
> > Finally the SetLayerTreeHost will change to SetLayerTree so Layers just talk
> to
> > the tree, keeping it somewhat consistent with the impl side where LayerImpls
> go
> > through the LayerTreeImpl for things they need from the host. Also helped me
> > refactor the code clearly so the use of LayerTreeHost is restricted to fewer
> > places, its much easier to see when something gets added whether it actually
> > needs to go on the host or the tree. WDYT?
> 
> I see, and since layers can change trees it wont make sense for the layer to
be
> given the uiresourcemanager at creation time. Or would it make sense to have
the
> UIResourceManager owned by the embedder and shared between all LayerTreeHosts?

I was mostly just moving code here so my understanding might not be completely
accurate but looks like the state that is tied to the host in the
UIResourceManager is requests for UI resources for layers tied to it, so if the
impl thread comes back and says, "I dropped all the resources, make the ones
that you want to use in this commit again", then something on the main thread
needs to be keeping track of those. I kind of don't want to change much logic in
this patch too, if its not necessary. Now that all of it is pulled out
seperately, perhaps it would be easier to change ownership of things if that
makes sense.

> 
> I was a bit :/ about this new category of "pass thru" methods on LayerTree,
but
> actually reading the impl of LayerTree there are a lot of methods that are
using
> LTH already, like SetNeedsCommit for example. I think if you add new methods
to
> LayerTree they should just be part of the LayerTree API and not pointed out as
> "pass thru" in comments, that's an implementation detail of how LayerTree
works.

Eeks, fair enough. There is a lot that's pass through already, so the comment is
just confusing. Removed.

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2322943003/diff/20001/ui/compositor/composito...
ui/compositor/compositor.h:419: std::unique_ptr<cc::LayerTreeHostInterface>
host_;
On 2016/09/08 22:37:12, danakj wrote:
> On 2016/09/08 22:05:24, Khushal wrote:
> > On 2016/09/08 20:58:46, danakj wrote:
> > > It's always an inprocess LTH for ui::Compositor, so this probably doesn't
> need
> > > to point at the abstract thing?
> > 
> > This is probably not decided at all, but what's going to happen with MUS?
> Would
> > that have the UI using a LayerTreeHostMUS? Just curious.
> 
> If MUS was just the DisplayCompositor, it would remain an inprocess LTH, and
the
> OutputSurface would go OOP like in the renderer.
> 
> If MUS was putting the whole impl-side out of process, I think we'd do it at
the
> proxy (thats how the prototype did it for MUS again). It's possible we could
> replace the LTH too, but in the end it would become concretely the other type
> then, I would say it's not clear and don't worry about planning around it in
> this way.

Okay, sounds good.

> 
> > Also, is there a reason that embedders should see the impl class? I just
think
> > it helps keep things clean because what's internal to cc remains internal,
if
> > someone wants to do something then they have to add it to the interface and
> its
> > obvious.
> 
> Ya I think there is. Sometimes embedders need information/things that are
> specific to them, but not part of the general API, and this allows it without
> casting or something. Example:
>
https://cs.chromium.org/chromium/src/content/common/gpu/client/context_provid...
> this doesn't need to be part of the general API since all users of it know the
> concrete type.

The pattern that I was seeing was that with the LayerTreeHost we avoided things
like that(for instance with stuff that's in threaded or single threaded mode
only) by adding it to the API and just commenting that it is not supported in
that mode (I looked at CompositeImmediately etc.) but if that's not necessary,
then that's awesome. Makes my change simpler. :D
Removed those bits.

[danakj, 2016-09-09 00:10:39]

On Thu, Sep 8, 2016 at 4:56 PM, <khushalsagar@chromium.org> wrote:

>
> https://codereview.chromium.org/2322943003/diff/20001/cc/
> layers/ui_resource_layer.cc
> File cc/layers/ui_resource_layer.cc (right):
>
> https://codereview.chromium.org/2322943003/diff/20001/cc/
> layers/ui_resource_layer.cc#newcode127
> cc/layers/ui_resource_layer.cc:127:
> GetLayerTree()->GetUIResourceManager(), bitmap_);
> On 2016/09/08 22:37:12, danakj wrote:
> > On 2016/09/08 22:05:24, Khushal wrote:
> > > On 2016/09/08 20:57:35, danakj wrote:
> > > > How about storing the UIResourceManager* in SetLayerTreeHost? Then
> you don't
> > > > need this accessor on LayerTree?
> > >
> > > Finally the SetLayerTreeHost will change to SetLayerTree so Layers
> just talk
> > to
> > > the tree, keeping it somewhat consistent with the impl side where
> LayerImpls
> > go
> > > through the LayerTreeImpl for things they need from the host. Also
> helped me
> > > refactor the code clearly so the use of LayerTreeHost is restricted
> to fewer
> > > places, its much easier to see when something gets added whether it
> actually
> > > needs to go on the host or the tree. WDYT?
> >
> > I see, and since layers can change trees it wont make sense for the
> layer to be
> > given the uiresourcemanager at creation time. Or would it make sense
> to have the
> > UIResourceManager owned by the embedder and shared between all
> LayerTreeHosts?
>
> I was mostly just moving code here so my understanding might not be
> completely accurate but looks like the state that is tied to the host in
> the UIResourceManager is requests for UI resources for layers tied to
> it, so if the impl thread comes back and says, "I dropped all the
> resources, make the ones that you want to use in this commit again",
> then something on the main thread needs to be keeping track of those. I
> kind of don't want to change much logic in this patch too, if its not
> necessary. Now that all of it is pulled out seperately, perhaps it would
> be easier to change ownership of things if that makes sense.
>
> >
> > I was a bit :/ about this new category of "pass thru" methods on
> LayerTree, but
> > actually reading the impl of LayerTree there are a lot of methods that
> are using
> > LTH already, like SetNeedsCommit for example. I think if you add new
> methods to
> > LayerTree they should just be part of the LayerTree API and not
> pointed out as
> > "pass thru" in comments, that's an implementation detail of how
> LayerTree works.
>
> Eeks, fair enough. There is a lot that's pass through already, so the
> comment is just confusing. Removed.
>
> https://codereview.chromium.org/2322943003/diff/20001/ui/
> compositor/compositor.h
> File ui/compositor/compositor.h (right):
>
> https://codereview.chromium.org/2322943003/diff/20001/ui/
> compositor/compositor.h#newcode419
> ui/compositor/compositor.h:419:
> std::unique_ptr<cc::LayerTreeHostInterface> host_;
> On 2016/09/08 22:37:12, danakj wrote:
> > On 2016/09/08 22:05:24, Khushal wrote:
> > > On 2016/09/08 20:58:46, danakj wrote:
> > > > It's always an inprocess LTH for ui::Compositor, so this probably
> doesn't
> > need
> > > > to point at the abstract thing?
> > >
> > > This is probably not decided at all, but what's going to happen with
> MUS?
> > Would
> > > that have the UI using a LayerTreeHostMUS? Just curious.
> >
> > If MUS was just the DisplayCompositor, it would remain an inprocess
> LTH, and the
> > OutputSurface would go OOP like in the renderer.
> >
> > If MUS was putting the whole impl-side out of process, I think we'd do
> it at the
> > proxy (thats how the prototype did it for MUS again). It's possible we
> could
> > replace the LTH too, but in the end it would become concretely the
> other type
> > then, I would say it's not clear and don't worry about planning around
> it in
> > this way.
>
> Okay, sounds good.
>
> >
> > > Also, is there a reason that embedders should see the impl class? I
> just think
> > > it helps keep things clean because what's internal to cc remains
> internal, if
> > > someone wants to do something then they have to add it to the
> interface and
> > its
> > > obvious.
> >
> > Ya I think there is. Sometimes embedders need information/things that
> are
> > specific to them, but not part of the general API, and this allows it
> without
> > casting or something. Example:
> >
> https://cs.chromium.org/chromium/src/content/common/
> gpu/client/context_provider_command_buffer.h?rcl=0&l=66
> > this doesn't need to be part of the general API since all users of it
> know the
> > concrete type.
>
> The pattern that I was seeing was that with the LayerTreeHost we avoided
> things like that(for instance with stuff that's in threaded or single
> threaded mode only) by adding it to the API and just commenting that it
> is not supported in that mode (I looked at CompositeImmediately etc.)
> but if that's not necessary, then that's awesome. Makes my change
> simpler. :D
>

Ya, because casting is terrible and LTH can have either. But if a thing
ever only had a SingleThreadProxy, I'd prefer it used that type instead of
a base Proxy.


> Removed those bits.
>
> https://codereview.chromium.org/2322943003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-09-09 00:10:41]

On Thu, Sep 8, 2016 at 4:56 PM, <khushalsagar@chromium.org> wrote:

>
> https://codereview.chromium.org/2322943003/diff/20001/cc/
> layers/ui_resource_layer.cc
> File cc/layers/ui_resource_layer.cc (right):
>
> https://codereview.chromium.org/2322943003/diff/20001/cc/
> layers/ui_resource_layer.cc#newcode127
> cc/layers/ui_resource_layer.cc:127:
> GetLayerTree()->GetUIResourceManager(), bitmap_);
> On 2016/09/08 22:37:12, danakj wrote:
> > On 2016/09/08 22:05:24, Khushal wrote:
> > > On 2016/09/08 20:57:35, danakj wrote:
> > > > How about storing the UIResourceManager* in SetLayerTreeHost? Then
> you don't
> > > > need this accessor on LayerTree?
> > >
> > > Finally the SetLayerTreeHost will change to SetLayerTree so Layers
> just talk
> > to
> > > the tree, keeping it somewhat consistent with the impl side where
> LayerImpls
> > go
> > > through the LayerTreeImpl for things they need from the host. Also
> helped me
> > > refactor the code clearly so the use of LayerTreeHost is restricted
> to fewer
> > > places, its much easier to see when something gets added whether it
> actually
> > > needs to go on the host or the tree. WDYT?
> >
> > I see, and since layers can change trees it wont make sense for the
> layer to be
> > given the uiresourcemanager at creation time. Or would it make sense
> to have the
> > UIResourceManager owned by the embedder and shared between all
> LayerTreeHosts?
>
> I was mostly just moving code here so my understanding might not be
> completely accurate but looks like the state that is tied to the host in
> the UIResourceManager is requests for UI resources for layers tied to
> it, so if the impl thread comes back and says, "I dropped all the
> resources, make the ones that you want to use in this commit again",
> then something on the main thread needs to be keeping track of those. I
> kind of don't want to change much logic in this patch too, if its not
> necessary. Now that all of it is pulled out seperately, perhaps it would
> be easier to change ownership of things if that makes sense.
>
> >
> > I was a bit :/ about this new category of "pass thru" methods on
> LayerTree, but
> > actually reading the impl of LayerTree there are a lot of methods that
> are using
> > LTH already, like SetNeedsCommit for example. I think if you add new
> methods to
> > LayerTree they should just be part of the LayerTree API and not
> pointed out as
> > "pass thru" in comments, that's an implementation detail of how
> LayerTree works.
>
> Eeks, fair enough. There is a lot that's pass through already, so the
> comment is just confusing. Removed.
>
> https://codereview.chromium.org/2322943003/diff/20001/ui/
> compositor/compositor.h
> File ui/compositor/compositor.h (right):
>
> https://codereview.chromium.org/2322943003/diff/20001/ui/
> compositor/compositor.h#newcode419
> ui/compositor/compositor.h:419:
> std::unique_ptr<cc::LayerTreeHostInterface> host_;
> On 2016/09/08 22:37:12, danakj wrote:
> > On 2016/09/08 22:05:24, Khushal wrote:
> > > On 2016/09/08 20:58:46, danakj wrote:
> > > > It's always an inprocess LTH for ui::Compositor, so this probably
> doesn't
> > need
> > > > to point at the abstract thing?
> > >
> > > This is probably not decided at all, but what's going to happen with
> MUS?
> > Would
> > > that have the UI using a LayerTreeHostMUS? Just curious.
> >
> > If MUS was just the DisplayCompositor, it would remain an inprocess
> LTH, and the
> > OutputSurface would go OOP like in the renderer.
> >
> > If MUS was putting the whole impl-side out of process, I think we'd do
> it at the
> > proxy (thats how the prototype did it for MUS again). It's possible we
> could
> > replace the LTH too, but in the end it would become concretely the
> other type
> > then, I would say it's not clear and don't worry about planning around
> it in
> > this way.
>
> Okay, sounds good.
>
> >
> > > Also, is there a reason that embedders should see the impl class? I
> just think
> > > it helps keep things clean because what's internal to cc remains
> internal, if
> > > someone wants to do something then they have to add it to the
> interface and
> > its
> > > obvious.
> >
> > Ya I think there is. Sometimes embedders need information/things that
> are
> > specific to them, but not part of the general API, and this allows it
> without
> > casting or something. Example:
> >
> https://cs.chromium.org/chromium/src/content/common/
> gpu/client/context_provider_command_buffer.h?rcl=0&l=66
> > this doesn't need to be part of the general API since all users of it
> know the
> > concrete type.
>
> The pattern that I was seeing was that with the LayerTreeHost we avoided
> things like that(for instance with stuff that's in threaded or single
> threaded mode only) by adding it to the API and just commenting that it
> is not supported in that mode (I looked at CompositeImmediately etc.)
> but if that's not necessary, then that's awesome. Makes my change
> simpler. :D
>

Ya, because casting is terrible and LTH can have either. But if a thing
ever only had a SingleThreadProxy, I'd prefer it used that type instead of
a base Proxy.


> Removed those bits.
>
> https://codereview.chromium.org/2322943003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Khushal, 2016-09-09 00:14:09]

khushalsagar@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Khushal, 2016-09-09 00:14:10]

+dtrainor for ui

[David Trainor- moved to gerrit, 2016-09-09 05:51:05]

ui/ lgtm :)

[Khushal, 2016-09-09 19:47:18]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:48:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 20:08:42]

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

[commit-bot: I haz the power, 2016-09-09 20:08:46]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Khushal, 2016-09-09 20:42:37]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 20:43:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 23:06:10]

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

[commit-bot: I haz the power, 2016-09-09 23:06:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aelias_OOO_until_Jul13, 2016-09-09 23:53:34]

lgtm modulo minor comment, thanks for the cleanup.

https://codereview.chromium.org/2322943003/diff/120001/cc/resources/ui_resour...
File cc/resources/ui_resource_manager.h (right):

https://codereview.chromium.org/2322943003/diff/120001/cc/resources/ui_resour...
cc/resources/ui_resource_manager.h:41: std::vector<UIResourceRequest>
TakeUIResourcesRequests();
This new method is CC-facing so it should be demarcated in some way.  In some
cases we create pure virtual "*Delegate" superclasses for this kind of thing,
although that would probably be overengineering here.  I'd suggest having a
comment wall (//-------...) and an explanation of what this does.

[Khushal, 2016-09-10 00:11:22]

Thanks Alex.

https://codereview.chromium.org/2322943003/diff/120001/cc/resources/ui_resour...
File cc/resources/ui_resource_manager.h (right):

https://codereview.chromium.org/2322943003/diff/120001/cc/resources/ui_resour...
cc/resources/ui_resource_manager.h:41: std::vector<UIResourceRequest>
TakeUIResourcesRequests();
On 2016/09/09 23:53:34, aelias wrote:
> This new method is CC-facing so it should be demarcated in some way.  In some
> cases we create pure virtual "*Delegate" superclasses for this kind of thing,
> although that would probably be overengineering here.  I'd suggest having a
> comment wall (//-------...) and an explanation of what this does.

Done.

[Khushal, 2016-09-10 00:11:37]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-09-10 00:11:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2322943003/#ps180001
(title: "move the other comment")

[commit-bot: I haz the power, 2016-09-10 00:12:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Khushal, 2016-09-10 01:31:29]

The CQ bit was unchecked by khushalsagar@chromium.org

[Khushal, 2016-09-10 01:33:54]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-09-10 01:33:55]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2322943003/#ps200001
(title: "virtual dtor")

[commit-bot: I haz the power, 2016-09-10 01:34:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 03:16:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-10 03:20:04]

Description was changed from

==========
cc: Move UI Resource management out of LayerTreeHost.

Moves out the UIResource request tracking into its own class composed
into the LayerTreeHost. The code is pretty isolated and doesn't need
to live in the host.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Move UI Resource management out of LayerTreeHost.

Moves out the UIResource request tracking into its own class composed
into the LayerTreeHost. The code is pretty isolated and doesn't need
to live in the host.

BUG=625283
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/8ec0740f51368ce84fd4ad5cd5ba9947f47c96cf
Cr-Commit-Position: refs/heads/master@{#417812}
==========

[commit-bot: I haz the power, 2016-09-10 03:20:05]

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