Views/Permissions: Use shorter strings for site setting drop downs.

Views/Permissions: Use shorter strings for site setting drop downs.

Drop-downs in the OIB typically show three options for each site setting - ask,
allow, and block. For Harmony, shorter strings should be used in these drop
downs. r453710 partly implemented the use of short strings for settings that
were set to the default, but it also introduced a bug where it would show the
incorrect string in place of the default option when there was a non-default
option selected. (For example, when set to 'Allow', the default option that
usually says 'Ask' would also say 'Allow', resulting in two 'Allow' options -
one using the long string and one using the short.)

Fix this bug and also start using short strings for all other site setting
options.

BUG=657292
Review-Url: https://codereview.chromium.org/2732963005
Cr-Commit-Position: refs/heads/master@{#455580}
Committed: https://chromium.googlesource.com/chromium/src/+/4b0f801fa697ed48e8c2b7f1c0cd1407cacdd783

[Patti Lor, 2017-03-07 05:04:38]

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

[commit-bot: I haz the power, 2017-03-07 05:05:17]

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

[Patti Lor, 2017-03-07 05:35:59]

patricialor@chromium.org changed reviewers:
+ raymes@chromium.org

[Patti Lor, 2017-03-07 05:35:59]

Hi Raymes, PTAL - I ended up just adding extra comments as opposed to doing any
refactoring.

[Patti Lor, 2017-03-07 06:27:15]

patricialor@chromium.org changed reviewers:
+ ellyjones@chromium.org

[Patti Lor, 2017-03-07 06:27:15]

+ellyjones@ - you probably have more context into this - PTAL?

Thanks!

[Patti Lor, 2017-03-07 06:33:48]

Description was changed from

==========
Views/Permissions: Use shorter strings for site setting drop downs.

Drop-downs in the OIB typically show three options for each site setting - ask,
allow, and block. For Harmony, shorter strings should be used in these drop
downs. r453710 partly implemented the use of short strings for settings that
were set to the default, but it also introduced a bug where it would show the
incorrect string in place of the default option there was a non-default option
selected. (For example, when set to 'Allow', the default option that usually
says 'Ask' would also say 'Allow', resulting in two 'Allow' options - one using
the long string and one using the short.)

Fix this bug and also start using short strings for all other site setting
options.

BUG=657292
==========

to

==========
Views/Permissions: Use shorter strings for site setting drop downs.

Drop-downs in the OIB typically show three options for each site setting - ask,
allow, and block. For Harmony, shorter strings should be used in these drop
downs. r453710 partly implemented the use of short strings for settings that
were set to the default, but it also introduced a bug where it would show the
incorrect string in place of the default option when there was a non-default
option selected. (For example, when set to 'Allow', the default option that
usually says 'Ask' would also say 'Allow', resulting in two 'Allow' options -
one using the long string and one using the short.)

Fix this bug and also start using short strings for all other site setting
options.

BUG=657292
==========

[commit-bot: I haz the power, 2017-03-07 07:26:39]

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

[commit-bot: I haz the power, 2017-03-07 07:26:39]

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

[Elly Fong-Jones, 2017-03-07 16:24:22]

lgtm

[Patti Lor, 2017-03-07 22:57:44]

patricialor@chromium.org changed reviewers:
+ palmer@chromium.org
- raymes@chromium.org

[Patti Lor, 2017-03-07 22:57:45]

Hi palmer@ - PTAL for owner's review of everything.

raymes -> CC as discussed offline :)

[palmer, 2017-03-07 23:39:46]

lgtm

https://codereview.chromium.org/2732963005/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2732963005/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_menu_model.cc:109: // Retrieve the
string to show for blocking the permission.
Nit: These comments suggest that moving this code into well-named helper
functions might clarify the intent of the code. E.g.

  label = GetBlockingPermissionLabel(profile, info.type, CONTENT_SETTING_BLOCK,
                                     effective_default_setting, info.source);

or such.

I could be wrong; your call. :)

[Patti Lor, 2017-03-08 06:52:20]

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

[commit-bot: I haz the power, 2017-03-08 06:52:44]

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

[Patti Lor, 2017-03-08 06:58:41]

Thanks both for the reviews - I did update all the references to the
PermissionInfo to use the member variable rather than the constructor argument.

https://codereview.chromium.org/2732963005/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/2732963005/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_menu_model.cc:109: // Retrieve the
string to show for blocking the permission.
On 2017/03/07 23:39:46, palmer wrote:
> Nit: These comments suggest that moving this code into well-named helper
> functions might clarify the intent of the code. E.g.
> 
>   label = GetBlockingPermissionLabel(profile, info.type,
CONTENT_SETTING_BLOCK,
>                                      effective_default_setting, info.source);
> 
> or such.
> 
> I could be wrong; your call. :)

Yeah, I did consider this - it's pretty simple for the "blocked" permission
type, but looking at "allowed" and "default" it's a lot more unclear - "allowed"
permissions aren't added to the menu at all based on certain conditions, and
default permissions are a lot more complicated since you also need access to the
HostContentSettingsMapFactory etc, resulting in them all having pretty different
function signatures (and adding them as methods is probably also bad)... so I
decided just to leave it after adding the comments.

Thanks for the suggestion though - just giving some context into why I decided
not to do that in case you were interested -- let me know if you still think
it's worth doing.

[commit-bot: I haz the power, 2017-03-08 07:34:14]

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

[commit-bot: I haz the power, 2017-03-08 07:34:15]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-03-08 23:00:02]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-03-08 23:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
ellyjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2732963005/#ps20001
(title: "Change all references to |info| in the constructor to references to
|permission_| to be consistent.")

[commit-bot: I haz the power, 2017-03-08 23:00: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, 2017-03-08 23:08:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489014002221700,
"parent_rev": "caf5f19356f9c6ea8a966d12ee152401aee91585", "commit_rev":
"4b0f801fa697ed48e8c2b7f1c0cd1407cacdd783"}

[commit-bot: I haz the power, 2017-03-08 23:09:21]

Description was changed from

==========
Views/Permissions: Use shorter strings for site setting drop downs.

Drop-downs in the OIB typically show three options for each site setting - ask,
allow, and block. For Harmony, shorter strings should be used in these drop
downs. r453710 partly implemented the use of short strings for settings that
were set to the default, but it also introduced a bug where it would show the
incorrect string in place of the default option when there was a non-default
option selected. (For example, when set to 'Allow', the default option that
usually says 'Ask' would also say 'Allow', resulting in two 'Allow' options -
one using the long string and one using the short.)

Fix this bug and also start using short strings for all other site setting
options.

BUG=657292
==========

to

==========
Views/Permissions: Use shorter strings for site setting drop downs.

Drop-downs in the OIB typically show three options for each site setting - ask,
allow, and block. For Harmony, shorter strings should be used in these drop
downs. r453710 partly implemented the use of short strings for settings that
were set to the default, but it also introduced a bug where it would show the
incorrect string in place of the default option when there was a non-default
option selected. (For example, when set to 'Allow', the default option that
usually says 'Ask' would also say 'Allow', resulting in two 'Allow' options -
one using the long string and one using the short.)

Fix this bug and also start using short strings for all other site setting
options.

BUG=657292

Review-Url: https://codereview.chromium.org/2732963005
Cr-Commit-Position: refs/heads/master@{#455580}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b0f801fa697ed48e8c2b7f1c0cd...
==========

[commit-bot: I haz the power, 2017-03-08 23:09:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4b0f801fa697ed48e8c2b7f1c0cd...