Fix Extensions.AppLaunchSource off-by-one error

Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
Committed: https://crrev.com/004905cf6e6644d5d31bba196cb4cb41fb68bb0a
Cr-Commit-Position: refs/heads/master@{#432638}

[wychen, 2016-11-07 19:18:13]

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

[commit-bot: I haz the power, 2016-11-07 19:18:42]

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

[wychen, 2016-11-07 19:19:05]

wychen@chromium.org changed reviewers:
+ benwells@chromium.org

[wychen, 2016-11-07 19:19:06]

PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[wychen, 2016-11-08 00:25:00]

Description was changed from

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
==========

to

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
==========

[wychen, 2016-11-08 00:25:01]

wychen@chromium.org changed reviewers:
+ cylee@chromium.org

[benwells, 2016-11-09 00:50:48]

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
extensions/browser/api/app_runtime/app_runtime_api.cc:56:
app_runtime::LaunchSource::LAUNCH_SOURCE_LAST + 1);
I think it is cleaner to keep using NUM_APP_LAUNCH_SOURCES.

https://codereview.chromium.org/2486453002/diff/1/extensions/common/api/app_r...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/api/app_r...
extensions/common/api/app_runtime.idl:20: // should not be re-ordered or
removed.
Could you also mention this should be kept in sync with the type in constants.h?

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants.h
File extensions/common/constants.h (left):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants...
extensions/common/constants.h:120: // Note the enumeration is used in UMA
histogram so entries
I think you should:
1. Add SOURCE_NONE to this list so it matches the auto generated list
2. Keep this comment
3. Add a comment that this should be kept in sync with the IDL.
4. Keep NUM_APP_LAUNCH_SOURCES.

[wychen, 2016-11-09 01:59:57]

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
extensions/browser/api/app_runtime/app_runtime_api.cc:56:
app_runtime::LaunchSource::LAUNCH_SOURCE_LAST + 1);
On 2016/11/09 00:50:48, benwells (slow) wrote:
> I think it is cleaner to keep using NUM_APP_LAUNCH_SOURCES.

UMA_HISTOGRAM_ENUMERATION will require matched enum type if both |sample| and
|boundary| are enums. Currently they are of different enums.

Since the convention of IDL doesn't include the MAX field, LAST+1 would be the
closest thing. Alternatively, we could also switch to AppLaunchSource. I don't
feel strongly either way.

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants.h
File extensions/common/constants.h (left):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants...
extensions/common/constants.h:120: // Note the enumeration is used in UMA
histogram so entries
On 2016/11/09 00:50:48, benwells (slow) wrote:
> I think you should:
> 1. Add SOURCE_NONE to this list so it matches the auto generated list
> 2. Keep this comment
> 3. Add a comment that this should be kept in sync with the IDL.
> 4. Keep NUM_APP_LAUNCH_SOURCES.

I thought about adding SOURCE_NONE, but SOURCE_UNTRACKED being explicitly
specified 0 worries me. Hopefully it's just me reading too much into it. Can we
prepend SOURCE_NONE as 0 and increment all of the original ones without breaking
anything? I'd be quite happy if we can.

If AppLaunchSource is intended to be identical to LaunchSource, except for one
additional field, NUM_APP_LAUNCH_SOURCES, then I'll also add static_assert()
somewhere to make sure the numbers of items match. In this case, function
GetLaunchSourceEnum() should almost be an identity function, right?

Then it become possible for us to simply use AppLaunchSource in
UMA_HISTOGRAM_ENUMERATION for both sample and boundary.

[benwells, 2016-11-09 03:32:30]

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
extensions/browser/api/app_runtime/app_runtime_api.cc:56:
app_runtime::LaunchSource::LAUNCH_SOURCE_LAST + 1);
On 2016/11/09 01:59:57, wychen wrote:
> On 2016/11/09 00:50:48, benwells (slow) wrote:
> > I think it is cleaner to keep using NUM_APP_LAUNCH_SOURCES.
> 
> UMA_HISTOGRAM_ENUMERATION will require matched enum type if both |sample| and
> |boundary| are enums. Currently they are of different enums.
> 
> Since the convention of IDL doesn't include the MAX field, LAST+1 would be the
> closest thing. Alternatively, we could also switch to AppLaunchSource. I don't
> feel strongly either way.

Ah ... OK I didn't realise they were mismatched currently, it makes sense they
should match so lets leave it as is.

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants.h
File extensions/common/constants.h (left):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants...
extensions/common/constants.h:120: // Note the enumeration is used in UMA
histogram so entries
On 2016/11/09 01:59:57, wychen wrote:
> On 2016/11/09 00:50:48, benwells (slow) wrote:
> > I think you should:
> > 1. Add SOURCE_NONE to this list so it matches the auto generated list
> > 2. Keep this comment
> > 3. Add a comment that this should be kept in sync with the IDL.
> > 4. Keep NUM_APP_LAUNCH_SOURCES.
> 
> I thought about adding SOURCE_NONE, but SOURCE_UNTRACKED being explicitly
> specified 0 worries me. Hopefully it's just me reading too much into it. Can
we
> prepend SOURCE_NONE as 0 and increment all of the original ones without
breaking
> anything? I'd be quite happy if we can.
> 
> If AppLaunchSource is intended to be identical to LaunchSource, except for one
> additional field, NUM_APP_LAUNCH_SOURCES, then I'll also add static_assert()
> somewhere to make sure the numbers of items match. In this case, function
> GetLaunchSourceEnum() should almost be an identity function, right?
> 
> Then it become possible for us to simply use AppLaunchSource in
> UMA_HISTOGRAM_ENUMERATION for both sample and boundary.

Yep. I think it is fine...

There are two things that count:
- we shouldn't change what values get put in the histograms so they are
consistent over time, and
- we should sent the same stuff to javascript.

I think both of these will be unchanged, as:
- the histogram was based on the other enum, and
- the stuff sent to javascript is also based on the other enum.

My reading of the code is that this enum (AppLaunchSource) is used everywhere
and passed in to the API implementation, which converts it to the IDL enum
before doing anything with it.

[wychen, 2016-11-11 04:03:51]

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/browser/api/app_...
extensions/browser/api/app_runtime/app_runtime_api.cc:56:
app_runtime::LaunchSource::LAUNCH_SOURCE_LAST + 1);
On 2016/11/09 03:32:30, benwells (slow) wrote:
> Ah ... OK I didn't realise they were mismatched currently, it makes sense they
> should match so lets leave it as is.

This type mismatch is exactly how I discovered this bug.

I basically went through all the compilation errors after locally adding a
static_assert() https://codereview.chromium.org/2469993002/.

BTW, by using LAST+1, it's essentially just an int, so it's excluded in the
assertion. Sadly there's no way to get the MAX without adding a field in the IDL
file.

https://codereview.chromium.org/2486453002/diff/1/extensions/common/api/app_r...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/api/app_r...
extensions/common/api/app_runtime.idl:20: // should not be re-ordered or
removed.
On 2016/11/09 00:50:48, benwells (slow) wrote:
> Could you also mention this should be kept in sync with the type in
constants.h?

Done.

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants.h
File extensions/common/constants.h (left):

https://codereview.chromium.org/2486453002/diff/1/extensions/common/constants...
extensions/common/constants.h:120: // Note the enumeration is used in UMA
histogram so entries
On 2016/11/09 03:32:30, benwells (slow) wrote:
> On 2016/11/09 01:59:57, wychen wrote:
> > On 2016/11/09 00:50:48, benwells (slow) wrote:
> > > I think you should:
> > > 1. Add SOURCE_NONE to this list so it matches the auto generated list
> > > 2. Keep this comment
> > > 3. Add a comment that this should be kept in sync with the IDL.
> > > 4. Keep NUM_APP_LAUNCH_SOURCES.
> > 
> > I thought about adding SOURCE_NONE, but SOURCE_UNTRACKED being explicitly
> > specified 0 worries me. Hopefully it's just me reading too much into it. Can
> we
> > prepend SOURCE_NONE as 0 and increment all of the original ones without
> breaking
> > anything? I'd be quite happy if we can.
> > 
> > If AppLaunchSource is intended to be identical to LaunchSource, except for
one
> > additional field, NUM_APP_LAUNCH_SOURCES, then I'll also add static_assert()
> > somewhere to make sure the numbers of items match. In this case, function
> > GetLaunchSourceEnum() should almost be an identity function, right?
> > 
> > Then it become possible for us to simply use AppLaunchSource in
> > UMA_HISTOGRAM_ENUMERATION for both sample and boundary.
> 
> Yep. I think it is fine...
> 
> There are two things that count:
> - we shouldn't change what values get put in the histograms so they are
> consistent over time, and
> - we should sent the same stuff to javascript.
> 
> I think both of these will be unchanged, as:
> - the histogram was based on the other enum, and
> - the stuff sent to javascript is also based on the other enum.
> 
> My reading of the code is that this enum (AppLaunchSource) is used everywhere
> and passed in to the API implementation, which converts it to the IDL enum
> before doing anything with it.

Done.

[benwells, 2016-11-14 02:32:31]

https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:82: case extensions::Name:
                                            \
This is all great but I'm wondering if it is overkill. We could just add a
static assert that NUM_BLAH == LAST_BLAH + 1 and static cast in the conversion
routine instead. Depending on the compiler / optimizations that might save code
size.

[wychen, 2016-11-15 22:41:30]

Patchset #4 (id:60001) has been deleted

[wychen, 2016-11-15 22:46:09]

https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:82: case extensions::Name:
                                            \
On 2016/11/14 02:32:31, benwells (slow) wrote:
> This is all great but I'm wondering if it is overkill. We could just add a
> static assert that NUM_BLAH == LAST_BLAH + 1 and static cast in the conversion
> routine instead. Depending on the compiler / optimizations that might save
code
> size.

I agree with you that the switch case would usually result in a lookup table.
However, static_cast'ing these two types to each other has different behavior,
when the input is "out of range". I guess there's nothing depending on this
behavior difference.

NUM_APP_LAUNCH_SOURCES == LAUNCH_SOURCE_LAST + 1 would be certainly needed.
static_assert'ing all the fields doesn't hurt.

[benwells, 2016-11-15 23:34:29]

On 2016/11/15 22:46:09, wychen wrote:
>
https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
> File extensions/browser/api/app_runtime/app_runtime_api.cc (right):
> 
>
https://codereview.chromium.org/2486453002/diff/20001/extensions/browser/api/...
> extensions/browser/api/app_runtime/app_runtime_api.cc:82: case
extensions::Name:
>                                             \
> On 2016/11/14 02:32:31, benwells (slow) wrote:
> > This is all great but I'm wondering if it is overkill. We could just add a
> > static assert that NUM_BLAH == LAST_BLAH + 1 and static cast in the
conversion
> > routine instead. Depending on the compiler / optimizations that might save
> code
> > size.
> 
> I agree with you that the switch case would usually result in a lookup table.
> However, static_cast'ing these two types to each other has different behavior,
> when the input is "out of range". I guess there's nothing depending on this
> behavior difference.
> 
> NUM_APP_LAUNCH_SOURCES == LAUNCH_SOURCE_LAST + 1 would be certainly needed.
> static_assert'ing all the fields doesn't hurt.

OK. I actually meant to lgtm the last patch set and leave this up to you ... if
it was me I'd just check the numbers add up to keep the code simpler, but I
won't block on that.

lgtm again for good measure :)

[wychen, 2016-11-15 23:44:37]

wychen@chromium.org changed reviewers:
+ jwd@chromium.org

[wychen, 2016-11-15 23:44:38]

+jwd@, could you take a look at histogram xml? Thanks!

[jwd, 2016-11-16 16:06:16]

lgtm

[wychen, 2016-11-16 17:39:51]

The CQ bit was checked by wychen@chromium.org

[wychen, 2016-11-16 17:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2486453002/#ps100001
(title: "fix enum in xml")

[commit-bot: I haz the power, 2016-11-16 17:40:36]

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-16 19:58:16]

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

[commit-bot: I haz the power, 2016-11-16 19:58:18]

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

[wychen, 2016-11-16 20:36:40]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2016-11-16 20:37:32]

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-16 22:00:47]

Description was changed from

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
==========

to

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
==========

[commit-bot: I haz the power, 2016-11-16 22:00:48]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-16 22:04:08]

Description was changed from

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405
==========

to

==========
Fix Extensions.AppLaunchSource off-by-one error

BUG=661405

Committed: https://crrev.com/004905cf6e6644d5d31bba196cb4cb41fb68bb0a
Cr-Commit-Position: refs/heads/master@{#432638}
==========

[commit-bot: I haz the power, 2016-11-16 22:04:10]

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