Handle malformed built-in PluginFinder data.

Handle malformed built-in PluginFinder data.

BUG=388560
Committed: https://crrev.com/107206c920359aba30e08970466d21f52b333dac
Cr-Commit-Position: refs/heads/master@{#327247}

[Bernhard Bauer, 2015-04-20 17:05:38]

bauerb@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Bernhard Bauer, 2015-04-20 17:05:38]

Please review!

[Alexei Svitkine (slow), 2015-04-20 17:15:11]

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:137:
base::JSONReader::JSON_PARSE_ERROR_COUNT + 1);
You don't need +1 here.

Also, please update that enum definition with a comment mentioning it's used in
UMA and entries shouldn't be deleted or renumbered.

https://codereview.chromium.org/1093153003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1093153003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:28468: +  <summary>Error codes when
parsing the built-in plugin list.</summary>
Mention when it's logged.

[Bernhard Bauer, 2015-04-21 16:34:11]

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:137:
base::JSONReader::JSON_PARSE_ERROR_COUNT + 1);
On 2015/04/20 17:15:10, Alexei Svitkine wrote:
> You don't need +1 here.

I'm (ab)using JSONReader::JSON_PARSE_ERROR_COUNT as a value (see comment above),
so the boundary value is the next one.

> Also, please update that enum definition with a comment mentioning it's used
in
> UMA and entries shouldn't be deleted or renumbered.

Done.

https://codereview.chromium.org/1093153003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1093153003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:28468: +  <summary>Error codes when
parsing the built-in plugin list.</summary>
On 2015/04/20 17:15:10, Alexei Svitkine wrote:
> Mention when it's logged.

Done.

[Alexei Svitkine (slow), 2015-04-21 16:50:36]

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
File chrome/browser/plugins/plugin_finder.cc (right):

https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
chrome/browser/plugins/plugin_finder.cc:137:
base::JSONReader::JSON_PARSE_ERROR_COUNT + 1);
On 2015/04/21 16:34:11, Bernhard Bauer wrote:
> On 2015/04/20 17:15:10, Alexei Svitkine wrote:
> > You don't need +1 here.
> 
> I'm (ab)using JSONReader::JSON_PARSE_ERROR_COUNT as a value (see comment
above),
> so the boundary value is the next one.
> 
> > Also, please update that enum definition with a comment mentioning it's used
> in
> > UMA and entries shouldn't be deleted or renumbered.
> 
> Done.

I see. I don't think we should do this. Otherwise, if someone ever adds a new
entry to the enum, it will renumber things. Why not just add it to the enum?

[Bernhard Bauer, 2015-04-22 21:20:53]

On 2015/04/21 16:50:36, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
> File chrome/browser/plugins/plugin_finder.cc (right):
> 
>
https://codereview.chromium.org/1093153003/diff/1/chrome/browser/plugins/plug...
> chrome/browser/plugins/plugin_finder.cc:137:
> base::JSONReader::JSON_PARSE_ERROR_COUNT + 1);
> On 2015/04/21 16:34:11, Bernhard Bauer wrote:
> > On 2015/04/20 17:15:10, Alexei Svitkine wrote:
> > > You don't need +1 here.
> > 
> > I'm (ab)using JSONReader::JSON_PARSE_ERROR_COUNT as a value (see comment
> above),
> > so the boundary value is the next one.
> > 
> > > Also, please update that enum definition with a comment mentioning it's
used
> > in
> > > UMA and entries shouldn't be deleted or renumbered.
> > 
> > Done.
> 
> I see. I don't think we should do this. Otherwise, if someone ever adds a new
> entry to the enum, it will renumber things. Why not just add it to the enum?

That feels a bit strange, because it's not an error that would ever be returned
by JsonReader.

What I ended up doing is defining a separate enum that just happens to be a
superset of the JsonReader error codes. That way we can demand that *those*
values never change, without affecting JsonReader, and if new values are added
there, the compiler will complain.

[Alexei Svitkine (slow), 2015-04-25 00:17:18]

lgtm

[Bernhard Bauer, 2015-04-27 08:33:03]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-04-27 08:33:10]

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

[commit-bot: I haz the power, 2015-04-27 09:11:22]

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

[commit-bot: I haz the power, 2015-04-27 09:11:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Bernhard Bauer, 2015-04-28 07:25:01]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-04-28 07:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1093153003/#ps60001
(title: "fix and rebase")

[commit-bot: I haz the power, 2015-04-28 07:25:12]

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

[commit-bot: I haz the power, 2015-04-28 08:32:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-28 08:33:24]

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