Centralize FrameSinkId allocation in one place

Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in RenderWidgetHostImpl.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2691363002
Cr-Commit-Position: refs/heads/master@{#452104}
Committed: https://chromium.googlesource.com/chromium/src/+/cab2befb20c03d2d91eedf162e92ffe8db80e44f

[xlai (Olivia), 2017-02-14 21:07:53]

Description was changed from

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in once place.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
==========

to

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in once place.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[xlai (Olivia), 2017-02-14 21:08:00]

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

[commit-bot: I haz the power, 2017-02-14 21:08:25]

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

[xlai (Olivia), 2017-02-14 21:10:40]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2017-02-14 21:49:05]

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

[commit-bot: I haz the power, 2017-02-14 21:49:41]

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

[xlai (Olivia), 2017-02-14 22:13:44]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2017-02-14 22:14:29]

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

[commit-bot: I haz the power, 2017-02-14 22:15:06]

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

[commit-bot: I haz the power, 2017-02-14 22:42:19]

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

[commit-bot: I haz the power, 2017-02-14 22:42:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xlai (Olivia), 2017-02-15 15:26:42]

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

[xlai (Olivia), 2017-02-15 15:27:00]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-15 15:27:06]

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

[xlai (Olivia), 2017-02-15 16:57:58]

Description was changed from

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in once place.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in RenderWidgetHostImpl.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-02-15 17:31:13]

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

[commit-bot: I haz the power, 2017-02-15 17:31:14]

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

[xlai (Olivia), 2017-02-15 18:05:45]

xlai@chromium.org changed reviewers:
+ boliu@chromium.org, danakj@chromium.org, fsamuel@chromium.org

[xlai (Olivia), 2017-02-15 18:05:46]

danakj@chromium.org: Please review changes in cc/, content/browser/compositor,
ui/compositor.

boliu@chromium.org: Please review changes in android_webview/,
content/browser/renderer_host/.

fsamuel@chromium.org: Please check whether the change aligns with the intended
way of FrameSinkId allocation in
browser and renderer.

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
File android_webview/browser/surfaces_instance.cc (right):

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
android_webview/browser/surfaces_instance.cc:91: return
frame_sink_id_allocator_.NextFrameSinkId();
This particular change will change the behavior to 
cc::FrameSinkId(0, next_sink_id++);
I think this is a missed place from the last update.

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

https://codereview.chromium.org/2691363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:100: #include
"ui/compositor/compositor.h"  // nogncheck
Need nogncheck to pass Android analyze. This is same as
https://cs.chromium.org/chromium/src/content/browser/compositor/surface_utils...

[boliu, 2017-02-15 19:59:20]

nice, thanks for following up!

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:14: FrameSinkId NextFrameSinkId() { return
FrameSinkId(0, next_sink_id_++); }
should have a comment here why the first part must be 0

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:17: uint32_t next_sink_id_ = 1u;
DISALLOW_COPY_AND_ASSIGN?

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

https://codereview.chromium.org/2691363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:100: #include
"ui/compositor/compositor.h"  // nogncheck
On 2017/02/15 18:05:46, xlai (Olivia) wrote:
> Need nogncheck to pass Android analyze. This is same as
>
https://cs.chromium.org/chromium/src/content/browser/compositor/surface_utils...

So he problem is gn check isn't aware of OS defines, and ui/compositor is not a
dependency on android. Probably worth a comment

https://codereview.chromium.org/2691363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:441: return
cc::FrameSinkId(
keep the comment that this scheme must be unique with cc::FrameSinkIdAllocator?

[danakj, 2017-02-15 20:18:10]

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:14: FrameSinkId NextFrameSinkId() { return
FrameSinkId(0, next_sink_id_++); }
On 2017/02/15 19:59:20, boliu wrote:
> should have a comment here why the first part must be 0

It seems very action-at-a-distance to decide 0 here in order to not collide with
RenderWidgetHostImpl (if I am understanding the reasoning correctly?)

It would be more straightforward I think to take the 0 in the constructor, or as
a parameter to this method, and have the callers pass 0 with a comment that 0
will be different than RenderWidgetHostImpl. Also then RWHI could use this
class?

wdyt?

https://codereview.chromium.org/2691363002/diff/60001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (left):

https://codereview.chromium.org/2691363002/diff/60001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:692: // The
FrameSinkId generated here must be unique with
This comment goes away, but we lose any notion of the importance that this
doesn't collide with RenderWidgetHostImpl, which seems bad. It's also not
tracable in the code anymore.

How can we make the code actually enforce this instead, or connect it back to a
place that understands it at least, like we do here?

[Fady Samuel, 2017-02-15 20:20:58]

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
File android_webview/browser/surfaces_instance.cc (right):

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
android_webview/browser/surfaces_instance.cc:91: return
frame_sink_id_allocator_.NextFrameSinkId();
On 2017/02/15 18:05:46, xlai (Olivia) wrote:
> This particular change will change the behavior to 
> cc::FrameSinkId(0, next_sink_id++);
> I think this is a missed place from the last update.

Thanks for catching this, Olivia! :D

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:12: class FrameSinkIdAllocator {
Class level comment explaining the purpose of this class.

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:14: FrameSinkId NextFrameSinkId() { return
FrameSinkId(0, next_sink_id_++); }
On 2017/02/15 19:59:20, boliu wrote:
> should have a comment here why the first part must be 0

Actually I'd prefer that the client_id be a param into the constructor.

[Fady Samuel, 2017-02-15 20:20:59]

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
File android_webview/browser/surfaces_instance.cc (right):

https://codereview.chromium.org/2691363002/diff/60001/android_webview/browser...
android_webview/browser/surfaces_instance.cc:91: return
frame_sink_id_allocator_.NextFrameSinkId();
On 2017/02/15 18:05:46, xlai (Olivia) wrote:
> This particular change will change the behavior to 
> cc::FrameSinkId(0, next_sink_id++);
> I think this is a missed place from the last update.

Thanks for catching this, Olivia! :D

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:12: class FrameSinkIdAllocator {
Class level comment explaining the purpose of this class.

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:14: FrameSinkId NextFrameSinkId() { return
FrameSinkId(0, next_sink_id_++); }
On 2017/02/15 19:59:20, boliu wrote:
> should have a comment here why the first part must be 0

Actually I'd prefer that the client_id be a param into the constructor.

[xlai (Olivia), 2017-02-15 22:38:36]

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:12: class FrameSinkIdAllocator {
On 2017/02/15 20:20:59, Fady Samuel wrote:
> Class level comment explaining the purpose of this class.

Done.

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:14: FrameSinkId NextFrameSinkId() { return
FrameSinkId(0, next_sink_id_++); }
On 2017/02/15 20:20:59, Fady Samuel wrote:
> On 2017/02/15 19:59:20, boliu wrote:
> > should have a comment here why the first part must be 0
> 
> Actually I'd prefer that the client_id be a param into the constructor.

Done.

https://codereview.chromium.org/2691363002/diff/60001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:17: uint32_t next_sink_id_ = 1u;
On 2017/02/15 19:59:20, boliu wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2691363002/diff/60001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (left):

https://codereview.chromium.org/2691363002/diff/60001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:692: // The
FrameSinkId generated here must be unique with
On 2017/02/15 20:18:10, danakj wrote:
> This comment goes away, but we lose any notion of the importance that this
> doesn't collide with RenderWidgetHostImpl, which seems bad. It's also not
> tracable in the code anymore.
> 
> How can we make the code actually enforce this instead, or connect it back to
a
> place that understands it at least, like we do here?

Done. I added the comment back in places where FrameSinkIdAllocator is
constructed.

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

https://codereview.chromium.org/2691363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:100: #include
"ui/compositor/compositor.h"  // nogncheck
On 2017/02/15 19:59:20, boliu wrote:
> On 2017/02/15 18:05:46, xlai (Olivia) wrote:
> > Need nogncheck to pass Android analyze. This is same as
> >
>
https://cs.chromium.org/chromium/src/content/browser/compositor/surface_utils...
> 
> So he problem is gn check isn't aware of OS defines, and ui/compositor is not
a
> dependency on android. Probably worth a comment

Done.

https://codereview.chromium.org/2691363002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:441: return
cc::FrameSinkId(
On 2017/02/15 19:59:20, boliu wrote:
> keep the comment that this scheme must be unique with
cc::FrameSinkIdAllocator?

Done.

[boliu, 2017-02-15 23:58:56]

lgtm

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:179: 0 /* client_id,
not to conflict with the client_id
nit: 0u everwhere

[Fady Samuel, 2017-02-17 14:00:24]

LGTM + nit

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:179: 0 /* client_id,
not to conflict with the client_id
nit: Maybe add a constexpr at the top of the file instead and put the comment
there. This comment looks a bit awkward inline here.

[Fady Samuel, 2017-02-17 14:01:52]

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:13: // -increasing sink_id.
nit: put the hyphen (the -) on the previous line or the entire compound word in
the second line?

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:17: : client_id_(client_id),
next_sink_id_(1u) {}
nit: New line after this.

[xlai (Olivia), 2017-02-17 15:55:11]

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:13: // -increasing sink_id.
On 2017/02/17 14:01:52, Fady Samuel wrote:
> nit: put the hyphen (the -) on the previous line or the entire compound word
in
> the second line?

Done.

https://codereview.chromium.org/2691363002/diff/80001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_id_allocator.h:17: : client_id_(client_id),
next_sink_id_(1u) {}
On 2017/02/17 14:01:52, Fady Samuel wrote:
> nit: New line after this.

Done.

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:179: 0 /* client_id,
not to conflict with the client_id
On 2017/02/15 23:58:56, boliu wrote:
> nit: 0u everwhere

Done.

https://codereview.chromium.org/2691363002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:179: 0 /* client_id,
not to conflict with the client_id
On 2017/02/17 14:00:24, Fady Samuel wrote:
> nit: Maybe add a constexpr at the top of the file instead and put the comment
> there. This comment looks a bit awkward inline here.

Done.

[Fady Samuel, 2017-02-17 16:00:30]

Please pass this along to another cc OWNER as danakj is on vacation.

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
cc/surfaces/frame_sink_id_allocator.h:16: constexpr
FrameSinkIdAllocator(uint32_t client_id)
Another nit, just noticed. This needs explicit.

[xlai (Olivia), 2017-02-20 18:28:49]

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

[commit-bot: I haz the power, 2017-02-20 18:29:04]

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

[commit-bot: I haz the power, 2017-02-20 20:41:22]

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

[commit-bot: I haz the power, 2017-02-20 20:41:23]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-02-21 22:56:57]

LGTM

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
cc/surfaces/frame_sink_id_allocator.h:16: constexpr
FrameSinkIdAllocator(uint32_t client_id)
On 2017/02/17 16:00:30, Fady Samuel wrote:
> Another nit, just noticed. This needs explicit.

Presubmit should warn about this I thought? Does the constexpr mess it up?

[xlai (Olivia), 2017-02-22 16:24:07]

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
File cc/surfaces/frame_sink_id_allocator.h (right):

https://codereview.chromium.org/2691363002/diff/100001/cc/surfaces/frame_sink...
cc/surfaces/frame_sink_id_allocator.h:16: constexpr
FrameSinkIdAllocator(uint32_t client_id)
On 2017/02/21 22:56:56, danakj wrote:
> On 2017/02/17 16:00:30, Fady Samuel wrote:
> > Another nit, just noticed. This needs explicit.
> 
> Presubmit should warn about this I thought? Does the constexpr mess it up?

Done, added explicit. I don't think constexpr and explicit would mess up; they
indicate that these constructor is both in-line and doesn't allow copy
initialization. A lot of ui/gfx/geometry classes use this combination of
keywords.

[xlai (Olivia), 2017-02-22 16:24:13]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-02-22 16:24:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, boliu@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2691363002/#ps120001
(title: "explicit")

[commit-bot: I haz the power, 2017-02-22 16:24:30]

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

[danakj, 2017-02-22 16:25:50]

On Wed, Feb 22, 2017 at 11:24 AM, <xlai@chromium.org> wrote:

>
> https://codereview.chromium.org/2691363002/diff/100001/cc/
> surfaces/frame_sink_id_allocator.h
> File cc/surfaces/frame_sink_id_allocator.h (right):
>
> https://codereview.chromium.org/2691363002/diff/100001/cc/
> surfaces/frame_sink_id_allocator.h#newcode16
> cc/surfaces/frame_sink_id_allocator.h:16: constexpr
> FrameSinkIdAllocator(uint32_t client_id)
> On 2017/02/21 22:56:56, danakj wrote:
> > On 2017/02/17 16:00:30, Fady Samuel wrote:
> > > Another nit, just noticed. This needs explicit.
> >
> > Presubmit should warn about this I thought? Does the constexpr mess it
> up?
>
> Done, added explicit. I don't think constexpr and explicit would mess
> up; they indicate that these constructor is both in-line and doesn't
> allow copy initialization. A lot of ui/gfx/geometry classes use this
> combination of keywords.
>

I mean, did they mess up the presubmit warning? How did this get uploaded
without the explicit?


>
> https://codereview.chromium.org/2691363002/
>

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

[xlai (Olivia), 2017-02-22 16:33:54]

On 2017/02/22 16:25:50, danakj wrote:
> On Wed, Feb 22, 2017 at 11:24 AM, <mailto:xlai@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2691363002/diff/100001/cc/
> > surfaces/frame_sink_id_allocator.h
> > File cc/surfaces/frame_sink_id_allocator.h (right):
> >
> > https://codereview.chromium.org/2691363002/diff/100001/cc/
> > surfaces/frame_sink_id_allocator.h#newcode16
> > cc/surfaces/frame_sink_id_allocator.h:16: constexpr
> > FrameSinkIdAllocator(uint32_t client_id)
> > On 2017/02/21 22:56:56, danakj wrote:
> > > On 2017/02/17 16:00:30, Fady Samuel wrote:
> > > > Another nit, just noticed. This needs explicit.
> > >
> > > Presubmit should warn about this I thought? Does the constexpr mess it
> > up?
> >
> > Done, added explicit. I don't think constexpr and explicit would mess
> > up; they indicate that these constructor is both in-line and doesn't
> > allow copy initialization. A lot of ui/gfx/geometry classes use this
> > combination of keywords.
> >
> 
> I mean, did they mess up the presubmit warning? How did this get uploaded
> without the explicit?
> 

Nope, there was no presubmit warning.

[danakj, 2017-02-22 16:36:11]

On Wed, Feb 22, 2017 at 11:33 AM, <xlai@chromium.org> wrote:

> On 2017/02/22 16:25:50, danakj wrote:
> > On Wed, Feb 22, 2017 at 11:24 AM, <mailto:xlai@chromium.org> wrote:
> >
> > >
> > > https://codereview.chromium.org/2691363002/diff/100001/cc/
> > > surfaces/frame_sink_id_allocator.h
> > > File cc/surfaces/frame_sink_id_allocator.h (right):
> > >
> > > https://codereview.chromium.org/2691363002/diff/100001/cc/
> > > surfaces/frame_sink_id_allocator.h#newcode16
> > > cc/surfaces/frame_sink_id_allocator.h:16: constexpr
> > > FrameSinkIdAllocator(uint32_t client_id)
> > > On 2017/02/21 22:56:56, danakj wrote:
> > > > On 2017/02/17 16:00:30, Fady Samuel wrote:
> > > > > Another nit, just noticed. This needs explicit.
> > > >
> > > > Presubmit should warn about this I thought? Does the constexpr mess
> it
> > > up?
> > >
> > > Done, added explicit. I don't think constexpr and explicit would mess
> > > up; they indicate that these constructor is both in-line and doesn't
> > > allow copy initialization. A lot of ui/gfx/geometry classes use this
> > > combination of keywords.
> > >
> >
> > I mean, did they mess up the presubmit warning? How did this get uploaded
> > without the explicit?
> >
>
> Nope, there was no presubmit warning.
>

It looks like constexpr breaks it.

/usr/local/google/home/danakj/s/c/src/cc/layers/layer.h:61:
 Single-parameter constructors should be marked explicit.
 [runtime/explicit] [5]

I get that for a constructor without it.

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

[commit-bot: I haz the power, 2017-02-22 17:18:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487780653651760,
"parent_rev": "5dae4677eefa9631e709b9d82b62417b01a98dc0", "commit_rev":
"cab2befb20c03d2d91eedf162e92ffe8db80e44f"}

[commit-bot: I haz the power, 2017-02-22 17:18:50]

Description was changed from

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in RenderWidgetHostImpl.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Centralize FrameSinkId allocation in one place

Currently the allocation of cc::FrameSinkId in render_host_view are scattered
around for different platforms. This CL puts it all in RenderWidgetHostImpl.

In addition, this CL introduced frame_sink_id_allocator so that there will be
no duplicate id generation code in different context factories.


BUG=685777
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2691363002
Cr-Commit-Position: refs/heads/master@{#452104}
Committed:
https://chromium.googlesource.com/chromium/src/+/cab2befb20c03d2d91eedf162e92...
==========

[commit-bot: I haz the power, 2017-02-22 17:18:54]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/cab2befb20c03d2d91eedf162e92...