Move GPU blacklist and driver bug workaround list from json to data struct.

Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2756793003
Cr-Commit-Position: refs/heads/master@{#461282}
Committed: https://chromium.googlesource.com/chromium/src/+/2602bcf4e08d2cb554520b42246f68ac22426bc8

[Zhenyao Mo, 2017-03-16 23:36:28]

Description was changed from

==========
[WIP] Move GPU blacklist and driver bug workaround list from json to data
struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
==========

to

==========
[WIP] Move GPU blacklist and driver bug workaround list from json to data
struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2017-03-17 17:29:07]

Description was changed from

==========
[WIP] Move GPU blacklist and driver bug workaround list from json to data
struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[WIP] Move GPU blacklist and driver bug workaround list from json to data
struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2017-03-27 23:20:51]

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

[commit-bot: I haz the power, 2017-03-27 23:21:36]

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-03-27 23:26:25]

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

[commit-bot: I haz the power, 2017-03-27 23:26:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Zhenyao Mo, 2017-03-28 18:06:03]

Description was changed from

==========
[WIP] Move GPU blacklist and driver bug workaround list from json to data
struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2017-03-28 18:08:12]

Description was changed from

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2017-03-28 18:08:22]

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

[commit-bot: I haz the power, 2017-03-28 18:09: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-03-28 18:13:13]

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

[commit-bot: I haz the power, 2017-03-28 18:13:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Zhenyao Mo, 2017-03-28 18:20:37]

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

[commit-bot: I haz the power, 2017-03-28 18:20:57]

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-03-28 19:27:47]

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

[commit-bot: I haz the power, 2017-03-28 19:27:48]

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

[Zhenyao Mo, 2017-03-28 21:57:31]

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

[commit-bot: I haz the power, 2017-03-28 21:59:13]

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

[Zhenyao Mo, 2017-03-28 22:16:46]

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

[commit-bot: I haz the power, 2017-03-28 22:17:32]

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-03-28 22:46:18]

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

[commit-bot: I haz the power, 2017-03-28 22:46:19]

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

[Zhenyao Mo, 2017-03-28 22:58:28]

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

[commit-bot: I haz the power, 2017-03-28 22:59:08]

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-03-28 23:39:59]

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

[commit-bot: I haz the power, 2017-03-28 23:40:00]

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

[Zhenyao Mo, 2017-03-29 00:00:42]

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

[commit-bot: I haz the power, 2017-03-29 00:01:56]

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-03-29 00:58:37]

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

[commit-bot: I haz the power, 2017-03-29 00:58:38]

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

[Zhenyao Mo, 2017-03-29 03:31:22]

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

[commit-bot: I haz the power, 2017-03-29 03:32:20]

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-03-29 04:38:45]

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

[commit-bot: I haz the power, 2017-03-29 04:38:46]

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

[Zhenyao Mo, 2017-03-29 21:41:22]

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

[commit-bot: I haz the power, 2017-03-29 21:42:19]

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

[Zhenyao Mo, 2017-03-29 22:27:54]

zmo@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Zhenyao Mo, 2017-03-29 22:27:55]

Finally this is ready

kbr: please review
brettw: FYI (feel free to also review)
piman: content/ OWNER approval
rdevlin: extensions/ OWNER approval

[Devlin, 2017-03-29 22:30:38]

extensions lgtm

[Zhenyao Mo, 2017-03-29 23:44:02]

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

[commit-bot: I haz the power, 2017-03-29 23:44:56]

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-03-30 01:47:32]

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

[commit-bot: I haz the power, 2017-03-30 01:47:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[brettw, 2017-03-30 22:54:40]

Thanks for getting to this!

I notice that the list is still global for all systems, which means there's a
brute-force search through a complicated list for all platforms, even though I'm
on Windows. If I recall, very few of the entries apply to Windows so this will
save a good amount of working.

When we first discussed this approach I was imagining that the JSON would get
converted to code at build time, and then that would filter the list by the
current platform. Pregenerating it is OK, but then I'd say we should have *_mac,
*_win, *_android etc. versions of the blacklist.

I think this shouldn't be too hard, but we can do this in a followup if it's
more convenient for you.

I didn't check into this, but you'll want to make sure the process for updating
the blacklist is document really obviously in the places people will be looking
when they change things (like the JSON file).

[Zhenyao Mo, 2017-03-30 23:09:35]

On 2017/03/30 22:54:40, brettw (plz ping after 24h) wrote:
> Thanks for getting to this!
> 
> I notice that the list is still global for all systems, which means there's a
> brute-force search through a complicated list for all platforms, even though
I'm
> on Windows. If I recall, very few of the entries apply to Windows so this will
> save a good amount of working.

> 
> When we first discussed this approach I was imagining that the JSON would get
> converted to code at build time, and then that would filter the list by the
> current platform. Pregenerating it is OK, but then I'd say we should have
*_mac,
> *_win, *_android etc. versions of the blacklist.
> 
> I think this shouldn't be too hard, but we can do this in a followup if it's
> more convenient for you.

Originally we have to process each entry from json to Entry data structure,
that's where the time is spent. Now entries are pre-processed by python script,
there is no cost for that at run time.

At run time, the cost for a entry that doesn't apply to a specific OS is trivia
is already optimized because I put OS condition matching the first thing when
applying an entry to make decision, and bypass the rest conditions if it's not a
match.

> 
> I didn't check into this, but you'll want to make sure the process for
updating
> the blacklist is document really obviously in the places people will be
looking
> when they change things (like the JSON file).

I plan to add a presubmit check for this, but this CL is already huge.  Will do
that as a followup.

[brettw, 2017-03-30 23:15:06]

On 2017/03/30 23:09:35, Zhenyao Mo wrote:
> On 2017/03/30 22:54:40, brettw (plz ping after 24h) wrote:
> > Thanks for getting to this!
> > 
> > I notice that the list is still global for all systems, which means there's
a
> > brute-force search through a complicated list for all platforms, even though
> I'm
> > on Windows. If I recall, very few of the entries apply to Windows so this
will
> > save a good amount of working.
> 
> > 
> > When we first discussed this approach I was imagining that the JSON would
get
> > converted to code at build time, and then that would filter the list by the
> > current platform. Pregenerating it is OK, but then I'd say we should have
> *_mac,
> > *_win, *_android etc. versions of the blacklist.
> > 
> > I think this shouldn't be too hard, but we can do this in a followup if it's
> > more convenient for you.
> 
> Originally we have to process each entry from json to Entry data structure,
> that's where the time is spent. Now entries are pre-processed by python
script,
> there is no cost for that at run time.
> 
> At run time, the cost for a entry that doesn't apply to a specific OS is
trivia
> is already optimized because I put OS condition matching the first thing when
> applying an entry to make decision, and bypass the rest conditions if it's not
a
> match.

OK, I guess the current logic is lightweight enough and the lists are small
enough it doesn't really matter.



> > I didn't check into this, but you'll want to make sure the process for
> updating
> > the blacklist is document really obviously in the places people will be
> looking
> > when they change things (like the JSON file).
> 
> I plan to add a presubmit check for this, but this CL is already huge.  Will
do
> that as a followup.

I'm not asking for a complicated presubmit (although I agree that's a nice thing
to add later). I just want a description of what steps you need to do at the top
of the JSON file (or wherever you think is better).

[Zhenyao Mo, 2017-03-30 23:42:54]

On 2017/03/30 23:15:06, brettw (plz ping after 24h) wrote:
> On 2017/03/30 23:09:35, Zhenyao Mo wrote:
> > On 2017/03/30 22:54:40, brettw (plz ping after 24h) wrote:
> > > Thanks for getting to this!
> > > 
> > > I notice that the list is still global for all systems, which means
there's
> a
> > > brute-force search through a complicated list for all platforms, even
though
> > I'm
> > > on Windows. If I recall, very few of the entries apply to Windows so this
> will
> > > save a good amount of working.
> > 
> > > 
> > > When we first discussed this approach I was imagining that the JSON would
> get
> > > converted to code at build time, and then that would filter the list by
the
> > > current platform. Pregenerating it is OK, but then I'd say we should have
> > *_mac,
> > > *_win, *_android etc. versions of the blacklist.
> > > 
> > > I think this shouldn't be too hard, but we can do this in a followup if
it's
> > > more convenient for you.
> > 
> > Originally we have to process each entry from json to Entry data structure,
> > that's where the time is spent. Now entries are pre-processed by python
> script,
> > there is no cost for that at run time.
> > 
> > At run time, the cost for a entry that doesn't apply to a specific OS is
> trivia
> > is already optimized because I put OS condition matching the first thing
when
> > applying an entry to make decision, and bypass the rest conditions if it's
not
> a
> > match.
> 
> OK, I guess the current logic is lightweight enough and the lists are small
> enough it doesn't really matter.

I did some rough profiling on my linux just to make sure.

Total 173 entries, 44 applies to linux.

With all entries included, total time to make a decision is 0.968 ms (worst case
of a couple runs)

From which, the time to go through 173 - 44 = 129 entries that don't apply on
linux is 0.009ms.

> 
> 
> 
> > > I didn't check into this, but you'll want to make sure the process for
> > updating
> > > the blacklist is document really obviously in the places people will be
> > looking
> > > when they change things (like the JSON file).
> > 
> > I plan to add a presubmit check for this, but this CL is already huge.  Will
> do
> > that as a followup.
> 
> I'm not asking for a complicated presubmit (although I agree that's a nice
thing
> to add later). I just want a description of what steps you need to do at the
top
> of the JSON file (or wherever you think is better).

Sure. Will add at the end of the json file where people usually add new entries.

[piman, 2017-03-30 23:44:41]

Overall, this is great. I have a few nits and one question, but no major issue.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
File gpu/config/gpu_blacklist.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
gpu/config/gpu_blacklist.h:17: static GpuBlacklist* Create();
nit: if it's not too much work, could we return a std::unique_ptr<GpuBlacklist>
here? To make ownership clear.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
gpu/config/gpu_blacklist.h:21: GpuBlacklist(const GpuControlListData& data);
nit: explicit

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:587: // TODO(XXX): Remove this
nit: can we? :)
Otherwise, can you add your username here instead of XXX?

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:207: GpuControlList(const GpuControlListData&
data);
nit: explicit

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h:9: // DO NOT
EDIT!
Do you think it's really worth checking in the generated files, as opposed to
generating them as part of the build?

We do it for the command buffer generator, mostly because the generated code is
security-sensitive, and changes to the generator itself are virtually impossible
to review for security issues (because the code snippets are all over the
place), so we want to review the generated code itself, instead.

In this case, I think the generated code is pretty meaningless, so I don't see
value in checking it in. Not doing so also means you don't have to worry about
clang-format etc.
Thoughts?

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_unittest.cc:38: GpuControlList* Create(size_t
entry_count, const Entry* entries) {
nit: return std::unique_ptr<GpuControlList>

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
File gpu/config/gpu_driver_bug_list.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_list.h:23: static GpuDriverBugList* Create();
nit: ditto re: std::unique_ptr

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_list.h:38: GpuDriverBugList(const GpuControlListData&
data);
nit: explicit

[Zhenyao Mo, 2017-03-30 23:50:21]

On 2017/03/30 23:44:41, piman wrote:
> Overall, this is great. I have a few nits and one question, but no major
issue.
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
> File gpu/config/gpu_blacklist.h (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
> gpu/config/gpu_blacklist.h:17: static GpuBlacklist* Create();
> nit: if it's not too much work, could we return a
std::unique_ptr<GpuBlacklist>
> here? To make ownership clear.
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
> gpu/config/gpu_blacklist.h:21: GpuBlacklist(const GpuControlListData& data);
> nit: explicit
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> File gpu/config/gpu_control_list.cc (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> gpu/config/gpu_control_list.cc:587: // TODO(XXX): Remove this
> nit: can we? :)
> Otherwise, can you add your username here instead of XXX?
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> File gpu/config/gpu_control_list.h (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> gpu/config/gpu_control_list.h:207: GpuControlList(const GpuControlListData&
> data);
> nit: explicit
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> File gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h:9: // DO NOT
> EDIT!
> Do you think it's really worth checking in the generated files, as opposed to
> generating them as part of the build?
> 
> We do it for the command buffer generator, mostly because the generated code
is
> security-sensitive, and changes to the generator itself are virtually
impossible
> to review for security issues (because the code snippets are all over the
> place), so we want to review the generated code itself, instead.
> 
> In this case, I think the generated code is pretty meaningless, so I don't see
> value in checking it in. Not doing so also means you don't have to worry about
> clang-format etc.
> Thoughts?

Can we do it in a followup CL?  I think for the first round, I want everything
to be uploaded here, so myself and reviewers can examine.

We can move it to build time generation after this lands (then Brett's idea of
only generating entries per platform can also be implemented)

> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> File gpu/config/gpu_control_list_unittest.cc (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
> gpu/config/gpu_control_list_unittest.cc:38: GpuControlList* Create(size_t
> entry_count, const Entry* entries) {
> nit: return std::unique_ptr<GpuControlList>
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
> File gpu/config/gpu_driver_bug_list.h (right):
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
> gpu/config/gpu_driver_bug_list.h:23: static GpuDriverBugList* Create();
> nit: ditto re: std::unique_ptr
> 
>
https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
> gpu/config/gpu_driver_bug_list.h:38: GpuDriverBugList(const
GpuControlListData&
> data);
> nit: explicit

[Ken Russell (switch to Gerrit), 2017-03-31 02:37:07]

Awesome work Mo. This is a tremendous improvement. Several smaller questions and
comments, but I think this is ready to go as it stands, and the sooner the
better. LGTM

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.cc:31: 1,                      //
features size
Could consider revising this handwritten entry to use
arraysize(kFeatureListForEntry0).

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:165: disabled_entry,
max_entry_id + 1);
Is there a problem with losing this UMA histogram? Has it been used in the
recent past?

Are you going to remove it from the internal histograms.xml?

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:121: if (op == kEQ)
Nit: think this would be better expressed as a switch statement now. Can wait
for a follow-on CL.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:226: bool
GpuControlList::DriverInfo::Contains(const GPUInfo& gpu_info) const {
Nice refactoring here and throughout of the previously monolithic
GpuControlList::GpuControlListEntry::Contains.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:329: // If gpu catemory is not specified, default
to the active gpu.
typo: catemory -> category

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:439: gpu_info.gl_version.empty()) {
It's a little hard to look at this updated condition and convince myself that it
does the same thing as the last one.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:513: processed_os_version =
processed_os_version.substr(0, pos);
Slightly tricky to recognize that this came from OsInfo::Contains. Nothing to be
done, but maybe a comment that this is done for efficiency?

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:199: const FeatureMap& feature_map) const;
I found the continued use of base::ListValue confusing because I thought it was
associated with the JSON parsing, but realize now that it's probably used for
about:gpu. Is that correct?

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:233: void GetReasons(base::ListValue*
problem_list, const std::string& tag) const;
Had the same question about the continued use of base::ListValue here.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_entry_unittest.cc (left):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_entry_unittest.cc:1330:
TEST_F(GpuControlListEntryTest, LinuxKernelVersion) {
Any loss in coverage due to removal of this test? I do see that there are more
thorough tests of OS version matching now.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_entry_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_entry_unittest.cc:21: constexpr auto kOsAny =
GpuControlList::kOsAny;
I thought there was another way of doing this like "using
GpuControlList::kOsLinux". If not, no problem.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h:9: // DO NOT
EDIT!
On 2017/03/30 23:44:41, piman wrote:
> Do you think it's really worth checking in the generated files, as opposed to
> generating them as part of the build?
> 
> We do it for the command buffer generator, mostly because the generated code
is
> security-sensitive, and changes to the generator itself are virtually
impossible
> to review for security issues (because the code snippets are all over the
> place), so we want to review the generated code itself, instead.
> 
> In this case, I think the generated code is pretty meaningless, so I don't see
> value in checking it in. Not doing so also means you don't have to worry about
> clang-format etc.
> Thoughts?

I think it's worth checking in the generated sources for the first couple of
iterations on the script which generates them so we can more easily see the
changes it causes. There are a couple of improvements we know we want to try,
we'll want to see the results, and it's painful to get generated build files off
the bots.

After that I agree we should generate them at build time and delete them from
the workspace.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_version_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_version_unittest.cc:14: constexpr auto kNumerical =
GpuControlList::kVersionStyleNumerical;
Same thing about trying the "using" directive.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
File gpu/config/process_json.py (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
gpu/config/process_json.py:120: def write_string_list(entry_id, is_exception,
exception_id, name_tag, data,
nit: this doesn't seem to be called with anything except is_exception=False,
exception_id=-1.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
gpu/config/process_json.py:589: os.chdir('../../' + path) # assume python script
is under gou/config
typo here and throughout: gou -> gpu

Also, I was going to point out that __file__ is the way to generally find the
location of the script but looking below I see you already know that. I would
probably have tried to avoid the chdir calls and just concatenated file paths,
but it's fine as is.

[Zhenyao Mo, 2017-03-31 19:13:00]

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

[commit-bot: I haz the power, 2017-03-31 19:13:23]

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

[Zhenyao Mo, 2017-03-31 19:14:15]

All comments addressed.

I tried to do a pure rebase and then make changes on top of that, but
unfortunately that doesn't quite work and review is still hard

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.cc:31: 1,                      //
features size
On 2017/03/31 02:37:06, Ken Russell wrote:
> Could consider revising this handwritten entry to use
> arraysize(kFeatureListForEntry0).

Done.

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (left):

https://codereview.chromium.org/2756793003/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:165: disabled_entry,
max_entry_id + 1);
On 2017/03/31 02:37:06, Ken Russell wrote:
> Is there a problem with losing this UMA histogram? Has it been used in the
> recent past?

No, it hasn't been used for a long long time.

> 
> Are you going to remove it from the internal histograms.xml?

Yes, will do.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
File gpu/config/gpu_blacklist.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
gpu/config/gpu_blacklist.h:17: static GpuBlacklist* Create();
On 2017/03/30 23:44:41, piman wrote:
> nit: if it's not too much work, could we return a
std::unique_ptr<GpuBlacklist>
> here? To make ownership clear.

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_blackli...
gpu/config/gpu_blacklist.h:21: GpuBlacklist(const GpuControlListData& data);
On 2017/03/30 23:44:41, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:121: if (op == kEQ)
On 2017/03/31 02:37:07, Ken Russell wrote:
> Nit: think this would be better expressed as a switch statement now. Can wait
> for a follow-on CL.

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:226: bool
GpuControlList::DriverInfo::Contains(const GPUInfo& gpu_info) const {
On 2017/03/31 02:37:07, Ken Russell wrote:
> Nice refactoring here and throughout of the previously monolithic
> GpuControlList::GpuControlListEntry::Contains.

Acknowledged.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:329: // If gpu catemory is not specified, default
to the active gpu.
On 2017/03/31 02:37:06, Ken Russell wrote:
> typo: catemory -> category

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:439: gpu_info.gl_version.empty()) {
On 2017/03/31 02:37:06, Ken Russell wrote:
> It's a little hard to look at this updated condition and convince myself that
it
> does the same thing as the last one.

I think the confusion comes from we have More::gl_version and
GLStrings::gl_version. The former is the processed version, the latter is the
raw GL string.

I'll look into consolidate there two pieces of information into one.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:513: processed_os_version =
processed_os_version.substr(0, pos);
On 2017/03/31 02:37:06, Ken Russell wrote:
> Slightly tricky to recognize that this came from OsInfo::Contains. Nothing to
be
> done, but maybe a comment that this is done for efficiency?

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.cc:587: // TODO(XXX): Remove this
On 2017/03/30 23:44:41, piman wrote:
> nit: can we? :)
> Otherwise, can you add your username here instead of XXX?

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:199: const FeatureMap& feature_map) const;
On 2017/03/31 02:37:06, Ken Russell wrote:
> I found the continued use of base::ListValue confusing because I thought it
was
> associated with the JSON parsing, but realize now that it's probably used for
> about:gpu. Is that correct?

Yes

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:207: GpuControlList(const GpuControlListData&
data);
On 2017/03/30 23:44:41, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list.h:233: void GetReasons(base::ListValue*
problem_list, const std::string& tag) const;
On 2017/03/31 02:37:06, Ken Russell wrote:
> Had the same question about the continued use of base::ListValue here.

Yes, it's for about:gpu

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_entry_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_entry_unittest.cc:21: constexpr auto kOsAny =
GpuControlList::kOsAny;
On 2017/03/31 02:37:06, Ken Russell wrote:
> I thought there was another way of doing this like "using
> GpuControlList::kOsLinux". If not, no problem.

No that doesn't compile.  using constexpr auto is what the compiler suggested.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h:9: // DO NOT
EDIT!
On 2017/03/30 23:44:41, piman wrote:
> Do you think it's really worth checking in the generated files, as opposed to
> generating them as part of the build?
> 
> We do it for the command buffer generator, mostly because the generated code
is
> security-sensitive, and changes to the generator itself are virtually
impossible
> to review for security issues (because the code snippets are all over the
> place), so we want to review the generated code itself, instead.
> 
> In this case, I think the generated code is pretty meaningless, so I don't see
> value in checking it in. Not doing so also means you don't have to worry about
> clang-format etc.
> Thoughts?

Acknowledged.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_testing_arrays_and_structs_autogen.h:9: // DO NOT
EDIT!
On 2017/03/31 02:37:06, Ken Russell wrote:
> On 2017/03/30 23:44:41, piman wrote:
> > Do you think it's really worth checking in the generated files, as opposed
to
> > generating them as part of the build?
> > 
> > We do it for the command buffer generator, mostly because the generated code
> is
> > security-sensitive, and changes to the generator itself are virtually
> impossible
> > to review for security issues (because the code snippets are all over the
> > place), so we want to review the generated code itself, instead.
> > 
> > In this case, I think the generated code is pretty meaningless, so I don't
see
> > value in checking it in. Not doing so also means you don't have to worry
about
> > clang-format etc.
> > Thoughts?
> 
> I think it's worth checking in the generated sources for the first couple of
> iterations on the script which generates them so we can more easily see the
> changes it causes. There are a couple of improvements we know we want to try,
> we'll want to see the results, and it's painful to get generated build files
off
> the bots.
> 
> After that I agree we should generate them at build time and delete them from
> the workspace.

Acknowledged.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_unittest.cc:38: GpuControlList* Create(size_t
entry_count, const Entry* entries) {
On 2017/03/30 23:44:41, piman wrote:
> nit: return std::unique_ptr<GpuControlList>

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_version_unittest.cc (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_version_unittest.cc:14: constexpr auto kNumerical =
GpuControlList::kVersionStyleNumerical;
On 2017/03/31 02:37:06, Ken Russell wrote:
> Same thing about trying the "using" directive.

No that doesn't compile.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
File gpu/config/gpu_driver_bug_list.h (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_list.h:23: static GpuDriverBugList* Create();
On 2017/03/30 23:44:41, piman wrote:
> nit: ditto re: std::unique_ptr

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_driver_...
gpu/config/gpu_driver_bug_list.h:38: GpuDriverBugList(const GpuControlListData&
data);
On 2017/03/30 23:44:41, piman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
File gpu/config/process_json.py (right):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
gpu/config/process_json.py:120: def write_string_list(entry_id, is_exception,
exception_id, name_tag, data,
On 2017/03/31 02:37:06, Ken Russell wrote:
> nit: this doesn't seem to be called with anything except is_exception=False,
> exception_id=-1.

Done.

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/process_jso...
gpu/config/process_json.py:589: os.chdir('../../' + path) # assume python script
is under gou/config
On 2017/03/31 02:37:06, Ken Russell wrote:
> typo here and throughout: gou -> gpu

Done

> 
> Also, I was going to point out that __file__ is the way to generally find the
> location of the script but looking below I see you already know that. I would
> probably have tried to avoid the chdir calls and just concatenated file paths,
> but it's fine as is.

Ackowledged

[commit-bot: I haz the power, 2017-03-31 20:39:15]

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

[commit-bot: I haz the power, 2017-03-31 20:39:16]

Dry run: 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...)

[Zhenyao Mo, 2017-03-31 21:17:00]

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
File gpu/config/gpu_control_list_entry_unittest.cc (left):

https://codereview.chromium.org/2756793003/diff/200001/gpu/config/gpu_control...
gpu/config/gpu_control_list_entry_unittest.cc:1330:
TEST_F(GpuControlListEntryTest, LinuxKernelVersion) {
On 2017/03/31 02:37:06, Ken Russell wrote:
> Any loss in coverage due to removal of this test? I do see that there are more
> thorough tests of OS version matching now.

Thanks for catching this.  This can no longer be tested at Entry level because
the target OS version if processed before passing to an Entry test. I planned to
move it to GpuControlList unittests, but forgot.  I just added it there, so no
coverage loss.

[Zhenyao Mo, 2017-03-31 22:37:33]

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

[commit-bot: I haz the power, 2017-03-31 22:38: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, 2017-03-31 23:54:10]

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

[commit-bot: I haz the power, 2017-03-31 23:54:12]

Dry run: This issue passed the CQ dry run.

[Zhenyao Mo, 2017-03-31 23:55:52]

Description was changed from

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Zhenyao Mo, 2017-03-31 23:57:15]

piman: landing with TBR to you since your suggestions are nits and I addressed
all of them

[Zhenyao Mo, 2017-03-31 23:57:28]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2017-03-31 23:57:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2756793003/#ps230001
(title: "pure rebase")

[commit-bot: I haz the power, 2017-03-31 23:58:00]

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-04-01 00:12:12]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1491004648039510,
"parent_rev": "1f2f12faf6b42983525f917bd5fabc2f02bdf8a3", "commit_rev":
"2602bcf4e08d2cb554520b42246f68ac22426bc8"}

[commit-bot: I haz the power, 2017-04-01 00:13:26]

Description was changed from

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Move GPU blacklist and driver bug workaround list from json to data struct.

BUG=691703
TEST=gpu_unittests
R=kbr@chromium.org,brettw@chromium.org,piman@chromium.org
TBR=piman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2756793003
Cr-Commit-Position: refs/heads/master@{#461282}
Committed:
https://chromium.googlesource.com/chromium/src/+/2602bcf4e08d2cb554520b42246f...
==========

[commit-bot: I haz the power, 2017-04-01 00:13:28]

Committed patchset #21 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/2602bcf4e08d2cb554520b42246f...

[agrieve, 2017-04-03 14:41:20]

On 2017/04/01 00:13:28, commit-bot: I haz the power wrote:
> Committed patchset #21 (id:230001) as
>
https://chromium.googlesource.com/chromium/src/+/2602bcf4e08d2cb554520b42246f...

This reduced ChromePublic.apk by 20kb. Woo!