Add mouse input forwarding to gpu service

Add mouse input forwarding to gpu service

Required for the SubscribeUniform WebGL extension.

BUG=422978
Committed: https://crrev.com/33edd848118675b2042df9c74e470bbe33c93d83
Cr-Commit-Position: refs/heads/master@{#306555}

[orglofch, 2014-10-11 01:48:25]

Patchset #2 (id:20001) has been deleted

[orglofch, 2014-10-11 03:08:08]

Patchset #2 (id:130001) has been deleted

[orglofch, 2014-10-11 03:18:36]

Patchset #2 (id:240001) has been deleted

[orglofch, 2014-10-11 03:20:56]

orglofch@chromium.org changed reviewers:
+ kbr@chromium.org, sievers@chromium.org, tsepez@chromium.org

[orglofch, 2014-10-11 03:20:57]

ptal.

@sievers I just fixed a typo in passing

https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:977: "HandleInputEventMain");
Just fixed a typo.

[orglofch, 2014-10-11 03:21:10]

orglofch@chromium.org changed reviewers:
+ bajones@chromium.org

[jdduke (slow), 2014-10-11 18:12:58]

On 2014/10/11 03:20:57, orglofch wrote:
> ptal.
> 
> @sievers I just fixed a typo in passing
> 
>
https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
> content/renderer/render_widget.cc:977: "HandleInputEventMain");
> Just fixed a typo.

Not lgtm. What exactly are you trying to accomplish here? Sending IPCs is not
cheap on Android; doing so just to log an event remotely is not justifiable.

[orglofch, 2014-10-13 16:47:22]

On 2014/10/11 18:12:58, jdduke wrote:
> On 2014/10/11 03:20:57, orglofch wrote:
> > ptal.
> > 
> > @sievers I just fixed a typo in passing
> > 
> >
>
https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/634313002/diff/1/content/renderer/render_widg...
> > content/renderer/render_widget.cc:977: "HandleInputEventMain");
> > Just fixed a typo.
> 
> Not lgtm. What exactly are you trying to accomplish here? Sending IPCs is not
> cheap on Android; doing so just to log an event remotely is not justifiable.

I've attached a bug which describes the motivation. 
I'll also add a patch to hide the feature behind a flag in case this incurs a
significant performance penalty.

[jdduke (slow), 2014-10-13 17:09:14]

On 2014/10/13 16:47:22, orglofch wrote:
> On 2014/10/11 18:12:58, jdduke wrote:
> > Not lgtm. What exactly are you trying to accomplish here? Sending IPCs is
not
> > cheap on Android; doing so just to log an event remotely is not justifiable.
> 
> I've attached a bug which describes the motivation. 
> I'll also add a patch to hide the feature behind a flag in case this incurs a
> significant performance penalty.

Thanks. Yes, a flag will be necessary for the time being.

[jdduke (slow), 2014-10-13 17:14:45]

https://codereview.chromium.org/634313002/diff/320001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/634313002/diff/320001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:419:
GpuProcessHost::GPU_PROCESS_KIND_UNSANDBOXED,
Is the GPU service interested in all event types? Even keyboard events? 

Also, even behind a flag, I'm not sure we'd ever want to always blast events
like this unless we had an explicit "subscription" signal from the render
process (pointer lock?). Perhaps someday we can use a similar fast path for
scrolling the page directly in the GPU process...

[orglofch, 2014-10-13 17:30:26]

https://codereview.chromium.org/634313002/diff/320001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/634313002/diff/320001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:419:
GpuProcessHost::GPU_PROCESS_KIND_UNSANDBOXED,
On 2014/10/13 17:14:45, jdduke wrote:
> Is the GPU service interested in all event types? Even keyboard events? 
> 
> Also, even behind a flag, I'm not sure we'd ever want to always blast events
> like this unless we had an explicit "subscription" signal from the render
> process (pointer lock?). Perhaps someday we can use a similar fast path for
> scrolling the page directly in the GPU process...

Only mouse events right now. I can't think of a use case for keyboard events but
I figured that one may arise. I'll filter these for now.
I'll also look into adding a subscription signal

[orglofch, 2014-11-06 23:03:35]

Patchset #4 (id:520001) has been deleted

[orglofch, 2014-11-06 23:03:51]

Patchset #3 (id:500001) has been deleted

[orglofch, 2014-11-08 01:11:50]

Patchset #3 (id:540001) has been deleted

[orglofch, 2014-11-08 01:30:36]

Patchset #3 (id:560001) has been deleted

[orglofch, 2014-11-08 01:59:03]

Patchset #3 (id:580001) has been deleted

[orglofch, 2014-11-09 22:47:34]

Patchset #3 (id:600001) has been deleted

[orglofch, 2014-11-09 23:33:30]

Patchset #3 (id:620001) has been deleted

[orglofch, 2014-11-09 23:43:20]

Patchset #3 (id:640001) has been deleted

[orglofch, 2014-11-09 23:45:39]

Patchset #3 (id:660001) has been deleted

[orglofch, 2014-11-09 23:54:43]

Patchset #3 (id:680001) has been deleted

[orglofch, 2014-11-10 00:55:33]

ptal. 
The IPC passes ValueState to GPU Service which is passed to the proper contexts
based on the render process host id. 

All functionality is hidden behind the --enable-subscribe-uniform-extension
flag. I elected to simply leave it at that instead of prematurely optimizing
(e.g. the GPU Service communicating which subscriptions are active) since we
currently only have one subscription target.

[Ken Russell (switch to Gerrit), 2014-11-17 23:19:32]

This lgtm as a preliminary and experimental step. You'll need OWNERS reviews in
some other directories. A couple of comments.

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:862: // extension is
enabled
Style nit here and throughout: please add comments with '.'.

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:868: #if
defined(OS_WIN)
This #ifdef is gross. Can you please figure out a better way to factor out the
decision about whether the target GPU process is sandboxed or not?

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:25: #include
"content/common/input/web_input_event_traits.h"
Is this #include strictly needed?

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:825: for
(StubMap::Iterator<GpuCommandBufferStub> it(&stubs_);
It would be nicer if this could more directly target the command buffer stubs
that are interested in this data. It's not worth a lot of complexity though.

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:285: GpuChannelMap::const_iterator
iter = gpu_channels_.find(client_id);
Again it would be nicer if this didn't involve a search.

[jdduke (slow), 2014-11-18 16:55:30]

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:859: const
base::CommandLine& command_line =
I'm wondering if we still want to send this message if the mouse event is
consumed before being forwarded to content, e.g., if consumed by one of the
|mouse_event_callbacks_| or by the TouchEmulator? Would it make sense to move
this logic down into |ForwardMouseEventWithLatencyInfo| below

input_router_->SendMouseEvent(...);

Not sure, I guess if we don't care about double handling maybe it doesn't
matter. 

You might also consider caching the CommandLine query either into a static
boolean or a member variable, as this method can get called with extreme
frequency (though that cost is probably dominated by sending the IPC, still,
don't want to pay the cost when the feature is disabled).

[orglofch, 2014-11-20 02:16:21]

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:859: const
base::CommandLine& command_line =
On 2014/11/18 16:55:30, jdduke wrote:
> I'm wondering if we still want to send this message if the mouse event is
> consumed before being forwarded to content, e.g., if consumed by one of the
> |mouse_event_callbacks_| or by the TouchEmulator? Would it make sense to move
> this logic down into |ForwardMouseEventWithLatencyInfo| below
> 
> input_router_->SendMouseEvent(...);
> 
> Not sure, I guess if we don't care about double handling maybe it doesn't
> matter. 
> 
> You might also consider caching the CommandLine query either into a static
> boolean or a member variable, as this method can get called with extreme
> frequency (though that cost is probably dominated by sending the IPC, still,
> don't want to pay the cost when the feature is disabled).

Done.

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:862: // extension is
enabled
On 2014/11/17 23:19:32, Ken Russell wrote:
> Style nit here and throughout: please add comments with '.'.

Done.

https://codereview.chromium.org/634313002/diff/700001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:868: #if
defined(OS_WIN)
On 2014/11/17 23:19:32, Ken Russell wrote:
> This #ifdef is gross. Can you please figure out a better way to factor out the
> decision about whether the target GPU process is sandboxed or not?

Done. This was unnecesarry logic, the GPU should be sandboxed unless the client
is collecting Dx diagnostic information.

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:25: #include
"content/common/input/web_input_event_traits.h"
On 2014/11/17 23:19:32, Ken Russell wrote:
> Is this #include strictly needed?

Nope, leftover from previously implementation. Removed.

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:825: for
(StubMap::Iterator<GpuCommandBufferStub> it(&stubs_);
On 2014/11/17 23:19:32, Ken Russell wrote:
> It would be nicer if this could more directly target the command buffer stubs
> that are interested in this data. It's not worth a lot of complexity though.

Are you thinking of something along the lines of decoder's reporting
subscriptions to the channel? The current implementation takes care of the edge
case where a client subscribes to a target later in the programs execution and
would still like the most recent mouse position (since new information will only
be populated if another event is triggered).

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:285: GpuChannelMap::const_iterator
iter = gpu_channels_.find(client_id);
On 2014/11/17 23:19:32, Ken Russell wrote:
> Again it would be nicer if this didn't involve a search.

Agreed, however, there doesn't appear to be another mechanism for associating
GpuChannels with RenderWidgetHosts outside of forwarding the data through the
renderer (which incurs latency).

[brianderson, 2014-11-20 21:11:25]

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

[brianderson, 2014-11-20 21:11:26]

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/700001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:285: GpuChannelMap::const_iterator
iter = gpu_channels_.find(client_id);
On 2014/11/20 02:16:21, orglofch wrote:
> On 2014/11/17 23:19:32, Ken Russell wrote:
> > Again it would be nicer if this didn't involve a search.
> 
> Agreed, however, there doesn't appear to be another mechanism for associating
> GpuChannels with RenderWidgetHosts outside of forwarding the data through the
> renderer (which incurs latency).

Seems okay to me. At least gpu_channels_ is a hash map, so performance should be
consistent regardless of the number of channels present.

https://codereview.chromium.org/634313002/diff/720001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/634313002/diff/720001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.h:32: union ValueState;
Is this needed?

[orglofch, 2014-11-20 21:25:04]

https://codereview.chromium.org/634313002/diff/720001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/634313002/diff/720001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.h:32: union ValueState;
On 2014/11/20 21:11:26, brianderson wrote:
> Is this needed?

Should have been in gpu_channel.h. Moved it.

[brianderson, 2014-11-21 00:16:55]

lgtm, but you'll need owners.

[Ken Russell (switch to Gerrit), 2014-11-21 02:13:18]

LGTM again; please figure out which OWNERS you'll need and ping them.

[orglofch, 2014-11-21 17:57:06]

orglofch@chromium.org changed reviewers:
+ piman@chromium.org

[orglofch, 2014-11-21 17:57:58]

+piman

[piman, 2014-11-21 19:48:51]

https://codereview.chromium.org/634313002/diff/740001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/740001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:287:
iter->second->HandleUpdateValueState(target, state);
Could we move the ValuebufferManager's pending_state_map_ into
GpuChannelManager, so that it's shared between all contexts? That way you
wouldn't need to iterate over all channels and contexts on every mouse event.

https://codereview.chromium.org/634313002/diff/740001/gpu/command_buffer/comm...
File gpu/command_buffer/common/value_state.h (right):

https://codereview.chromium.org/634313002/diff/740001/gpu/command_buffer/comm...
gpu/command_buffer/common/value_state.h:13: #endif
Why do you need this here?

[bajones, 2014-11-21 19:50:34]

gpu/command_buffer LGTM

[orglofch, 2014-11-21 22:04:13]

https://codereview.chromium.org/634313002/diff/740001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/740001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:287:
iter->second->HandleUpdateValueState(target, state);
On 2014/11/21 19:48:51, piman (Very slow to review) wrote:
> Could we move the ValuebufferManager's pending_state_map_ into
> GpuChannelManager, so that it's shared between all contexts? That way you
> wouldn't need to iterate over all channels and contexts on every mouse event.

This would require GLES2Decoders to know about their GpuChannel so they can
request the state on calls to PopulateUniformSubscriptions, unless the
GpuChannel can intercept cmd buffer calls? Is that a dependency we don't mind
having? The cost isn't too bad right now, we don't iterate over all channels (we
do perform a lookup) but we do iterate over all the contexts.

https://codereview.chromium.org/634313002/diff/740001/gpu/command_buffer/comm...
File gpu/command_buffer/common/value_state.h (right):

https://codereview.chromium.org/634313002/diff/740001/gpu/command_buffer/comm...
gpu/command_buffer/common/value_state.h:13: #endif
On 2014/11/21 19:48:51, piman (Very slow to review) wrote:
> Why do you need this here?

I had thought there was dependency preventing gl2extchromium.h from being
included in render_widget_host_impl.cc but that appears to not be the case.
Removed.

[piman, 2014-11-21 23:41:46]

On Fri, Nov 21, 2014 at 2:04 PM, <orglofch@chromium.org> wrote:

>
> https://codereview.chromium.org/634313002/diff/740001/
> content/common/gpu/gpu_channel_manager.cc
> File content/common/gpu/gpu_channel_manager.cc (right):
>
> https://codereview.chromium.org/634313002/diff/740001/
> content/common/gpu/gpu_channel_manager.cc#newcode287
> content/common/gpu/gpu_channel_manager.cc:287:
> iter->second->HandleUpdateValueState(target, state);
> On 2014/11/21 19:48:51, piman (Very slow to review) wrote:
>
>> Could we move the ValuebufferManager's pending_state_map_ into
>> GpuChannelManager, so that it's shared between all contexts? That way
>>
> you
>
>> wouldn't need to iterate over all channels and contexts on every mouse
>>
> event.
>
> This would require GLES2Decoders to know about their GpuChannel so they
> can request the state on calls to PopulateUniformSubscriptions, unless
> the GpuChannel can intercept cmd buffer calls? Is that a dependency we
> don't mind having? The cost isn't too bad right now, we don't iterate
> over all channels (we do perform a lookup) but we do iterate over all
> the contexts.


It's typically more the other way around: when the GpuCommandBufferStub
creates the ContextGroup, it can pass a pointer to the pending_state_map_
(or suitable wrapper if needed). See how e.g. we pass the
(shared) MailboxManager that's owned by the GpuChannelManager.

"not too bad" is all relative, since it happens on all mouse moves (there
can be a lot!). Also, you're redundantly setting the state multiple times
(multiple contexts share the ValuebufferManager).


>
> https://codereview.chromium.org/634313002/diff/740001/gpu/
> command_buffer/common/value_state.h
> File gpu/command_buffer/common/value_state.h (right):
>
> https://codereview.chromium.org/634313002/diff/740001/gpu/
> command_buffer/common/value_state.h#newcode13
> gpu/command_buffer/common/value_state.h:13: #endif
> On 2014/11/21 19:48:51, piman (Very slow to review) wrote:
>
>> Why do you need this here?
>>
>
> I had thought there was dependency preventing gl2extchromium.h from
> being included in render_widget_host_impl.cc but that appears to not be
> the case. Removed.
>
> https://codereview.chromium.org/634313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-11-21 23:46:38]

https://codereview.chromium.org/634313002/diff/760001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/760001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:912: if
(subscribe_uniform_enabled_) {
It can be done as a follow-up, but I'd like to see this being only enabled once
a context actually subscribes to the value. It will have a non-negligible cost
on all platforms, and we don't want to pay for it when no-one is looking at the
data.

The gpu process can send a message to request the data to be sent (and,
respectively, to stop having it be sent) once the first context subscribes (and
respectively, once the last context stops subscribing).

[brianderson, 2014-11-22 01:49:50]

Can you add tests verifying that input forwarded to one channel cannot be seen
by other channels? That security guarantee could be easy to break in the future
without modifying the messages that we send, so will not trigger a security
review automatically.

https://codereview.chromium.org/634313002/diff/760001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/634313002/diff/760001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:287:
iter->second->HandleUpdateValueState(target, state);
For the security review, it's important to note that the intent of this method
is to only forward the mouse events to the channel that belongs to the Renderer
the input is already going to.

Contexts that belong to other Renderer channels should not have a way to look at
another Renderer's input events, which I believe this patch also guarantees.

[orglofch, 2014-11-22 02:33:14]

Patchset #7 (id:780001) has been deleted

[orglofch, 2014-11-24 22:06:08]

Patchset #8 (id:820001) has been deleted

[orglofch, 2014-11-24 22:14:43]

Patchset #8 (id:840001) has been deleted

[orglofch, 2014-11-24 22:19:43]

Patchset #8 (id:860001) has been deleted

[orglofch, 2014-11-24 23:18:40]

Patchset #8 (id:880001) has been deleted

[orglofch, 2014-11-24 23:25:30]

Patchset #8 (id:900001) has been deleted

[orglofch, 2014-11-24 23:34:12]

Patchset #8 (id:920001) has been deleted

[orglofch, 2014-11-24 23:41:50]

Patchset #8 (id:940001) has been deleted

[orglofch, 2014-11-24 23:56:54]

Patchset #8 (id:960001) has been deleted

[orglofch, 2014-11-25 00:09:43]

Patchset #8 (id:980001) has been deleted

[orglofch, 2014-11-25 00:26:31]

Patchset #8 (id:1000001) has been deleted

[orglofch, 2014-11-25 00:43:50]

Patchset #8 (id:1020001) has been deleted

[orglofch, 2014-11-25 01:00:46]

Patchset #8 (id:1040001) has been deleted

[orglofch, 2014-11-25 01:17:41]

Patchset #8 (id:1060001) has been deleted

[orglofch, 2014-11-25 01:37:17]

Patchset #8 (id:1080001) has been deleted

[orglofch, 2014-11-25 02:04:33]

Patchset #8 (id:1100001) has been deleted

[orglofch, 2014-11-25 17:32:35]

Patchset #8 (id:1120001) has been deleted

[orglofch, 2014-11-25 18:15:48]

Patchset #8 (id:1140001) has been deleted

[orglofch, 2014-11-25 18:23:26]

Patchset #8 (id:1160001) has been deleted

[orglofch, 2014-11-25 18:55:19]

Patchset #8 (id:1180001) has been deleted

[orglofch, 2014-11-25 19:30:53]

Patchset #8 (id:1200001) has been deleted

[orglofch, 2014-11-25 20:05:09]

I've added a security test gpu_channel_manager_unittest.cc just to make sure the
gpu_channels don't share the ValueStates.

piman@ you're right I forgot I had moved the manager into the shared context.
I've modified it so ValuebufferManager is created in and updated in GpuChannel,
we still don't want to share it between different channels for security reasons.

https://codereview.chromium.org/634313002/diff/760001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/760001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:912: if
(subscribe_uniform_enabled_) {
On 2014/11/21 23:46:38, piman (Very slow to review) wrote:
> It can be done as a follow-up, but I'd like to see this being only enabled
once
> a context actually subscribes to the value. It will have a non-negligible cost
> on all platforms, and we don't want to pay for it when no-one is looking at
the
> data.
> 
> The gpu process can send a message to request the data to be sent (and,
> respectively, to stop having it be sent) once the first context subscribes
(and
> respectively, once the last context stops subscribing).

Acknowledged. Marked as a TODO.

[orglofch, 2014-11-25 23:24:18]

Patchset #9 (id:1240001) has been deleted

[orglofch, 2014-11-26 01:17:12]

Patchset #9 (id:1260001) has been deleted

[piman, 2014-11-26 20:47:09]

https://codereview.chromium.org/634313002/diff/1220001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/634313002/diff/1220001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:216:
scoped_refptr<gpu::gles2::ValuebufferManager> valuebuffer_manager_;
I don't think you can share the whole ValuebufferManager. It needs to stay
per-ContextGroup, otherwise independent ContextGroup could see each other's
Valuebuffers etc. It could also cause leaks (valuebuffers not destroyed when the
ContextGroup is destroyed).

I think what you want to share is only the pending_state_map_.

https://codereview.chromium.org/634313002/diff/1220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/valuebuffer_manager.cc (right):

https://codereview.chromium.org/634313002/diff/1220001/gpu/command_buffer/ser...
gpu/command_buffer/service/valuebuffer_manager.cc:103: const ValueState
*ValuebufferManager::GetPendingState(
nit: star on the left.

[orglofch, 2014-11-27 03:37:13]

Patchset #9 (id:1280001) has been deleted

[orglofch, 2014-11-27 05:15:38]

Patchset #9 (id:1300001) has been deleted

[orglofch, 2014-11-27 20:20:12]

Patchset #9 (id:1320001) has been deleted

[orglofch, 2014-11-27 20:26:31]

Patchset #9 (id:1340001) has been deleted

[orglofch, 2014-11-27 20:58:40]

Patchset #9 (id:1360001) has been deleted

[orglofch, 2014-11-27 21:10:35]

Patchset #9 (id:1380001) has been deleted

[orglofch, 2014-11-27 21:19:13]

Patchset #9 (id:1400001) has been deleted

[orglofch, 2014-11-27 21:43:46]

Patchset #9 (id:1420001) has been deleted

[orglofch, 2014-11-27 21:51:40]

Patchset #9 (id:1440001) has been deleted

[orglofch, 2014-11-28 00:52:22]

Patchset #10 (id:1480001) has been deleted

[orglofch, 2014-11-28 00:52:26]

Patchset #9 (id:1460001) has been deleted

[orglofch, 2014-11-29 01:50:10]

Patchset #9 (id:1500001) has been deleted

[orglofch, 2014-11-29 02:57:53]

Patchset #9 (id:1520001) has been deleted

[orglofch, 2014-11-29 02:58:02]

Patchset #9 (id:1540001) has been deleted

[orglofch, 2014-11-29 03:54:06]

Patchset #9 (id:1560001) has been deleted

[orglofch, 2014-11-29 04:04:48]

Patchset #9 (id:1580001) has been deleted

[orglofch, 2014-12-01 00:59:58]

Patchset #10 (id:1610001) has been deleted

[orglofch, 2014-12-01 01:54:07]

Patchset #10 (id:1630001) has been deleted

[orglofch, 2014-12-01 01:54:29]

https://codereview.chromium.org/634313002/diff/1220001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://codereview.chromium.org/634313002/diff/1220001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:216:
scoped_refptr<gpu::gles2::ValuebufferManager> valuebuffer_manager_;
On 2014/11/26 20:47:09, piman (Very slow to review) wrote:
> I don't think you can share the whole ValuebufferManager. It needs to stay
> per-ContextGroup, otherwise independent ContextGroup could see each other's
> Valuebuffers etc. It could also cause leaks (valuebuffers not destroyed when
the
> ContextGroup is destroyed).
> 
> I think what you want to share is only the pending_state_map_.

Done.

https://codereview.chromium.org/634313002/diff/1220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/valuebuffer_manager.cc (right):

https://codereview.chromium.org/634313002/diff/1220001/gpu/command_buffer/ser...
gpu/command_buffer/service/valuebuffer_manager.cc:103: const ValueState
*ValuebufferManager::GetPendingState(
On 2014/11/26 20:47:09, piman (Very slow to review) wrote:
> nit: star on the left.

Done. Removed, no longer needed for testing.

[piman, 2014-12-01 19:52:05]

lgtm

[orglofch, 2014-12-01 21:53:46]

Patchset #10 (id:1650001) has been deleted

[orglofch, 2014-12-02 00:24:56]

Patchset #10 (id:1670001) has been deleted

[orglofch, 2014-12-02 01:05:17]

Patchset #10 (id:1690001) has been deleted

[Tom Sepez, 2014-12-02 17:47:43]

Messages LGTM.

[orglofch, 2014-12-02 20:36:48]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-12-02 20:37:10]

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

[commit-bot: I haz the power, 2014-12-02 20:37:23]

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

[commit-bot: I haz the power, 2014-12-02 20:37:25]

A disapproval has been posted by following reviewers: jdduke@chromium.org.
Please make sure to get positive review before another CQ attempt.

[jdduke (slow), 2014-12-02 20:56:43]

https://codereview.chromium.org/634313002/diff/1710001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/1710001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:241:
subscribe_uniform_enabled_ =
This lgtm, and was the only concern I had from the previous patch (I can't
really speak to any of the other files affected here).

https://codereview.chromium.org/634313002/diff/1710001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:919:
GpuProcessHost::GPU_PROCESS_KIND_SANDBOXED,
Nit: This should be 4-space indent for the args.

[orglofch, 2014-12-02 21:01:39]

The CQ bit was checked by orglofch@chromium.org

[orglofch, 2014-12-02 21:01:53]

https://codereview.chromium.org/634313002/diff/1710001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/634313002/diff/1710001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:919:
GpuProcessHost::GPU_PROCESS_KIND_SANDBOXED,
On 2014/12/02 20:56:42, jdduke wrote:
> Nit: This should be 4-space indent for the args.

Done.

[commit-bot: I haz the power, 2014-12-02 21:03:10]

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

[commit-bot: I haz the power, 2014-12-02 21:09:50]

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

[commit-bot: I haz the power, 2014-12-02 21:09:52]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/88832)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[orglofch, 2014-12-02 21:14:59]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-12-02 21:15:55]

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

[commit-bot: I haz the power, 2014-12-02 22:55:01]

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

[commit-bot: I haz the power, 2014-12-02 22:55:03]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[orglofch, 2014-12-03 04:22:01]

Patchset #13 (id:1760001) has been deleted

[orglofch, 2014-12-03 04:23:28]

The CQ bit was checked by orglofch@chromium.org

[commit-bot: I haz the power, 2014-12-03 04:23:49]

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

[commit-bot: I haz the power, 2014-12-03 05:32:25]

Committed patchset #13 (id:1780001)

[commit-bot: I haz the power, 2014-12-03 05:33:05]

Patchset 13 (id:??) landed as
https://crrev.com/33edd848118675b2042df9c74e470bbe33c93d83
Cr-Commit-Position: refs/heads/master@{#306555}