Pass Surface ID namespace to renderer compositors.

Pass Surface ID namespace to renderer compositors.

In order to enable renderer processes to create SurfaceSequences, they
have to receive Surface ID namespaces from the browser process so that
they can avoid collisions.

BUG=478802
Committed: https://crrev.com/c7a5afd66499fc8442e1b3146f55b227638934d8
Cr-Commit-Position: refs/heads/master@{#329708}

Committed: https://crrev.com/b4e2a3bbaabf98abf089396e7ce375b58fa8dfcd
Cr-Commit-Position: refs/heads/master@{#329852}

[kenrb, 2015-05-06 20:05:21]

kenrb@chromium.org changed reviewers:
+ jbauman@chromium.org

[kenrb, 2015-05-06 20:05:21]

kenrb@chromium.org changed required reviewers:
+ jbauman@chromium.org

[kenrb, 2015-05-06 20:10:00]

John: PTAL? I decided to split this off from
https://codereview.chromium.org/1094113003/ to keep the patch sizes down and
unrelated discussions separated.

I think there might be a problem with this, in that DelegatedFrameHost lazily
creates an ID allocator and SurfaceFactory. This might make it possible for the
renderer to try to create a Sequence (e.g. for a child frame SurfaceLayer)
before it has swapped a delegated frame and receives the SurfaceID back.

If my concern is valid, then it might be difficult to ensure this gets to the
renderer before it receives any child frame Surfaces but after the
RenderWidgetCompositor is started.

[jbauman, 2015-05-06 23:33:16]

I think we could allocate the namespace ahead of time (maybe in the
RenderWidgetHelper) and give it to the DelegatedFrameHost when it's created;
that way we could avoid the race condition.

[kenrb, 2015-05-08 19:27:38]

jbauman: PTAL again? It now initializes the renderer namespace value as early as
possible, which is when ViewMsg_New or ViewMsg_CreatingNew_ACK is sent. In this
case it triggers the DelegatedFrameHost to create a SurfaceFactory and
IdAllocator early.

[jbauman, 2015-05-12 00:09:12]

Thanks for figuring this out.

https://codereview.chromium.org/1129883003/diff/20001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1129883003/diff/20001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:385:
client_->DelegatedFrameHostUpdateSurfaceIdNamespace(
Let's just create the id_allocator_ in the constructor, so we don't have to
worry about the namespace changing. It should be fine if multiple
SurfaceFactories have the same namespace as long as the ids they create don't
overlap.

[kenrb, 2015-05-12 20:01:41]

That was a good suggestion, I also noticed a flaw in what I had done before --
if the view was created after the RenderWidgetHost had been initialized, then
the namespace would not get passed. That should be fixed now.

https://codereview.chromium.org/1129883003/diff/20001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/1129883003/diff/20001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:385:
client_->DelegatedFrameHostUpdateSurfaceIdNamespace(
On 2015/05/12 00:09:12, jbauman wrote:
> Let's just create the id_allocator_ in the constructor, so we don't have to
> worry about the namespace changing. It should be fine if multiple
> SurfaceFactories have the same namespace as long as the ids they create don't
> overlap.

Done.

[jbauman, 2015-05-13 00:36:53]

lgtm

[kenrb, 2015-05-13 01:00:40]

kenrb@chromium.org changed reviewers:
+ nasko@chromium.org

[kenrb, 2015-05-13 01:00:40]

kenrb@chromium.org changed required reviewers:
+ nasko@chromium.org

[kenrb, 2015-05-13 01:00:40]

Nasko can you please give content owner review?

[nasko, 2015-05-13 13:51:25]

LGTM with nits.

https://codereview.chromium.org/1129883003/diff/40001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/common/view_mes...
content/common/view_messages.h:538: 
nit: No need for extra empty line here.

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:80: void
SetSurfaceIdNamespace(uint32 surface_id_namespace);
uint32_t?

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/render...
content/renderer/render_widget.h:447: void OnSetSurfaceIdNamespace(uint32
surface_id_namespace);
uint32_t?

[kenrb, 2015-05-13 19:56:57]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-05-13 19:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1129883003/#ps60001
(title: "Nits fixed")

[commit-bot: I haz the power, 2015-05-13 19:57:08]

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

[kenrb, 2015-05-13 19:57:09]

https://codereview.chromium.org/1129883003/diff/40001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/common/view_mes...
content/common/view_messages.h:538: 
On 2015/05/13 13:51:24, nasko wrote:
> nit: No need for extra empty line here.

Done.

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:80: void
SetSurfaceIdNamespace(uint32 surface_id_namespace);
On 2015/05/13 13:51:24, nasko wrote:
> uint32_t?

Done.

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/1129883003/diff/40001/content/renderer/render...
content/renderer/render_widget.h:447: void OnSetSurfaceIdNamespace(uint32
surface_id_namespace);
On 2015/05/13 13:51:25, nasko wrote:
> uint32_t?

Done.

[commit-bot: I haz the power, 2015-05-13 20:37:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-13 20:38:17]

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

[jsbell, 2015-05-13 22:23:47]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1131673009/ by jsbell@chromium.org.

The reason for reverting is: Speculative revert to address widespread blink
layout test failures on mac following a chromium revision in range
329707-329709.

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9%20%28r...

No stacks but sample output:

STDERR: Assertion failed: (impl_.get() != nullptr), function operator->, file
../../base/memory/scoped_ptr.h, line 386.

We'll try reproducing locally and watch the bots. We can reland if this does not
green up the webkit waterfall..

[kenrb, 2015-05-14 14:48:36]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-05-14 14:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1129883003/#ps80001
(title: "Fix Mac bug.")

[commit-bot: I haz the power, 2015-05-14 14:49:23]

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

[dcheng, 2015-05-14 14:53:37]

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

[dcheng, 2015-05-14 14:53:38]

https://codereview.chromium.org/1129883003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1129883003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1567: if
(delegated_frame_host_)
Can we add a comment that explains why this can happen on Mac but not Aura?

[commit-bot: I haz the power, 2015-05-14 15:05:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-14 15:07:03]

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