Android Webview IPC-based sync compositing

Android Webview IPC-based sync compositing

Existing browser-renderer call path uses
SynchronousCompositorImpl to cross the logical process boundary:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorImpl <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

New call path, added behind kIPCSyncCompositing switch essentially
replaces SynchronousCompositorImpl with actual IPC code. New call
path looks like this:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorHost <-> IPC
<-> SynchronousCompositorFilter
<-> SynchronousCompositorProxy <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

Browser side:

Introduce SynchronousCompositorBase which adds methods used by RWHVA.

Add SynchronousCompositorHost which instead of calling to renderer
directly, uses sync IPCs instead.

Renderer side:

Add SynchronousCompositorFilter which is responsible for filtering sync
compositing messages and sending them to compositor thread. The filter
doubles as the SynchronousCompositorRegistry implementation. Filter is
owned by RenderThreadImpl so should outlive all other objects and the
compositor thread itself.

Add SynchronousCompositorProxy. This takes the place of
SynchronousCompositorImpl for the single-process path; the proxy is the
client for OutputSurface and BeginFrameSource. Proxy also receives sync
IPC from SynchronousCompositorHost and send replies and async IPCs back.
Proxy is used on the compositor thread only. Proxy lifetime is
controlled by the filter (registry) and is outlived by both
OutputSurface and BeginFrameSource so life time management is simple.

OutputSurface and BeginFrameSource are re-used.

IPCs:

There are common browser and renderer states that are sent on each
message from and to browser side. There is a versioning system for the
renderer state since messages may arrive out of order on the browser side.

BUG=526842
Committed: https://crrev.com/bee541f46cb093523cc5736932e071573424d8de
Cr-Commit-Position: refs/heads/master@{#357955}

[boliu, 2015-10-21 00:09:33]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-10-21 00:09:33]

Alright. This is the big one!

I tried to summarize the changes in the CL description. I don't think it's
worthwhile to break this up any further.

[boliu, 2015-10-22 18:05:51]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org

[boliu, 2015-10-22 18:05:52]

+dcheng for security/IPC. You watched my talk. Want to do the security review
for this? I bet my IPC changes are the lowest quality part of this patch, my
first time digging into what all the IPC macros actually do..

And gentle ping for sievers

I'm totally open to splitting this up more, other than adding the IPCs one piece
at a time maybe (which isn't that much code), I don't really see a good way to
break this down more.

[dcheng, 2015-10-27 21:06:59]

I'm pretty sure I don't really understand this patch yet, but here's some random
thoughts on my initial pass.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:67: if
(!frame->delegated_frame_data.get()) {
No .get()

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:70: if (frame.get())
Ditto, no .get()

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:72: return frame.Pass();
return frame; should work

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:161: if
((renderer_param_version_ - params.version) < 0x80000000) {
What's the significance of 0x80000000? I think this needs a comment.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:30: gfx::Size
surface_size,
gfx::Size and gfx::Rect are usually passed by const ref aren't they?

https://codereview.chromium.org/1408123005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:378: return
sync_compositor_.get() && sync_compositor_->OnMessageReceived(message);
No .get(). etc.

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
File content/common/android/sync_compositor_messages.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.cc:9: void
ParamTraits<gfx::ScrollOffset>::Write(
It feels like maybe this should be in ui/gfx/ipc?

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.cc:16: base::PickleIterator*
iter,
Hmm, did clang-format do this? It looks funny.

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.h:15: #ifndef
CONTENT_COMMON_ANDROID_SYNC_COMPOSITOR_MESSAGES_H_
Does this include guard need to intentionally omit wrapping the headers too?

https://codereview.chromium.org/1408123005/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:109:
DCHECK(entry_map_.find(routing_id) != entry_map_.end());
You could use ContainsKey.

[boliu, 2015-10-27 22:44:05]

Description was changed from

==========
Android Webview IPC-based sync compositing

New code path added behind kIPCSyncCompositing switch.

Browser side:

Introduce SynchronousCompositorBase which adds methods used by RWHVA.

Add SynchronousCompositorHost which instead of calling to renderer
directly, uses sync IPCs instead.

Renderer side:

Add SynchronousCompositorFilter which is responsible for filtering sync
compositing messages and sending them to compositor thread. The filter
doubles as the SynchronousCompositorRegistry implementation. Filter is
owned by RenderThreadImpl so should outlive all other objects and the
compositor thread itself.

Add SynchronousCompositorProxy. This takes the place of
SynchronousCompositorImpl for the single-process path; the proxy is the
client for OutputSurface and BeginFrameSource. Proxy also receives sync
IPC from SynchronousCompositorHost and send replies and async IPCs back.
Proxy is used on the compositor thread only. Proxy lifetime is
controlled by the filter (registry) and is outlived by both
OutputSurface and BeginFrameSource so life time management is simple.

OutputSurface and BeginFrameSource are re-used.

IPCs:

There are common browser and renderer states that are sent on each
message from and to browser side. There is a versioning system for the
renderer state since messages may arrive out of order on the browser side.

BUG=526842
==========

to

==========
Android Webview IPC-based sync compositing

Existing browser-renderer call path uses
SynchronousCompositorImpl to cross the logical process boundary:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorImpl <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

New call path, added behind kIPCSyncCompositing switch essentially
replaces SynchronousCompositorImpl with actual IPC code, which
looks like this:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorHost <-> IPC
<-> SynchronousCompositorFilter
<-> SynchronousCompositorProxy <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

Browser side:

Introduce SynchronousCompositorBase which adds methods used by RWHVA.

Add SynchronousCompositorHost which instead of calling to renderer
directly, uses sync IPCs instead.

Renderer side:

Add SynchronousCompositorFilter which is responsible for filtering sync
compositing messages and sending them to compositor thread. The filter
doubles as the SynchronousCompositorRegistry implementation. Filter is
owned by RenderThreadImpl so should outlive all other objects and the
compositor thread itself.

Add SynchronousCompositorProxy. This takes the place of
SynchronousCompositorImpl for the single-process path; the proxy is the
client for OutputSurface and BeginFrameSource. Proxy also receives sync
IPC from SynchronousCompositorHost and send replies and async IPCs back.
Proxy is used on the compositor thread only. Proxy lifetime is
controlled by the filter (registry) and is outlived by both
OutputSurface and BeginFrameSource so life time management is simple.

OutputSurface and BeginFrameSource are re-used.

IPCs:

There are common browser and renderer states that are sent on each
message from and to browser side. There is a versioning system for the
renderer state since messages may arrive out of order on the browser side.

BUG=526842
==========

[boliu, 2015-10-27 22:48:41]

Description was changed from

==========
Android Webview IPC-based sync compositing

Existing browser-renderer call path uses
SynchronousCompositorImpl to cross the logical process boundary:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorImpl <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

New call path, added behind kIPCSyncCompositing switch essentially
replaces SynchronousCompositorImpl with actual IPC code, which
looks like this:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorHost <-> IPC
<-> SynchronousCompositorFilter
<-> SynchronousCompositorProxy <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

Browser side:

Introduce SynchronousCompositorBase which adds methods used by RWHVA.

Add SynchronousCompositorHost which instead of calling to renderer
directly, uses sync IPCs instead.

Renderer side:

Add SynchronousCompositorFilter which is responsible for filtering sync
compositing messages and sending them to compositor thread. The filter
doubles as the SynchronousCompositorRegistry implementation. Filter is
owned by RenderThreadImpl so should outlive all other objects and the
compositor thread itself.

Add SynchronousCompositorProxy. This takes the place of
SynchronousCompositorImpl for the single-process path; the proxy is the
client for OutputSurface and BeginFrameSource. Proxy also receives sync
IPC from SynchronousCompositorHost and send replies and async IPCs back.
Proxy is used on the compositor thread only. Proxy lifetime is
controlled by the filter (registry) and is outlived by both
OutputSurface and BeginFrameSource so life time management is simple.

OutputSurface and BeginFrameSource are re-used.

IPCs:

There are common browser and renderer states that are sent on each
message from and to browser side. There is a versioning system for the
renderer state since messages may arrive out of order on the browser side.

BUG=526842
==========

to

==========
Android Webview IPC-based sync compositing

Existing browser-renderer call path uses
SynchronousCompositorImpl to cross the logical process boundary:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorImpl <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

New call path, added behind kIPCSyncCompositing switch essentially
replaces SynchronousCompositorImpl with actual IPC code. New call
path looks like this:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorHost <-> IPC
<-> SynchronousCompositorFilter
<-> SynchronousCompositorProxy <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

Browser side:

Introduce SynchronousCompositorBase which adds methods used by RWHVA.

Add SynchronousCompositorHost which instead of calling to renderer
directly, uses sync IPCs instead.

Renderer side:

Add SynchronousCompositorFilter which is responsible for filtering sync
compositing messages and sending them to compositor thread. The filter
doubles as the SynchronousCompositorRegistry implementation. Filter is
owned by RenderThreadImpl so should outlive all other objects and the
compositor thread itself.

Add SynchronousCompositorProxy. This takes the place of
SynchronousCompositorImpl for the single-process path; the proxy is the
client for OutputSurface and BeginFrameSource. Proxy also receives sync
IPC from SynchronousCompositorHost and send replies and async IPCs back.
Proxy is used on the compositor thread only. Proxy lifetime is
controlled by the filter (registry) and is outlived by both
OutputSurface and BeginFrameSource so life time management is simple.

OutputSurface and BeginFrameSource are re-used.

IPCs:

There are common browser and renderer states that are sent on each
message from and to browser side. There is a versioning system for the
renderer state since messages may arrive out of order on the browser side.

BUG=526842
==========

[boliu, 2015-10-27 22:49:14]

No new patch set yet

On 2015/10/27 21:06:59, dcheng wrote:
> I'm pretty sure I don't really understand this patch yet

Added this block to description:
"""
Existing browser-renderer call path uses
SynchronousCompositorImpl to cross the logical process boundary:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorImpl <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]

New call path, added behind kIPCSyncCompositing switch essentially
replaces SynchronousCompositorImpl with actual IPC code. New call
path looks like this:

[BrowserViewRenderer, RenderWidgetHostViewAndroid]
<-> SynchronousCompositorHost <-> IPC
<-> SynchronousCompositorFilter
<-> SynchronousCompositorProxy <->
[SynchronousCompositorOutputSurface,
 SynchronousCompositorExternalBeginFrameSource]
"""


SynchronousCompositorImpl lives in content/browser/android/in_process which has
special DEPS rules to allow it to call into content/renderer. This patch
basically replaces everything going through SynchronousCompositorImpl with real
IPCs.

Hopefully that clears things up a bit, maybe :)

[boliu, 2015-10-28 02:07:06]

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:67: if
(!frame->delegated_frame_data.get()) {
On 2015/10/27 21:06:58, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:70: if (frame.get())
On 2015/10/27 21:06:58, dcheng wrote:
> Ditto, no .get()

Done.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:72: return frame.Pass();
On 2015/10/27 21:06:58, dcheng wrote:
> return frame; should work

Done.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:161: if
((renderer_param_version_ - params.version) < 0x80000000) {
On 2015/10/27 21:06:58, dcheng wrote:
> What's the significance of 0x80000000? I think this needs a comment.

It's for comparing versions when the version can wrap around int. 0x80000000 is
the "mid point" o the resulting difference I guess. I stole this from
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Added a comment

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:30: gfx::Size
surface_size,
On 2015/10/27 21:06:58, dcheng wrote:
> gfx::Size and gfx::Rect are usually passed by const ref aren't they?

I _remember_ at some point these were supposed to be passed by value because
they are two ints. Ha, found this:
https://code.google.com/p/chromium/issues/detail?id=159273#c31, so const ref it
is then.

Pulling it out into https://codereview.chromium.org/1417893006/, and I'll rebase
this patch when that lands. For now, I'll keep this building on top of trunk.

https://codereview.chromium.org/1408123005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:378: return
sync_compositor_.get() && sync_compositor_->OnMessageReceived(message);
On 2015/10/27 21:06:58, dcheng wrote:
> No .get(). etc.

Done.

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
File content/common/android/sync_compositor_messages.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.cc:9: void
ParamTraits<gfx::ScrollOffset>::Write(
On 2015/10/27 21:06:58, dcheng wrote:
> It feels like maybe this should be in ui/gfx/ipc?

Good call! Done

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.cc:16: base::PickleIterator*
iter,
On 2015/10/27 21:06:58, dcheng wrote:
> Hmm, did clang-format do this? It looks funny.

Nope. I just didn't clang-format this.

And clang-formatted it now :)

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/20001/content/common/android/...
content/common/android/sync_compositor_messages.h:15: #ifndef
CONTENT_COMMON_ANDROID_SYNC_COMPOSITOR_MESSAGES_H_
On 2015/10/27 21:06:58, dcheng wrote:
> Does this include guard need to intentionally omit wrapping the headers too?

Only ipc/ipc_message_macros.h need to live outside the ifndef, for the define
IPC_MESSAGE_START line below.

But copying from content/common/cc_messages.h, all the includes there are
outside of the ifndef as well.

Did not change anything

https://codereview.chromium.org/1408123005/diff/20001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/20001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:109:
DCHECK(entry_map_.find(routing_id) != entry_map_.end());
On 2015/10/27 21:06:58, dcheng wrote:
> You could use ContainsKey.

Done

[no sievers, 2015-10-29 01:37:01]

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/in_process/DEPS (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/in_process/DEPS:6: # Include boliu@chromium.org on the
review for any additions to this file.
i think you can enforce that through the OWNERS file with 'set noparent'

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:296: return
false;
ideally NOTREACHED(), or some comment '// not handling any messages' or so would
be nice

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:76: bool
did_activate_pending_tree_;
nit: DISALLOW_COPY_AND_ASSIGN();

https://codereview.chromium.org/1408123005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:359: if
(SyncCompositorOnMessageReceived(message)) {
can you do a IPC_MESSAGE_ID_CLASS(message.type()) != SyncCompositorHostMsg check
first?
then you can also make sure it was handled.

https://codereview.chromium.org/1408123005/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:20: struct CommonBrowserParams
{
nit: this name seems a bit generic

Do all these structs actually have to be in here? Or can they maybe be inside
SynchronousCompositor or somewhere?
also would make the ifdef-boundaries less odd

https://codereview.chromium.org/1408123005/diff/60001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1408123005/diff/60001/content/content_browser...
content/content_browser.gypi:395:
'browser/android/synchronous_compositor_host.h',
can you rebase and put this under 'android_in_process_browser_sources'?
you might obviously want to rename the source set now, but these don't work in
the aura build since there are RWHVA dependencies

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:38: if
(IPC_MESSAGE_ID_CLASS(message.type()) != SyncCompositorMsgStart) {
If you implement GetSupportedMessageClasses() you can avoid this.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:154:
make_scoped_ptr(new SynchronousCompositorProxy(
why do we still need the registry and why can't the lifetime be the same as a
normal render compositor now?

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.h:95: EntryMap
entry_map_;
nit: DISALLOW_COPY_AND_ASSIGN();

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:15: class
SynchronousCompositorProxy::ScopedInsideReceive {
nit:: base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
achieves this

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:111: if
(!inside_receive_) {
can this all be one helper function?

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.h:113: bool
did_activate_pending_tree_;
nit: DISALLOW_COPY_AND_ASSIGN();

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:373: bool
using_synchronous_compositor =
DCHECK(!SynchronousCompositorFactory::GetInstance() ||
!cmd->HasSwitch(switches::kIPCSyncCompositing));

[boliu, 2015-10-29 04:24:44]

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/in_process/DEPS (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/in_process/DEPS:6: # Include boliu@chromium.org on the
review for any additions to this file.
On 2015/10/29 01:37:00, sievers wrote:
> i think you can enforce that through the OWNERS file with 'set noparent'

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:296: return
false;
On 2015/10/29 01:37:00, sievers wrote:
> ideally NOTREACHED(), or some comment '// not handling any messages' or so
would
> be nice

NOTREACHED is good.

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.h:76: bool
did_activate_pending_tree_;
On 2015/10/29 01:37:00, sievers wrote:
> nit: DISALLOW_COPY_AND_ASSIGN();

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:359: if
(SyncCompositorOnMessageReceived(message)) {
On 2015/10/29 01:37:00, sievers wrote:
> can you do a IPC_MESSAGE_ID_CLASS(message.type()) != SyncCompositorHostMsg
check
> first?
> then you can also make sure it was handled.

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:20: struct CommonBrowserParams
{
On 2015/10/29 01:37:00, sievers wrote:
> nit: this name seems a bit generic
> 
> Do all these structs actually have to be in here? Or can they maybe be inside
> SynchronousCompositor or somewhere?
> also would make the ifdef-boundaries less odd

They have to be in common because both renderer and browser sides need them.
They could be a in a different file though if preferred. But I think the general
pattern is put these at the top of the messages file (unless they get used in
places other than these IPCs)

Yeah, these kind of sound generic for content namespace. Prepended
SyncCompositor to them.

https://codereview.chromium.org/1408123005/diff/60001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1408123005/diff/60001/content/content_browser...
content/content_browser.gypi:395:
'browser/android/synchronous_compositor_host.h',
On 2015/10/29 01:37:00, sievers wrote:
> can you rebase and put this under 'android_in_process_browser_sources'?
> you might obviously want to rename the source set now, but these don't work in
> the aura build since there are RWHVA dependencies

Done

Hmm, I don't recall rebasing this part. And I rebase every day..

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:38: if
(IPC_MESSAGE_ID_CLASS(message.type()) != SyncCompositorMsgStart) {
On 2015/10/29 01:37:00, sievers wrote:
> If you implement GetSupportedMessageClasses() you can avoid this.

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:154:
make_scoped_ptr(new SynchronousCompositorProxy(
On 2015/10/29 01:37:00, sievers wrote:
> why do we still need the registry and why can't the lifetime be the same as a
> normal render compositor now?

Because code paths are still shared with merged thread implementation.

When merged thread path is removed, and SynchronousCompositorOutputSurface and
ExternalBeginFrameSource can start sending and receiving IPCs, then registry and
the proxy won't be needed anymore.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.h:95: EntryMap
entry_map_;
On 2015/10/29 01:37:01, sievers wrote:
> nit: DISALLOW_COPY_AND_ASSIGN();

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:15: class
SynchronousCompositorProxy::ScopedInsideReceive {
On 2015/10/29 01:37:01, sievers wrote:
> nit:: base::AutoReset<bool> scoped_inside_receive(&inside_receive_, true);
> achieves this

Done

Can't remember why I didn't go with AutoReset. I think it was because I had more
complex use cases in mind. I still do, it's just not in this patch yet.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:111: if
(!inside_receive_) {
On 2015/10/29 01:37:01, sievers wrote:
> can this all be one helper function?

Done. I think I had a TODO for this :)

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.h (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.h:113: bool
did_activate_pending_tree_;
On 2015/10/29 01:37:01, sievers wrote:
> nit: DISALLOW_COPY_AND_ASSIGN();

Done.

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1408123005/diff/60001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:373: bool
using_synchronous_compositor =
On 2015/10/29 01:37:01, sievers wrote:
> DCHECK(!SynchronousCompositorFactory::GetInstance() ||
> !cmd->HasSwitch(switches::kIPCSyncCompositing));

Done.

[no sievers, 2015-11-03 00:41:30]

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
File content/browser/android/in_process/OWNERS (right):

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/in_process/OWNERS:4: per-file DEPS=boliu@chromium.org
you can never go on a vacation again :)  maybe add hush@?

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:67: frame.reset();
when does that happen? add a comment maybe

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:190: // Make sure
renderer side is initialized.
nit: i don't get the comment

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:74:
DCHECK(io_task_runner_.get());  // TODO(boliu): Not really thread safe.
ok but if you say DCHECK(io_task_runner_.get()) then you are expecting nobody to
send a msg to the browser until OnFilterAdded() happened. how is that
guaranteed?
but if it is, then might as well fix the thread safety issue by passing in the
io task runner into the constructor from render_thread_impl.cc. probably
deserves a comment still why |sender_| will be set then.

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:218: // TODO(boliu):
mutable and swap?
nit: remove todo or do it :)

[boliu, 2015-11-03 01:39:57]

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
File content/browser/android/in_process/OWNERS (right):

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/in_process/OWNERS:4: per-file DEPS=boliu@chromium.org
On 2015/11/03 00:41:30, sievers wrote:
> you can never go on a vacation again :)  maybe add hush@?

Honestly I think the comment in DEPS worked fine before. No one ever touches
in_process anyway.

Just removing this altogether.

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:67: frame.reset();
On 2015/11/03 00:41:30, sievers wrote:
> when does that happen? add a comment maybe

Yes. As of https://codereview.chromium.org/1394263004, compositor is free to not
swap a hardware draw if it doesn't want to.

Added a comment.

https://codereview.chromium.org/1408123005/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:190: // Make sure
renderer side is initialized.
On 2015/11/03 00:41:30, sievers wrote:
> nit: i don't get the comment

It means these scroll/scale values are valid. eg page_scale_factor is
initialized to 0 in compositor, which meas compositor is not fully initialized
if a value of 0 is seen here.

Updated comment.

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_filter.cc:74:
DCHECK(io_task_runner_.get());  // TODO(boliu): Not really thread safe.
On 2015/11/03 00:41:30, sievers wrote:
> ok but if you say DCHECK(io_task_runner_.get()) then you are expecting nobody
to
> send a msg to the browser until OnFilterAdded() happened. how is that
> guaranteed?

Ohh, I should not have left this as a TODO.

So new patch guarantees this now:
Added a new FilterReadyyOnCompositorThread call posted from OnFilterAdded. No
SynchronousCompositorProxy is created on compositor thread until this signal.
And since the proxy is the only thing calling send, this is guaranteed.

Also DCHECK the status bool instead of accessing this which might not be thread
safe.

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:218: // TODO(boliu):
mutable and swap?
On 2015/11/03 00:41:30, sievers wrote:
> nit: remove todo or do it :)

kk, using a const_cast here. My code should be fine with it. How safe do you
think ipc code will be in handling this..?

[no sievers, 2015-11-03 18:08:28]

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:218: // TODO(boliu):
mutable and swap?
On 2015/11/03 01:39:57, boliu wrote:
> On 2015/11/03 00:41:30, sievers wrote:
> > nit: remove todo or do it :)
> 
> kk, using a const_cast here. My code should be fine with it. How safe do you
> think ipc code will be in handling this..?

You can pass a const-ref to the msg by using IPC_MESSAGE_HANDLER_GENERIC() and
then manually deserialize it into a non-const variable, see
RenderFrameProxy::OnCompositorFrameSwapped().

[boliu, 2015-11-03 18:12:13]

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:218: // TODO(boliu):
mutable and swap?
On 2015/11/03 18:08:28, sievers wrote:
> On 2015/11/03 01:39:57, boliu wrote:
> > On 2015/11/03 00:41:30, sievers wrote:
> > > nit: remove todo or do it :)
> > 
> > kk, using a const_cast here. My code should be fine with it. How safe do you
> > think ipc code will be in handling this..?
> 
> You can pass a const-ref to the msg by using IPC_MESSAGE_HANDLER_GENERIC() and
> then manually deserialize it into a non-const variable, see
> RenderFrameProxy::OnCompositorFrameSwapped().

I really don't want to do that since the CommonParams is sent in every single
ipc message, so I don't want to turn all of them into
IPC_MESSAGE_HANDLER_GENERIC.

I'm looking into just passing the FrameAck in the struct instead, but that has
some other complications on the sending side since FrameAck is not copy-able.
One sec..

[boliu, 2015-11-03 18:25:09]

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:218: // TODO(boliu):
mutable and swap?
On 2015/11/03 18:12:12, boliu wrote:
> On 2015/11/03 18:08:28, sievers wrote:
> > On 2015/11/03 01:39:57, boliu wrote:
> > > On 2015/11/03 00:41:30, sievers wrote:
> > > > nit: remove todo or do it :)
> > > 
> > > kk, using a const_cast here. My code should be fine with it. How safe do
you
> > > think ipc code will be in handling this..?
> > 
> > You can pass a const-ref to the msg by using IPC_MESSAGE_HANDLER_GENERIC()
and
> > then manually deserialize it into a non-const variable, see
> > RenderFrameProxy::OnCompositorFrameSwapped().
> 
> I really don't want to do that since the CommonParams is sent in every single
> ipc message, so I don't want to turn all of them into
> IPC_MESSAGE_HANDLER_GENERIC.
> 
> I'm looking into just passing the FrameAck in the struct instead, but that has
> some other complications on the sending side since FrameAck is not copy-able.
> One sec..

Ok. PS7 sends the FrameAck through IPC instead, so no need for const_cast here.
The sending side became a bit more verbose; and (possibly?) slightly less
efficient, but shouldn't matter much

[no sievers, 2015-11-03 19:26:58]

lgtm

[dcheng, 2015-11-04 09:47:41]

https://codereview.chromium.org/1408123005/diff/120001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:188: if (!client_)
How can client_ be null? It's unconditionally dereferenced in the constructor.

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
File content/common/android/OWNERS (right):

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/OWNERS:30: per-file *param_traits*.h=set noparent
Nit: make this *_param_traits.* (even though they don't exist yet)

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/sync_compositor_messages.h:64: bool
did_activate_pending_tree);
I'll leave it to your judgement, but one nice thing about not having a
constructor like this is that it's very clear that you're not accidentally
mixing up parameters.

Because there are so many parameters, and so many have the same type, it'd be
easy to accidentally typo the order, and the compiler would never notice.

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/sync_compositor_messages.h:67: unsigned int version;
Explicitly size to uint32_t
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-i...)

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:18:
DCHECK(compositor_task_runner_.get());
No .get(): the smart pointer types can all be boolean tested without an explicit
.get()

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:42:
DCHECK_EQ(IPC_MESSAGE_ID_CLASS(message.type()), SyncCompositorMsgStart);
Nit: nice to do expected, actual for consistency with gtest macros (even if it's
not important for DCHECK)

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:81: this,
base::Unretained(message)))) {
base::Unretained() should be unnecessary here.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.h:84: typedef
base::ScopedPtrHashMap<int,  // routing_id
using here, for consistency?

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:114: }
Maybe just take advantage of ScopedVector<T>::weak_clear()?  I'm not sure this
is that much easier to read.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:186: if
(frame_ptr.get()) {
No .get()

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.h:24: }
Nit: // namespace blink for consistency

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.h:102: bool
inside_receive_;  // TODO(boliu): versioning
Not sure I understand this comment.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:863: if
(sync_compositor_message_filter_.get()) {
Nit: no .get()

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:865: sync_compositor_message_filter_ =
NULL;
Nit: nullptr

[boliu, 2015-11-04 16:52:05]

https://codereview.chromium.org/1408123005/diff/120001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:188: if (!client_)
On 2015/11/04 09:47:41, dcheng wrote:
> How can client_ be null? It's unconditionally dereferenced in the constructor.

Nope. client_ is guaranteed to not change, and outlive this class. Removed this
check.

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
File content/common/android/OWNERS (right):

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/OWNERS:30: per-file *param_traits*.h=set noparent
On 2015/11/04 09:47:41, dcheng wrote:
> Nit: make this *_param_traits.* (even though they don't exist yet)

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/sync_compositor_messages.h:64: bool
did_activate_pending_tree);
On 2015/11/04 09:47:41, dcheng wrote:
> I'll leave it to your judgement, but one nice thing about not having a
> constructor like this is that it's very clear that you're not accidentally
> mixing up parameters.
> 
> Because there are so many parameters, and so many have the same type, it'd be
> easy to accidentally typo the order, and the compiler would never notice.

Done.

So the upside of having the constructor means everything will be intialized to a
valid value (most of the default values aren't valid).

But then either way, it's a good thing there is only a single function that sets
all of these variables, so it really doesn't matter.

(I was going to make point about how these variables are not set twice in the
name of efficency, but then SynchronousCompositorProxy::ConstructCommonParams
doesn't construct this struct, so it's copied either way)

So, taking your suggestion. Removed this constructor and set the values
individually instead to make it more readable.

Also removed the constructor with parameters from BrowserParams. That's not used
anymore.

https://codereview.chromium.org/1408123005/diff/120001/content/common/android...
content/common/android/sync_compositor_messages.h:67: unsigned int version;
On 2015/11/04 09:47:41, dcheng wrote:
> Explicitly size to uint32_t
>
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-i...)

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:18:
DCHECK(compositor_task_runner_.get());
On 2015/11/04 09:47:41, dcheng wrote:
> No .get(): the smart pointer types can all be boolean tested without an
explicit
> .get()

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:42:
DCHECK_EQ(IPC_MESSAGE_ID_CLASS(message.type()), SyncCompositorMsgStart);
On 2015/11/04 09:47:41, dcheng wrote:
> Nit: nice to do expected, actual for consistency with gtest macros (even if
it's
> not important for DCHECK)

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.cc:81: this,
base::Unretained(message)))) {
On 2015/11/04 09:47:41, dcheng wrote:
> base::Unretained() should be unnecessary here.

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_filter.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_filter.h:84: typedef
base::ScopedPtrHashMap<int,  // routing_id
On 2015/11/04 09:47:41, dcheng wrote:
> using here, for consistency?

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:114: }
On 2015/11/04 09:47:41, dcheng wrote:
> Maybe just take advantage of ScopedVector<T>::weak_clear()?  I'm not sure this
> is that much easier to read.

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:186: if
(frame_ptr.get()) {
On 2015/11/04 09:47:41, dcheng wrote:
> No .get()

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.h (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.h:24: }
On 2015/11/04 09:47:41, dcheng wrote:
> Nit: // namespace blink for consistency

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.h:102: bool
inside_receive_;  // TODO(boliu): versioning
On 2015/11/04 09:47:41, dcheng wrote:
> Not sure I understand this comment.

Just removed this comment.

I think this was a TODO for adding versioning parameter, since a synchronous
reply is sent back when inside_receive_ is flipped from true to false. Hmm,
depending on the message, maybe it's better to send it along with the
synchronous reply. That's an optimization which can come later.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:863: if
(sync_compositor_message_filter_.get()) {
On 2015/11/04 09:47:41, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/1408123005/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:865: sync_compositor_message_filter_ =
NULL;
On 2015/11/04 09:47:41, dcheng wrote:
> Nit: nullptr

Done.

[dcheng, 2015-11-04 18:24:29]

lgtm

https://codereview.chromium.org/1408123005/diff/140001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/140001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:109: for (size_t i = 0;
i < messages.size(); ++i) {
Nit: I think this should work:

for (auto* message : messages) { Send(message); }

[boliu, 2015-11-04 18:35:32]

https://codereview.chromium.org/1408123005/diff/140001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1408123005/diff/140001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:109: for (size_t i = 0;
i < messages.size(); ++i) {
On 2015/11/04 18:24:29, dcheng wrote:
> Nit: I think this should work:
> 
> for (auto* message : messages) { Send(message); }

Yep. Done

[boliu, 2015-11-04 18:36:12]

Thanks both! :D

[boliu, 2015-11-04 18:53:56]

boliu@chromium.org changed reviewers:
+ tsepez@chromium.org

[boliu, 2015-11-04 18:53:57]

+tsepez for ui/gfx/ipc/gfx_param_traits.cc

Looks like the OWNERS file is interpreted as owned by tsepez@ only for that
file? Not sure if intentional or not

[Tom Sepez, 2015-11-04 19:49:43]

param traits LGTM.

[boliu, 2015-11-04 19:50:49]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-11-04 19:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1408123005/#ps160001
(title: "for each")

[commit-bot: I haz the power, 2015-11-04 19:51:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408123005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408123005/160001

[commit-bot: I haz the power, 2015-11-04 20:44:21]

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

[commit-bot: I haz the power, 2015-11-04 20:44:22]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[boliu, 2015-11-04 21:45:59]

On 2015/11/04 20:44:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

Weird, gfx_ipc.dll link error on windows component build only. gfx::ScrollOffset
is GFX_EXPORT-ed though. Hmm...

[boliu, 2015-11-04 23:12:37]

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

[commit-bot: I haz the power, 2015-11-04 23:13:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408123005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408123005/180001

[boliu, 2015-11-04 23:40:58]

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

[boliu, 2015-11-04 23:40:59]

+danakj to stamp ui/gfx/ipc/gfx_ipc.gyp

I don't know why this is suddenly needed now. But win_chromium_compile_dbg_ng
green so I'm happy.

[danakj, 2015-11-04 23:51:54]

On 2015/11/04 23:40:59, boliu wrote:
> +danakj to stamp ui/gfx/ipc/gfx_ipc.gyp
> 
> I don't know why this is suddenly needed now. But win_chromium_compile_dbg_ng
> green so I'm happy.

I've been adding more methods that are defined in .cc files, and windows seems
to expect you to link things you include more than other compilers in weird
ways. Similar things happened trying to include cc into areas of blink. LGTM

[boliu, 2015-11-04 23:54:08]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2015-11-04 23:54:15]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-11-04 23:54:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1408123005/#ps180001
(title: "fix win compile")

[commit-bot: I haz the power, 2015-11-04 23:55:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408123005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408123005/180001

[commit-bot: I haz the power, 2015-11-05 00:53:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-05 00:54:15]

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