[Extensions] Deprecate old install prompt types

[Extensions] Deprecate old install prompt types

Remove old install prompt types from the code. Because the enum is used
in histograms, this isn't removed completely, but we can comment them
out to make sure they aren't used.

Also move away from using the "array of messages" approach, because this
required padding with 0's for deprecated types, and was significantly
harder to read.

Finally, remove a few deprecated messages that were found by doing this.

BUG=689227
Review-Url: https://codereview.chromium.org/2705733002
Cr-Commit-Position: refs/heads/master@{#452038}
Committed: https://chromium.googlesource.com/chromium/src/+/c7f8d139f448266d41723a8fba0347a246111c03

[Devlin, 2017-02-17 23:25:06]

Description was changed from

==========
[Extensions] Deprecate old install prompt types

Remove old install prompt types from the code. Because the enum is used
in histograms, this isn't removed completely, but we can comment them
out to make sure they aren't used.

Also move away from using the "array of messages" approach, because this
required padding with 0's for deprecated types, and was significantly
harder to read.

Finally, remove a few deprecated messages that were found by doing this.

BUG=None
==========

to

==========
[Extensions] Deprecate old install prompt types

Remove old install prompt types from the code. Because the enum is used
in histograms, this isn't removed completely, but we can comment them
out to make sure they aren't used.

Also move away from using the "array of messages" approach, because this
required padding with 0's for deprecated types, and was significantly
harder to read.

Finally, remove a few deprecated messages that were found by doing this.

BUG=689227
==========

[Devlin, 2017-02-17 23:25:10]

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

[commit-bot: I haz the power, 2017-02-17 23:26:12]

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

[Devlin, 2017-02-17 23:37:00]

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

[commit-bot: I haz the power, 2017-02-17 23:37: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-02-18 01:42:19]

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

[commit-bot: I haz the power, 2017-02-18 01:42:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Devlin, 2017-02-18 02:02:27]

The CQ bit was checked by rdevlin.cronin@chromium.org

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

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

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

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

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

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Devlin, 2017-02-21 17:08:25]

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

[Devlin, 2017-02-21 17:08:25]

lazyboy@, mind taking a look?

I personally find it a lot easier to grok which prompts show which titles
without the map lookups (especially since half of them required further logic
anyway), but lemme know if you feel differently.

[lazyboy, 2017-02-21 21:43:05]

Agreed this is more readable, few comments.

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:140: NOTREACHED();
Can types_ be ever set to these upon constructing ExtensionInstallPrompt?
If not, maybe we can DCHECK these in the constructor and make type_ const. I
couldn't find code that calls ExtensionInstallPrompt::set_type()...

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:281: buttons =
ui::DIALOG_BUTTON_CANCEL;
Since there's only one special case, I think even the following would be simpler
to read:

DCHECK(type_ != UNSET_PROMPT_TYPE && type_ != NUM_PROMPT_TYPES)
if (type_ == POST_INSTALL_PERMISSIONS_PROMPT)
  if (ShouldDisplayRevokeButton()) return CANCEL|OK;
  else return CANCEL;

return OK|CANCEL;

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:307: case
RE_ENABLE_PROMPT:
RE_ENABLE_PROMPT used to return
IDS_EXTENSION_PROMPT_RE_ENABLE_BUTTON (kAccpetButtonIds[3])

PERMISSIONS_PROMPT used to return
IDS_EXTENSION_PROMPT_PERMISSIONS_BUTTON (kAcceptButtonIds[4])

Is this change intentional?

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:326: // If there are
neither retained files nor devices, leave id 0 so there
"leave id to -1"

[Devlin, 2017-02-22 00:08:40]

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:140: NOTREACHED();
On 2017/02/21 21:43:05, lazyboy wrote:
> Can types_ be ever set to these upon constructing ExtensionInstallPrompt?
> If not, maybe we can DCHECK these in the constructor and make type_ const. I
> couldn't find code that calls ExtensionInstallPrompt::set_type()...

Done.

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:281: buttons =
ui::DIALOG_BUTTON_CANCEL;
On 2017/02/21 21:43:05, lazyboy wrote:
> Since there's only one special case, I think even the following would be
simpler
> to read:
> 
> DCHECK(type_ != UNSET_PROMPT_TYPE && type_ != NUM_PROMPT_TYPES)
> if (type_ == POST_INSTALL_PERMISSIONS_PROMPT)
>   if (ShouldDisplayRevokeButton()) return CANCEL|OK;
>   else return CANCEL;
> 
> return OK|CANCEL;

Done.

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:307: case
RE_ENABLE_PROMPT:
On 2017/02/21 21:43:05, lazyboy wrote:
> RE_ENABLE_PROMPT used to return
> IDS_EXTENSION_PROMPT_RE_ENABLE_BUTTON (kAccpetButtonIds[3])
> 
> PERMISSIONS_PROMPT used to return
> IDS_EXTENSION_PROMPT_PERMISSIONS_BUTTON (kAcceptButtonIds[4])
> 
> Is this change intentional?

Good catch!  Changed.

https://codereview.chromium.org/2705733002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_install_prompt.cc:326: // If there are
neither retained files nor devices, leave id 0 so there
On 2017/02/21 21:43:05, lazyboy wrote:
> "leave id to -1"

Done.

[Devlin, 2017-02-22 00:08:45]

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

[commit-bot: I haz the power, 2017-02-22 00:10:10]

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

[lazyboy, 2017-02-22 00:16:38]

lgtm

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

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Devlin, 2017-02-22 02:39:09]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-02-22 02:40:01]

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

[commit-bot: I haz the power, 2017-02-22 04:42:31]

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

[commit-bot: I haz the power, 2017-02-22 04:42:32]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Devlin, 2017-02-22 05:24:50]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:25:53]

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

[commit-bot: I haz the power, 2017-02-22 07:26:18]

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

[commit-bot: I haz the power, 2017-02-22 07:26:19]

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

[Devlin, 2017-02-22 13:56:46]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-02-22 13:57:11]

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

[commit-bot: I haz the power, 2017-02-22 14:29:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487771806624140,
"parent_rev": "319c00e1c8188a7264d674e6bceeb35cb7e6dc63", "commit_rev":
"c7f8d139f448266d41723a8fba0347a246111c03"}

[commit-bot: I haz the power, 2017-02-22 14:29:43]

Description was changed from

==========
[Extensions] Deprecate old install prompt types

Remove old install prompt types from the code. Because the enum is used
in histograms, this isn't removed completely, but we can comment them
out to make sure they aren't used.

Also move away from using the "array of messages" approach, because this
required padding with 0's for deprecated types, and was significantly
harder to read.

Finally, remove a few deprecated messages that were found by doing this.

BUG=689227
==========

to

==========
[Extensions] Deprecate old install prompt types

Remove old install prompt types from the code. Because the enum is used
in histograms, this isn't removed completely, but we can comment them
out to make sure they aren't used.

Also move away from using the "array of messages" approach, because this
required padding with 0's for deprecated types, and was significantly
harder to read.

Finally, remove a few deprecated messages that were found by doing this.

BUG=689227

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

[commit-bot: I haz the power, 2017-02-22 14:29:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c7f8d139f448266d41723a8fba03...