Replaced is_null() with is_valid in SurfaceId and related classes.

This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is
called. destroy_sequence_ had non-zero sequence_ and thus is not null.
Using is_valid() instead of is_null() would help us catch bugs like
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org

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

[Alex Z., 2016-10-18 15:16:50]

Description was changed from

==========
Replaced is_null() with is_valid in SurfaceId and related classes.

BUG=656639
==========

to

==========
Replaced is_null() with is_valid in SurfaceId and related classes.

BUG=656639
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alex Z., 2016-10-18 15:16:51]

Description was changed from

==========
Replaced is_null() with is_valid in SurfaceId and related classes.

BUG=656639
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replaced is_null() with is_valid in SurfaceId and related classes.

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

[Alex Z., 2016-10-18 15:35:12]

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

[commit-bot: I haz the power, 2016-10-18 15:35:27]

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

[commit-bot: I haz the power, 2016-10-18 15:52:29]

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

[commit-bot: I haz the power, 2016-10-18 15:52:30]

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

[Alex Z., 2016-10-18 15:57:14]

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

[commit-bot: I haz the power, 2016-10-18 15:57:39]

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

[commit-bot: I haz the power, 2016-10-18 16:15:23]

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

[commit-bot: I haz the power, 2016-10-18 16:15:24]

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...)

[Alex Z., 2016-10-18 18:36:54]

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

[commit-bot: I haz the power, 2016-10-18 18:47:31]

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

[commit-bot: I haz the power, 2016-10-18 19:08:05]

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

[commit-bot: I haz the power, 2016-10-18 19:08:06]

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

[Alex Z., 2016-10-18 22:13:17]

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

[commit-bot: I haz the power, 2016-10-18 22:13:52]

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

[commit-bot: I haz the power, 2016-10-18 22:40:27]

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

[commit-bot: I haz the power, 2016-10-18 22:40:27]

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...)

[Alex Z., 2016-10-19 16:05:46]

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

[commit-bot: I haz the power, 2016-10-19 16:06:19]

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

[commit-bot: I haz the power, 2016-10-19 16:59:59]

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

[commit-bot: I haz the power, 2016-10-19 17:00:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alex Z., 2016-10-19 21:19:31]

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

[commit-bot: I haz the power, 2016-10-19 21:20:23]

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

[commit-bot: I haz the power, 2016-10-19 22:15:47]

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

[commit-bot: I haz the power, 2016-10-19 22:15:48]

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

[Fady Samuel, 2016-10-20 12:15:35]

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

[Fady Samuel, 2016-10-20 12:15:37]

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (left):

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:26: constexpr bool is_null() const { return
local_id_ == 0 && nonce_ == 0; }
Ideally, I'd like to get rid of is_null() because I worry that that could be
misused.

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (right):

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:26: constexpr bool is_valid() const { return
local_id_ != 0; }
I disagree with this. is_valid() means local_id_ != 0 and nonce_ != 0;

[Alex Z., 2016-10-20 15:05:57]

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

[commit-bot: I haz the power, 2016-10-20 15:06:20]

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

[Alex Z., 2016-10-20 15:07:36]

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

[commit-bot: I haz the power, 2016-10-20 15:07:46]

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

[commit-bot: I haz the power, 2016-10-20 16:00:05]

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

[commit-bot: I haz the power, 2016-10-20 16:00:05]

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

[Alex Z., 2016-10-20 17:07:37]

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

[commit-bot: I haz the power, 2016-10-20 17:08:13]

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

[commit-bot: I haz the power, 2016-10-20 17:52:37]

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

[commit-bot: I haz the power, 2016-10-20 17:52:38]

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

[Fady Samuel, 2016-11-01 03:35:40]

It would be nice to revive this CL. I think it's a good cleanup

[Alex Z., 2016-11-02 16:55:06]

On 2016/11/01 03:35:40, Fady Samuel wrote:
> It would be nice to revive this CL. I think it's a good cleanup

Some tests were failing with empty FrameSinkIds so it should pass all trybots
once the client ID flag CL is landed.

[Alex Z., 2016-11-08 21:11:40]

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

[commit-bot: I haz the power, 2016-11-08 21:12:26]

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

[Alex Z., 2016-11-08 21:25:07]

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

[commit-bot: I haz the power, 2016-11-08 21:25:46]

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

[Alex Z., 2016-11-08 21:38:18]

staraz@chromium.org changed reviewers:
+ dtrainor@chromium.org, piman@chromium.org, reveman@chromium.org,
sky@chromium.org, torne@chromium.org

[Alex Z., 2016-11-08 21:38:20]

torne@chromium.org: Please review changes in android_webview/

dtrainor@chromium.org: Please review changes in ui/android/

sky@chromium.org: Please review changes in services/ui/ and ui/aura/mus/

reveman@chromium.org: Please review changes in components/exo/surface.cc

piman@chromium.org: Please review changes in blimp/, cc/, content/ and
ui/compositor/

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (left):

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:26: constexpr bool is_null() const { return
local_id_ == 0 && nonce_ == 0; }
On 2016/10/20 12:15:36, Fady Samuel wrote:
> Ideally, I'd like to get rid of is_null() because I worry that that could be
> misused.

Done.

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (right):

https://codereview.chromium.org/2425923003/diff/180001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:26: constexpr bool is_valid() const { return
local_id_ != 0; }
On 2016/10/20 12:15:36, Fady Samuel wrote:
> I disagree with this. is_valid() means local_id_ != 0 and nonce_ != 0;

Done.

[commit-bot: I haz the power, 2016-11-08 21:51:10]

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

[commit-bot: I haz the power, 2016-11-08 21:51:12]

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

[Alex Z., 2016-11-08 21:54:39]

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

[commit-bot: I haz the power, 2016-11-08 21:55:10]

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

[Fady Samuel, 2016-11-08 22:18:09]

Could you please update your description to explain why you're doing this?

NOT SurfaceId/FrameSinkId/LocalFrameId::is_null() != valid.

We mistakely use is_null() as a proxy for is/is not valid.

Please list changes in logic in is_valid().

[piman, 2016-11-08 23:16:34]

https://codereview.chromium.org/2425923003/diff/300001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:119: // Sometime frame_sink_id_ is null
What does this comment refer to? There's no frame_sink_id_ here. Is it in
reference to destroy_sequence_.frame_sink_id?

With your change, doesn't that mean the DCHECK is incorrect?

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:44: return frame_sink_id_.is_valid() &&
local_frame_id_.is_valid();
This changes the logic. Is that intended?

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_se...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_se...
cc/surfaces/surface_sequence.h:25: bool is_valid() const { return
frame_sink_id.is_valid() && sequence > 0u; }
Ditto, this changes the logic. Is that intended?

[commit-bot: I haz the power, 2016-11-08 23:28:24]

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

[commit-bot: I haz the power, 2016-11-08 23:28:25]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2016-11-09 15:27:47]

Description was changed from

==========
Replaced is_null() with is_valid in SurfaceId and related classes.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId (IDs in the
following paragraphs) to is_valid() with slightly different logic.

is_null() returns true if any field of the class is set to a non-default value.
We used to assume that an ID being not null is equivalent to being valid. The
assumption was logically false and left room for bugs (656639, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is a special
case where FrameSinkId::is_valid() returns true if either of its fields is
non-zero). Thus, a valid ID is always not null but not the other way around

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

[Alex Z., 2016-11-09 15:29:15]

https://codereview.chromium.org/2425923003/diff/300001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:119: // Sometime frame_sink_id_ is null
On 2016/11/08 23:16:34, piman wrote:
> What does this comment refer to? There's no frame_sink_id_ here. Is it in
> reference to destroy_sequence_.frame_sink_id?
> 
> With your change, doesn't that mean the DCHECK is incorrect?

Sorry about the confusion.
That was one of the notes I left while debugging. It must have somehow escaped
the sweep when I removed my debug statements.
I will remove it in the next patchset.

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:44: return frame_sink_id_.is_valid() &&
local_frame_id_.is_valid();
On 2016/11/08 23:16:34, piman wrote:
> This changes the logic. Is that intended?

Yes, it is intended.
We used to assume that a not-null SurfaceId is a valid SurfaceId, which turned
out to be false.
I will add more context to the CL's description.

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_se...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/2425923003/diff/300001/cc/surfaces/surface_se...
cc/surfaces/surface_sequence.h:25: bool is_valid() const { return
frame_sink_id.is_valid() && sequence > 0u; }
On 2016/11/08 23:16:34, piman wrote:
> Ditto, this changes the logic. Is that intended?

Yes. I will update the description to reflex the changes.

[Alex Z., 2016-11-09 15:44:35]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId (IDs in the
following paragraphs) to is_valid() with slightly different logic.

is_null() returns true if any field of the class is set to a non-default value.
We used to assume that an ID being not null is equivalent to being valid. The
assumption was logically false and left room for bugs (656639, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is a special
case where FrameSinkId::is_valid() returns true if either of its fields is
non-zero). Thus, a valid ID is always not null but not the other way around

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId (IDs in the
following paragraphs) to is_valid() with slightly different logic.

is_null() returns true if any field of the class is set to a non-default value.
We used to assume that an ID being not null is equivalent to being valid. The
assumption was logically false and left room for bugs (The one fixed by CL
2447143003, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is a special
case where FrameSinkId::is_valid() returns true if either of its fields is
non-zero). Thus, a valid ID is always not null but not the other way around

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

[Fady Samuel, 2016-11-09 15:54:56]

nits: Could you please point to a specific example or two of where !is_null() is
the wrong thing to test? Also, could you please wrap your description to 80
characters per line? It's a bit more readable that way.

[Alex Z., 2016-11-09 15:58:20]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId (IDs in the
following paragraphs) to is_valid() with slightly different logic.

is_null() returns true if any field of the class is set to a non-default value.
We used to assume that an ID being not null is equivalent to being valid. The
assumption was logically false and left room for bugs (The one fixed by CL
2447143003, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is a special
case where FrameSinkId::is_valid() returns true if either of its fields is
non-zero). Thus, a valid ID is always not null but not the other way around

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns true if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid. The assumption was logically false and left
 room for bugs (The one fixed by CL 2447143003, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around

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

[Alex Z., 2016-11-09 16:18:19]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns true if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid. The assumption was logically false and left
 room for bugs (The one fixed by CL 2447143003, for example).

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around. is_valid() poses a more strict check than 
!is_null() and would help us catch bugs where a class is only partially
 initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long enough for
LayerTreeHostInProcess::SetFrameSinkId() to be called by an IPC. The bug
resulted in SurfaceLayer::destroy_sequence_ having an empty FrameSinkId when
SurfaceLayer::SatisfyDestroySequence() is called. destroy_sequence_ had non-zero
sequence_ and thus is not null. Using is_valid() instead of is_null() would help
us catch bugs like this.

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

[Alex Z., 2016-11-09 16:18:44]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around. is_valid() poses a more strict check than 
!is_null() and would help us catch bugs where a class is only partially
 initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long enough for
LayerTreeHostInProcess::SetFrameSinkId() to be called by an IPC. The bug
resulted in SurfaceLayer::destroy_sequence_ having an empty FrameSinkId when
SurfaceLayer::SatisfyDestroySequence() is called. destroy_sequence_ had non-zero
sequence_ and thus is not null. Using is_valid() instead of is_null() would help
us catch bugs like this.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around. is_valid() poses a more strict check than 
!is_null() and would help us catch bugs where a class is only partially
 initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long 
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
 IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

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

[Alex Z., 2016-11-09 16:19:54]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
 logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
 a special case where FrameSinkId::is_valid() returns true if either of
 its fields is non-zero). Thus, a valid ID is always not null but not
 the other way around. is_valid() poses a more strict check than 
!is_null() and would help us catch bugs where a class is only partially
 initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long 
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
 IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

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

[Alex Z., 2016-11-09 16:20:51]

On 2016/11/09 15:54:56, Fady Samuel wrote:
> nits: Could you please point to a specific example or two of where !is_null()
is
> the wrong thing to test? Also, could you please wrap your description to 80
> characters per line? It's a bit more readable that way.

Done.

[Fady Samuel, 2016-11-09 16:22:25]

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

[commit-bot: I haz the power, 2016-11-09 16:22:51]

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

[Fady Samuel, 2016-11-09 16:24:32]

On 2016/11/09 16:20:51, StarAZ1 wrote:
> On 2016/11/09 15:54:56, Fady Samuel wrote:
> > nits: Could you please point to a specific example or two of where
!is_null()
> is
> > the wrong thing to test? Also, could you please wrap your description to 80
> > characters per line? It's a bit more readable that way.
> 
> Done.

I think it's also worth mentioning that a LocalFrameId that has a 0 nonce is not
valid, but may be not null if the local_id is set. We don't want to deal with
invalid LocalFrameIds in the browser or window server.

[Fady Samuel, 2016-11-09 16:24:32]

On 2016/11/09 16:20:51, StarAZ1 wrote:
> On 2016/11/09 15:54:56, Fady Samuel wrote:
> > nits: Could you please point to a specific example or two of where
!is_null()
> is
> > the wrong thing to test? Also, could you please wrap your description to 80
> > characters per line? It's a bit more readable that way.
> 
> Done.

I think it's also worth mentioning that a LocalFrameId that has a 0 nonce is not
valid, but may be not null if the local_id is set. We don't want to deal with
invalid LocalFrameIds in the browser or window server.

[Alex Z., 2016-11-09 16:29:23]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but it may
pass the !is_null() check if the local_id is set. We do not want to deal with
invalid LocalFrameIds in the browser or window server.

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

[Alex Z., 2016-11-09 16:29:41]

On 2016/11/09 16:24:32, Fady Samuel wrote:
> On 2016/11/09 16:20:51, StarAZ1 wrote:
> > On 2016/11/09 15:54:56, Fady Samuel wrote:
> > > nits: Could you please point to a specific example or two of where
> !is_null()
> > is
> > > the wrong thing to test? Also, could you please wrap your description to
80
> > > characters per line? It's a bit more readable that way.
> > 
> > Done.
> 
> I think it's also worth mentioning that a LocalFrameId that has a 0 nonce is
not
> valid, but may be not null if the local_id is set. We don't want to deal with
> invalid LocalFrameIds in the browser or window server.

Description updated.

[Alex Z., 2016-11-09 16:34:18]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but it may
pass the !is_null() check if the local_id is set. We do not want to deal with
invalid LocalFrameIds in the browser or window server.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

[piman, 2016-11-09 16:35:46]

lgtm

[Fady Samuel, 2016-11-09 16:37:16]

lgtm, I think this is an important change because it prevents us from
accidentally assuming !is_null() means that it's safe to use a LocalFrameId in a
privileged process.

Furthermore, FrameSinkIds are now known ahead of time and so often, cached
SurfaceIds are not null because they contain a non-null and valid FrameSinkId.
However, the SurfaceId is not valid because the LocalFrameId is not valid.

This CL captures this subtlety to avoid bugs in the future.

[sky, 2016-11-09 16:37:32]

LGTM

[commit-bot: I haz the power, 2016-11-09 17:08:03]

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

[commit-bot: I haz the power, 2016-11-09 17:08:05]

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_...)

[reveman, 2016-11-09 17:31:29]

components/exo lgtm

[Alex Z., 2016-11-09 18:41:07]

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

[commit-bot: I haz the power, 2016-11-09 18:41:29]

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

[commit-bot: I haz the power, 2016-11-09 19:36:17]

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

[commit-bot: I haz the power, 2016-11-09 19:36:18]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-11-09 21:00:13]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-11-09 21:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, piman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2425923003/#ps340001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-09 21:00:41]

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

[Alex Z., 2016-11-09 21:03:06]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org, torne@chromium.org
==========

[Alex Z., 2016-11-09 21:05:18]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org, torne@chromium.org
==========

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org
==========

[commit-bot: I haz the power, 2016-11-09 21:07:45]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org
==========

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org
==========

[commit-bot: I haz the power, 2016-11-09 21:07:47]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-11-09 21:15:15]

Description was changed from

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId 
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is 
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an 
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is 
called. destroy_sequence_ had non-zero sequence_ and thus is not null. 
Using is_valid() instead of is_null() would help us catch bugs like 
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org
==========

to

==========
This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is
called. destroy_sequence_ had non-zero sequence_ and thus is not null.
Using is_valid() instead of is_null() would help us catch bugs like
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

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

TBR=dtrainor@chromium.org;torne@chromium.org

Committed: https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2
Cr-Commit-Position: refs/heads/master@{#431029}
==========

[commit-bot: I haz the power, 2016-11-09 21:15:17]

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

[David Trainor- moved to gerrit, 2016-11-09 21:27:44]

blimp/ lgtm

[Torne, 2016-11-10 13:40:44]

android_webview LGTM, sorry for the delay.