Convert Register Protocol Handler dialog for Harmony/Material Design

Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The other issue regarding the wrapping of text should be a separate issue and confined to the implementing views/controls.

BUG=652024
Review-Url: https://codereview.chromium.org/2571613002
Cr-Commit-Position: refs/heads/master@{#442683}
Committed: https://chromium.googlesource.com/chromium/src/+/44ec2a4fd5e1c052077575f84b5b5cbdb703cef6

[kylix_rd, 2016-12-12 21:31:53]

Description was changed from

==========
Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The
other issue regarding the wrapping of text should be a separate issue and
confined to the implementing views/controls.

BUG=652024
==========

to

==========
Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The
other issue regarding the wrapping of text should be a separate issue and
confined to the implementing views/controls.

BUG=652024
==========

[kylix_rd, 2016-12-12 21:31:53]

kylixrd@chromium.org changed reviewers:
+ pkasting@chromium.org

[kylix_rd, 2016-12-12 21:33:16]

I was unable to locate a specific Material Design spec for the
views::kCheckboxIndent value. If there is one, a reference would be appreciated.
Otherwise, I've left that value alone.

[Peter Kasting, 2016-12-12 22:24:16]

I don't know of a spec for the checkbox indent; I'd ask ainslie@ for any info
there.

LGTM

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:212:
RELATED_CONTROL_HORIZONTAL_SPACING));
Seems unfortunate that we have to write something this verbose so many times.

None of my solution ideas are great (e.g. "add namespace alias locally to reduce
length of qualified enum name; bind a lamba to
layout_delegate->GetLayoutDistance(); store return values for various distances
in locals").  Feel free to use any of these, or none, or others, if you have
ideas on how to improve.

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:332: 0,
views::kRelatedControlHorizontalSpacing);
Do you need to fix this one?

[kylix_rd, 2016-12-13 14:21:23]

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:212:
RELATED_CONTROL_HORIZONTAL_SPACING));
On 2016/12/12 22:24:16, Peter Kasting wrote:
> Seems unfortunate that we have to write something this verbose so many times.
> 
> None of my solution ideas are great (e.g. "add namespace alias locally to
reduce
> length of qualified enum name; bind a lamba to
> layout_delegate->GetLayoutDistance(); store return values for various
distances
> in locals").  Feel free to use any of these, or none, or others, if you have
> ideas on how to improve.

Good point. I was slavishly following the examples set forth in some of the
other already converted code. I should know better.

https://codereview.chromium.org/2571613002/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:332: 0,
views::kRelatedControlHorizontalSpacing);
On 2016/12/12 22:24:16, Peter Kasting wrote:
> Do you need to fix this one?

Doh! And that's why we have code reviews...

[kylix_rd, 2017-01-05 20:33:40]

Patchset #3 (id:40001) has been deleted

[kylix_rd, 2017-01-05 20:35:02]

Patchset #3 (id:60001) has been deleted

[kylix_rd, 2017-01-05 20:43:47]

kylixrd@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[kylix_rd, 2017-01-05 21:46:35]

Patchset #3 (id:80001) has been deleted

[kylix_rd, 2017-01-05 21:48:16]

kylixrd@chromium.org changed reviewers:
- pkasting@chromium.org

[kylix_rd, 2017-01-05 21:51:35]

sky@ - Please look over the changes in ui/views, chrome/browser/ui. Notably, the
ViewsDelegate changes.

msw@ - Please check the changes down in ui/views/bubble

[sky, 2017-01-05 23:26:40]

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:574: return
ViewsDelegate::GetDialogFrameViewInsets();
Is it intentional that you aren't calling GetBubbleDialogMargins?

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:221:
LayoutDelegate::LayoutDistanceType::RELATED_CONTROL_VERTICAL_SPACING);
Isn't this the same as 216 above?

https://codereview.chromium.org/2571613002/diff/100001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/ui/views/views_delegat...
ui/views/views_delegate.cc:139: return kRelatedControlVerticalSpacing;
If there are no more consumers of these constants, can you move them from
layout_constants.cc to here (in anonymous namespace at top of file)? That way
people don't add more uses when they should be using this.

[kylix_rd, 2017-01-06 14:36:50]

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:574: return
ViewsDelegate::GetDialogFrameViewInsets();
On 2017/01/05 23:26:40, sky wrote:
> Is it intentional that you aren't calling GetBubbleDialogMargins?

Oops.

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:221:
LayoutDelegate::LayoutDistanceType::RELATED_CONTROL_VERTICAL_SPACING);
On 2017/01/05 23:26:40, sky wrote:
> Isn't this the same as 216 above?

Wrong constant. Will fix.

https://codereview.chromium.org/2571613002/diff/100001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/100001/ui/views/views_delegat...
ui/views/views_delegate.cc:139: return kRelatedControlVerticalSpacing;
On 2017/01/05 23:26:40, sky wrote:
> If there are no more consumers of these constants, can you move them from
> layout_constants.cc to here (in anonymous namespace at top of file)? That way
> people don't add more uses when they should be using this.

Once most of the Harmony work is done, I suspect there will be far fewer uses of
these constants. Right now, a quick search over the codebase reveals significant
uses.

[msw, 2017-01-06 23:17:43]

It's a little weird that we are relying on chrome to supply views dialog/bubble
insets, and weird that we are using this system instead of a layout_constants
header or NativeTheme, but apparently I'm not up with the latest plans and
patterns here... Are there docs I can read?

https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:569: return
gfx::Insets(HarmonyLayoutDelegate::kHarmonyLayoutUnit,
optional nit: use the ctor that takes one |all| arg.

https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
ui/views/views_delegate.cc:148: return gfx::Insets(kPanelVertMargin,
optional nit: use the ctor that takes |vertical|, |horizontal| args.

[sky, 2017-01-06 23:38:32]

The reason for that is views shouldn't have any knowledge of style
information, that should be left up to the embedder. This way chrome
can easily customize style information to suit the current theme.

  -Scott

On Fri, Jan 6, 2017 at 3:17 PM,  <msw@chromium.org> wrote:
> It's a little weird that we are relying on chrome to supply views
> dialog/bubble
> insets, and weird that we are using this system instead of a
> layout_constants
> header or NativeTheme, but apparently I'm not up with the latest plans and
> patterns here... Are there docs I can read?
>
>
>
https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
> File chrome/browser/ui/views/chrome_views_delegate.cc (right):
>
>
https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
> chrome/browser/ui/views/chrome_views_delegate.cc:569: return
> gfx::Insets(HarmonyLayoutDelegate::kHarmonyLayoutUnit,
> optional nit: use the ctor that takes one |all| arg.
>
>
https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
> File ui/views/views_delegate.cc (right):
>
>
https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
> ui/views/views_delegate.cc:148: return gfx::Insets(kPanelVertMargin,
> optional nit: use the ctor that takes |vertical|, |horizontal| args.
>
> https://codereview.chromium.org/2571613002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylix_rd, 2017-01-10 14:36:44]

https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:569: return
gfx::Insets(HarmonyLayoutDelegate::kHarmonyLayoutUnit,
On 2017/01/06 23:17:43, msw wrote:
> optional nit: use the ctor that takes one |all| arg.

Done.

https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/120001/ui/views/views_delegat...
ui/views/views_delegate.cc:148: return gfx::Insets(kPanelVertMargin,
On 2017/01/06 23:17:43, msw wrote:
> optional nit: use the ctor that takes |vertical|, |horizontal| args.

Done.

[sky, 2017-01-10 16:00:59]

LGTM

https://codereview.chromium.org/2571613002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:568: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
no {}

[kylix_rd, 2017-01-10 17:24:12]

https://codereview.chromium.org/2571613002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2571613002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:568: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
On 2017/01/10 16:00:59, sky wrote:
> no {}

Done.

[msw, 2017-01-10 19:29:22]

lgtm

[kylix_rd, 2017-01-10 19:39:49]

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

[commit-bot: I haz the power, 2017-01-10 19:40:24]

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-01-10 20:54:57]

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

[commit-bot: I haz the power, 2017-01-10 20:54:58]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2017-01-10 20:57:19]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2017-01-10 20:57:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2571613002/#ps160001
(title: "More fixed nits")

[commit-bot: I haz the power, 2017-01-10 20:57:42]

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-01-10 21:03:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484081839052960,
"parent_rev": "e5410705426f111d9ea78d121b70cb87ddf38187", "commit_rev":
"44ec2a4fd5e1c052077575f84b5b5cbdb703cef6"}

[commit-bot: I haz the power, 2017-01-10 21:03:48]

Description was changed from

==========
Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The
other issue regarding the wrapping of text should be a separate issue and
confined to the implementing views/controls.

BUG=652024
==========

to

==========
Convert Register Protocol Handler dialog for Harmony/Material Design

NOTE: this CL only addresses the primary content of the referenced issue. The
other issue regarding the wrapping of text should be a separate issue and
confined to the implementing views/controls.

BUG=652024

Review-Url: https://codereview.chromium.org/2571613002
Cr-Commit-Position: refs/heads/master@{#442683}
Committed:
https://chromium.googlesource.com/chromium/src/+/44ec2a4fd5e1c052077575f84b5b...
==========

[commit-bot: I haz the power, 2017-01-10 21:03:50]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/44ec2a4fd5e1c052077575f84b5b...