harmony: partly harmonize permissions bubble

harmony: partly harmonize permissions bubble

This change:
1) Changes the title string to not include the trailing colon (this affects the
   non-Harmony version too)
2) Switches from 9px to 8px for the spacing between items (affects non-Harmony)
3) Makes the indent on permission entries conditional on non-Harmony

BUG=605667
Review-Url: https://codereview.chromium.org/2654123004
Cr-Commit-Position: refs/heads/master@{#447888}
Committed: https://chromium.googlesource.com/chromium/src/+/2e942b323369f872f50f42cf5623f3265470861d

[Elly Fong-Jones, 2017-01-26 19:01:14]

Description was changed from

==========
harmony: partly harmonize permissions bubble

This change:
1) Changes the title string to not include the trailing colon (this affects the
   non-Harmony version too)
2) Switches from 9px to 8px for the spacing between items (affects non-Harmony)
3) Makes the indent on permission entries conditional on non-Harmony

BUG=605667
==========

to

==========
harmony: partly harmonize permissions bubble

This change:
1) Changes the title string to not include the trailing colon (this affects the
   non-Harmony version too)
2) Switches from 9px to 8px for the spacing between items (affects non-Harmony)
3) Makes the indent on permission entries conditional on non-Harmony

BUG=605667
==========

[Elly Fong-Jones, 2017-01-26 19:01:14]

ellyjones@chromium.org changed reviewers:
+ palmer@chromium.org, pkasting@chromium.org

[Elly Fong-Jones, 2017-01-26 19:01:34]

palmer: ptal as c/b/ui/v/website_settings OWNER?
pkasting: ptal as c/b/ui/v OWNER?

[Peter Kasting, 2017-01-26 21:54:29]

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:41: return
kHarmonyLayoutUnit / 2;
Shouldn't this be 16 px and not 8?

I'm looking at
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
, maybe I'm looking at the wrong spec or misunderstanding how this is used.

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:231:
RELATED_CONTROL_HORIZONTAL_SPACING)
This changes the indent for non-Harmony from 10 px to 8.  More critically, it
doesn't seem like RELATED_CONTROL_HORIZONTAL_SPACING, which is meant to be the
horizontal distance between the end of one control and the start of a second
control on the same line, is the right semantic value for "indentation for a
section of controls related to some block above it".  kCheckboxIndent was the
only "indenting"-related constant in layout_constants.h AFAIK; maybe the right
solution here is to add a new LayoutDistanceType enum value for this that's
kCheckboxIndent in pre-Harmony and 0 in Harmony, and then not bother to call
UseExtraDialogPadding here.

If we do this, I don't know that the new enum should be called CHECKBOX_INDENT
-- it would be nice to be more generic with the name in some way.

[Elly Fong-Jones, 2017-01-30 19:47:34]

pkasting: ptal? :)

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:41: return
kHarmonyLayoutUnit / 2;
On 2017/01/26 21:54:29, Peter Kasting wrote:
> Shouldn't this be 16 px and not 8?
> 
> I'm looking at
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
> , maybe I'm looking at the wrong spec or misunderstanding how this is used.

Oh, yes, you are correct - this ought to have been 16px.

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/web...
File chrome/browser/ui/views/website_settings/permission_prompt_impl.cc (right):

https://codereview.chromium.org/2654123004/diff/1/chrome/browser/ui/views/web...
chrome/browser/ui/views/website_settings/permission_prompt_impl.cc:231:
RELATED_CONTROL_HORIZONTAL_SPACING)
On 2017/01/26 21:54:29, Peter Kasting wrote:
> This changes the indent for non-Harmony from 10 px to 8.  More critically, it
> doesn't seem like RELATED_CONTROL_HORIZONTAL_SPACING, which is meant to be the
> horizontal distance between the end of one control and the start of a second
> control on the same line, is the right semantic value for "indentation for a
> section of controls related to some block above it".  kCheckboxIndent was the
> only "indenting"-related constant in layout_constants.h AFAIK; maybe the right
> solution here is to add a new LayoutDistanceType enum value for this that's
> kCheckboxIndent in pre-Harmony and 0 in Harmony, and then not bother to call
> UseExtraDialogPadding here.
> 
> If we do this, I don't know that the new enum should be called CHECKBOX_INDENT
> -- it would be nice to be more generic with the name in some way.

CHECKBOX_INDENT already was 0 in Harmony and kCheckboxIndent pre-Harmony, so
I've renamed it to SUBSECTION_INDENT and made this code use it instead of
RELATED_CONTROL_HORIZONTAL_SPACING.

[Peter Kasting, 2017-02-01 02:20:20]

LGTM

https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:35: SUBSECTION_INDENT,
I liked your use of SUBSECTION here, so I adopted it in
https://codereview.chromium.org/2666043002/ .  I added HORIZONTAL_ there as
well.  Once that change lands (in CQ as I write this), you'll want to revert
this piece of this change, and update the rest of your patch to use that name.

https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.h:37: ITEM_LABEL_SPACING,
I also found a need for this constant locally, so adding this seems good.  In my
checkout (not yet up for review), I picked this name and comment, for
parallelism with the other names above (and I put this in alphabetical order
with them).  WDYT?

    // Horizontal spacing between an item such as an icon or checkbox and a
    // label related to it.
    RELATED_LABEL_HORIZONTAL_SPACING,

[Elly Fong-Jones, 2017-02-02 19:27:00]

On 2017/02/01 02:20:20, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/harmony/layout_delegate.h (right):
> 
>
https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/harmony/layout_delegate.h:35: SUBSECTION_INDENT,
> I liked your use of SUBSECTION here, so I adopted it in
> https://codereview.chromium.org/2666043002/ .  I added HORIZONTAL_ there as
> well.  Once that change lands (in CQ as I write this), you'll want to revert
> this piece of this change, and update the rest of your patch to use that name.

Done.

>
https://codereview.chromium.org/2654123004/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/harmony/layout_delegate.h:37: ITEM_LABEL_SPACING,
> I also found a need for this constant locally, so adding this seems good.  In
my
> checkout (not yet up for review), I picked this name and comment, for
> parallelism with the other names above (and I put this in alphabetical order
> with them).  WDYT?
> 
>     // Horizontal spacing between an item such as an icon or checkbox and a
>     // label related to it.
>     RELATED_LABEL_HORIZONTAL_SPACING,

Done.

[Elly Fong-Jones, 2017-02-02 19:27:11]

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

[Elly Fong-Jones, 2017-02-02 19:27:41]

ellyjones@chromium.org changed reviewers:
- sky@chromium.org

[Elly Fong-Jones, 2017-02-02 19:27:52]

palmer: ping?

[palmer, 2017-02-02 19:33:02]

lgtm

[Elly Fong-Jones, 2017-02-02 23:07:50]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-02-02 23:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2654123004/#ps60001
(title: "fix constant names")

[commit-bot: I haz the power, 2017-02-02 23:08:52]

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-03 00:00:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486076870955810,
"parent_rev": "6d7564a2dce2d90226bfd84eb45c72daf1da2f7d", "commit_rev":
"2e942b323369f872f50f42cf5623f3265470861d"}

[commit-bot: I haz the power, 2017-02-03 00:00:54]

Description was changed from

==========
harmony: partly harmonize permissions bubble

This change:
1) Changes the title string to not include the trailing colon (this affects the
   non-Harmony version too)
2) Switches from 9px to 8px for the spacing between items (affects non-Harmony)
3) Makes the indent on permission entries conditional on non-Harmony

BUG=605667
==========

to

==========
harmony: partly harmonize permissions bubble

This change:
1) Changes the title string to not include the trailing colon (this affects the
   non-Harmony version too)
2) Switches from 9px to 8px for the spacing between items (affects non-Harmony)
3) Makes the indent on permission entries conditional on non-Harmony

BUG=605667

Review-Url: https://codereview.chromium.org/2654123004
Cr-Commit-Position: refs/heads/master@{#447888}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e942b323369f872f50f42cf5623...
==========

[commit-bot: I haz the power, 2017-02-03 00:00:55]

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