It looks as though the PageInfo bubble has not had its close button updated to MD like all the other Dialog Non-Client views.

WebsiteSettingsPopupView sets a custom button in its PopupHeaderView constructor, rather than just overriding BubbleDialogDelegateView::ShouldShowCloseButton() to return `true`.

There might be a reason for this in terms of alignment or something.

Changing the popup to use the regular non-client view and keeping the current alignment requires minor tweaks to BubbleDialogDelegateView, but it seems like a nice cleanup. -> https://codereview.chromium.org/2581493002

Does it look right? (screengrab attached. old on the left, new on the right).

Deleted: Screen Shot 2016-12-15 at 10.21.23 am.png 117 KB

	Screen Shot 2016-12-15 at 10.21.23 am.png 117 KB View Download

The main bug about Page Info using its own close button is  Issue 640851 .

 Issue 640851  has been merged into this issue.

tapted@ - thank you for making this tweak. According to the Harmony spec, the center of the x should be 16x16 from the top-right of the dialog. It looks like you need to scoot the x 3pt to the right and 1pt up to match this.

Made https://codereview.chromium.org/2610613002 for the close button position
(and picking up https://codereview.chromium.org/2581493002 again)

Updated side-by-side after review, corresponding to patchset 4: https://codereview.chromium.org/2581493002/#ps60001

This changes the layout slightly so that the Page Info bubble uses the standard margins that other bubbles use, while still keeping the details text and permissions items horizontally aligned.

(note the close button rejiggery is orthogonal and hasn't landed yet)

Deleted: Screen Shot 2017-01-05 at 1.09.17 pm.png 179 KB

	Screen Shot 2017-01-05 at 1.09.17 pm.png 179 KB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6d869558bf4e9b9b16c1284f438c7989dc61997

commit e6d869558bf4e9b9b16c1284f438c7989dc61997
Author: tapted <tapted@chromium.org>
Date: Thu Jan 05 04:13:52 2017

Ensure the Harmony close buttons appear in the correct place.

The Harmony spec states that the centre of the close button on dialogs
should be 16x16 from the top right of the window. Make it so.

This depends on the way lines are drawn in the vector icon. Values
chosen here were determined using the 1x assets and verified on Windows
and Mac.

BUG= 674269 

Review-Url: https://codereview.chromium.org/2610613002
Cr-Commit-Position: refs/heads/master@{#441581}

[modify] https://crrev.com/e6d869558bf4e9b9b16c1284f438c7989dc61997/ui/views/bubble/bubble_frame_view.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37240b4fdef1ec7017f59d4cb7ced8c655e5c595

commit 37240b4fdef1ec7017f59d4cb7ced8c655e5c595
Author: tapted <tapted@chromium.org>
Date: Wed Jan 11 03:33:37 2017

PageInfo bubble: Use the non-client view's window title and close button.

Currently it sets a custom close button which is different to the other
Harmony dialogs.

To keep items aligned, use the standard panel margins from
ui/views/layout_constants.h. (13px rather than 16px). This makes the
panel slightly more narrow.

BUG= 640851 ,  674269 

Review-Url: https://codereview.chromium.org/2581493002
Cr-Commit-Position: refs/heads/master@{#442779}

[modify] https://crrev.com/37240b4fdef1ec7017f59d4cb7ced8c655e5c595/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
[modify] https://crrev.com/37240b4fdef1ec7017f59d4cb7ced8c655e5c595/chrome/browser/ui/views/website_settings/website_settings_popup_view.h

(1) Is it correct to say this bug is specific to the PageInfo dialog, and not a general bug about all dialog close buttons (i.e. the other Harmonized dialogs have correct close button focus states)?  Or are there multiple dialogs whose close buttons need to be synchronized?  I'd like to make sure this bug is titled correctly.

(2) It looks from the Harmony specs (specifically, https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03c-dialog-specs.png%3Fz=width and https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-02-icons.png%3Fz=width ) like close buttons should always be 16x16, and their edges 8 px in from the dialog edge.  We should be asking the layout delegate for these values using the BUTTON_HEDGE_MARGIN_NEW and BUTTON_VEDGE_MARGIN_NEW enum values (see e.g. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/cookie_info_view.cc?q=BUTTON_HEDGE_MARGIN_NEW&sq=package:chromium&l=151&dr=C ).  It concerns me that the CL in comment 8 seems to be using completely different values, implying that the close button in question is too big (or is not aligned correctly), and the CL in comment 9 is using layout constants (from pre-Harmony days) instead of the layout delegate.

(1): Yes. (There was also  Issue 640851  which was more-specifically PageInfo).

(2): re pre-Harmony constants, there was a follow-up in https://codereview.chromium.org/2640593002 (for  Issue 681081 ) to make PageInfo inherit more layout from the bubble. #c9 wasn't yet trying to Harmonize the dialog, so things like views::kPanelHorizMargin were used, but since that follow-up landed in r444474, PageInfo should be inheriting the harmony padding.

(2): re close button margins - I agree these constants in bubble_frame_view.cc have a bit of a smell about them :/. A part of it could be some code in VectorIconButton:

void VectorIconButton::SetIcon(gfx::VectorIconId id) {
  id_ = id;

  if (!border()) {
    SetBorder(
        views::CreateEmptyBorder(kButtonExtraTouchSize, kButtonExtraTouchSize,
                                 kButtonExtraTouchSize, kButtonExtraTouchSize));
  }
}

That adds 4px. So the effective margins from the asset are right=8, top=9 which gets closer to the 8px from the spec. That extra 1px on the top could be due to the way the lines in the vector asset are being drawn. So BUTTON_{H,V}EDGE_MARGIN_NEW should be usable if we remove that kButtonExtraTouchSize border and update the coordinates in the vector assets.

For comment 11 item (2), see  bug 691895 .

 Issue 674266  has been merged into this issue.

I think the issues described here are all fixed. pkasting landed some awesome cleanups in r448723 and r447348 that address some of the notes in #c10-#c12.