Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16279002:
bookmarks: Convert "Remove" link into a LabelButton. (Closed)
|
Created:
7 years, 6 months ago by tfarina Modified:
7 years, 6 months ago CC:
chromium-reviews, Sebastien Gabriel, stromme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionbookmarks: Convert "Remove" link into a LabelButton.
This also tweaked all the paddings/spacings all around to make the bubble really closer to the mocks.
BUG=231694
TEST=ctrl+D or press the star button in the omnibox, observe the Bookmark Bubble,
there should be a "Remove" Button in the left side of "Edit.." button.
R=msw@chromium.org, sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207802
Patch Set 1 #
Total comments: 2
Patch Set 2 : getting there #
Total comments: 15
Patch Set 3 : override GetMinimumSize() #
Total comments: 4
Patch Set 4 : Spacing #Patch Set 5 : a bunch of changes #
Total comments: 6
Patch Set 6 : SetTitle() changes #Patch Set 7 : kSecondColumnSetID -> kFirstColumnSecondID #
Total comments: 6
Patch Set 8 : Mike changes #
Total comments: 8
Patch Set 9 : more Mike changes #
Total comments: 11
Patch Set 10 : revert IDS_ changes and set_color() #
Total comments: 7
Patch Set 11 : background color and SetArrowPaintType #
Total comments: 2
Patch Set 12 : reuse it #
Total comments: 4
Patch Set 13 : final changes? #
Total comments: 1
Messages
Total messages: 36 (0 generated)
https://codereview.chromium.org/16279002/diff/1/chrome/browser/ui/views/bookm... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/1/chrome/browser/ui/views/bookm... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:196: // Bottom (buttons) row. Try a different approach: Remove ColumnSet(2) (used for the input fields) altogether. Instead, use the same column set for the input rows and the button row. When adding the textfield and combobox views, use the GridLayout::AddView overload that takes a column span (of 3 or maybe more). This should make the views line up as we want. https://codereview.chromium.org/16279002/diff/1/chrome/browser/ui/views/bookm... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/16279002/diff/1/chrome/browser/ui/views/bookm... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:103: // Button for removing the bookmark. nit: combine the 3 button comments and remove the blank lines.
https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:169: views::Label* title_label = new views::Label( Try GetBubbleFrameView()->SetTitle(...); remove this, its column set, and row. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:191: cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing); This value (8) needs to be increased to match 15 in the spec. The closest constant is kUnrelatedControlHorizontalSpacing (12), use that. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:220: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); This value (4) needs to be increased to match 10 in the spec. The closest constant is kRelatedControlVerticalSpacing (8), use that. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:226: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); ditto, use kRelatedControlVerticalSpacing. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:237: BookmarkBubbleView::BookmarkBubbleView(views::View* anchor_view, The left/right/bottom insets are 7 and should be 20, like your other CL: https://codereview.chromium.org/15808006/diff/1/chrome/browser/ui/views/bookm... set_margins(gfx::Insets(20, 20, 20, 20)); You may use kUnrelatedControlLargeHorizontalSpacing and kUnrelatedControlVerticalSpacing. (top may be wrong, and depends on whether you use SetTitle() or not)
https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (left): https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:131: Override GetPreferredSize() or GetMinimumSize to return: view::Size size(views::BubbleDelegateView::GetPreferredSize()); size.SetToMax((gfx::Size(kMinBubbleWidth, 0)); return size; https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:47: const int kMinimumFieldSize = 180; Make this kMinBubbleWidth = 336 (maybe 340) to match spec. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:194: GridLayout::USE_PREF, 0, kMinimumFieldSize); Remove kMinimumFieldSize here.
looking good https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:43: // Minimum width for the fields - they will push out the size of the bubble if nit: update comment. https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:69: virtual gfx::Size GetMinimumSize() OVERRIDE; nit: // Overridden from views::View (or views::BubbleDelegateView)
https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:47: const int kMinimumFieldSize = 180; On 2013/06/01 01:18:48, msw wrote: > Make this kMinBubbleWidth = 336 (maybe 340) to match spec. Done. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:169: views::Label* title_label = new views::Label( On 2013/06/01 01:07:28, msw wrote: > Try GetBubbleFrameView()->SetTitle(...); remove this, its column set, and row. Not yet, looking into ViewHierarchyChanged() approach. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:191: cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing); On 2013/06/01 01:07:28, msw wrote: > This value (8) needs to be increased to match 15 in the spec. > The closest constant is kUnrelatedControlHorizontalSpacing (12), use that. Done. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:194: GridLayout::USE_PREF, 0, kMinimumFieldSize); On 2013/06/01 01:18:48, msw wrote: > Remove kMinimumFieldSize here. Done. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:220: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); On 2013/06/01 01:07:28, msw wrote: > This value (4) needs to be increased to match 10 in the spec. > The closest constant is kRelatedControlVerticalSpacing (8), use that. Done. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:226: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); On 2013/06/01 01:07:28, msw wrote: > ditto, use kRelatedControlVerticalSpacing. Done. https://codereview.chromium.org/16279002/diff/4001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:237: BookmarkBubbleView::BookmarkBubbleView(views::View* anchor_view, On 2013/06/01 01:07:28, msw wrote: > The left/right/bottom insets are 7 and should be 20, like your other CL: > https://codereview.chromium.org/15808006/diff/1/chrome/browser/ui/views/bookm... > set_margins(gfx::Insets(20, 20, 20, 20)); > You may use kUnrelatedControlLargeHorizontalSpacing and > kUnrelatedControlVerticalSpacing. > (top may be wrong, and depends on whether you use SetTitle() or not) Done. https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:43: // Minimum width for the fields - they will push out the size of the bubble if On 2013/06/01 02:05:53, msw wrote: > nit: update comment. Done. https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h (right): https://codereview.chromium.org/16279002/diff/9001/chrome/browser/ui/views/bo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h:69: virtual gfx::Size GetMinimumSize() OVERRIDE; On 2013/06/01 02:05:53, msw wrote: > nit: // Overridden from views::View > (or views::BubbleDelegateView) Done.
https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:40: // Minimum size of the bubble. This is used to shrink the width of the bubble Actually, this minimum width is clamping the bubble to be bigger. If you remove this minimum width (now that the kMinimumFieldSize is gone), the bubble would be smaller than we'd want. You should just say "Minimum width of the bubble.", and increase the value to 340 (336 is a little smaller than the spec). https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:182: // Top (title) row. Still hoping you'll use the bubble's title feature with a smaller top inset. https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:259: set_margins(gfx::Insets(13, reduce the top inset here (probably to 0), if you're using the bubble title.
patch set 6 -> http://imgur.com/Dows9BK https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:40: // Minimum size of the bubble. This is used to shrink the width of the bubble On 2013/06/01 18:04:38, msw wrote: > Actually, this minimum width is clamping the bubble to be bigger. If you remove > this minimum width (now that the kMinimumFieldSize is gone), the bubble would be > smaller than we'd want. You should just say "Minimum width of the bubble.", and > increase the value to 340 (336 is a little smaller than the spec). Done. https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:182: // Top (title) row. On 2013/06/01 18:04:38, msw wrote: > Still hoping you'll use the bubble's title feature with a smaller top inset. Done. https://codereview.chromium.org/16279002/diff/15001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:259: set_margins(gfx::Insets(13, On 2013/06/01 18:04:38, msw wrote: > reduce the top inset here (probably to 0), if you're using the bubble title. Done.
I think this is looking pretty good, but want to confirm with PM/UX. +CC Sebastien and Andrew; see http://imgur.com/Dows9BK or later updates. https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:44: // The padding used between textfield and combobox and between combobox and the Sorry, I was wrong, you can remove this. https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:199: layout->AddPaddingRow(0, kVerticalSpacing); Sorry, this should be 12 to match spec; use kUnrelatedControlHorizontalSpacing. https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:205: layout->AddPaddingRow(0, kVerticalSpacing); Sorry, this should be ~8 to match spec; use kRelatedControlVerticalSpacing.
before -> http://imgur.com/t65iLri after -> http://imgur.com/Dows9BK
patch set 8 -> http://imgur.com/uSbQmDu https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:44: // The padding used between textfield and combobox and between combobox and the On 2013/06/02 20:59:52, msw wrote: > Sorry, I was wrong, you can remove this. Done. https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:199: layout->AddPaddingRow(0, kVerticalSpacing); On 2013/06/02 20:59:52, msw wrote: > Sorry, this should be 12 to match spec; use kUnrelatedControlHorizontalSpacing. Done. https://codereview.chromium.org/16279002/diff/24001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:205: layout->AddPaddingRow(0, kVerticalSpacing); On 2013/06/02 20:59:52, msw wrote: > Sorry, this should be ~8 to match spec; use kRelatedControlVerticalSpacing. Done.
This is so close to spec, that Sebastien and Andrew believe that we should finish the last bits of polish before landing. Unfortunately, that means you'll have to wait until I can update the textfield and combobox assets (I'll do it this week), and then make sure the padding is still right. I might also tweak the button heights, and adjust the title positioning on CrOS, and make sure this all works on Windows too. Make the suggested changes and I'll follow up asap on my end. https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: views::kUnrelatedControlLargeHorizontalSpacing, Please change the left inset to 19. https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:235: views::kUnrelatedControlVerticalSpacing, Please change the bottom inset to 18. https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:236: views::kUnrelatedControlLargeHorizontalSpacing)); Please change the right inset to 18. https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:237: // Compensate for built-in vertical padding in the anchor view's image. Please also set_color() to the native theme dialog color. https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: GetBubbleFrameView()->SetTitle(l10n_util::GetStringUTF16( Please also set_paint_arrow(PAINT_TRANSPARENT); https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:324: newly_bookmarked_ ? IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARKED Update these strings to remove the colons ':'
https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: GetBubbleFrameView()->SetTitle(l10n_util::GetStringUTF16( On 2013/06/04 00:57:47, msw wrote: > Please also set_paint_arrow(PAINT_TRANSPARENT); What is this for? To "remove" the arrow? If yes, then I think we can pass BubbleBorder::NONE?
https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: GetBubbleFrameView()->SetTitle(l10n_util::GetStringUTF16( On 2013/06/08 23:33:21, tfarina wrote: > On 2013/06/04 00:57:47, msw wrote: > > Please also set_paint_arrow(PAINT_TRANSPARENT); > What is this for? To "remove" the arrow? If yes, then I think we can pass > BubbleBorder::NONE? Yeah, remove the arrow with PAINT_NONE, not PAINT_TRANSPARENT. Don't use the Arrow enum NONE, that will anchor the bubble incorrectly.
On Mon, Jun 3, 2013 at 9:57 PM, <msw@chromium.org> wrote: > I have made all the required changes below except the set_color one. After -> http://imgur.com/c99X7ue > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): > > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: > views::kUnrelatedControlLargeHorizontalSpacing, > Please change the left inset to 19. > Done. > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:235: > views::kUnrelatedControlVerticalSpacing, > Please change the bottom inset to 18. > Done. > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:236: > views::kUnrelatedControlLargeHorizontalSpacing)); > Please change the right inset to 18. > Done. > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:237: // > Compensate for built-in vertical padding in the anchor view's image. > Please also set_color() to the native theme dialog color. > What is the native theme dialog color? Why we need to manually set it? Could you point me to an example? > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: > GetBubbleFrameView()->SetTitle(l10n_util::GetStringUTF16( > Please also set_paint_arrow(PAINT_TRANSPARENT); > Done. > https://codereview.chromium.org/16279002/diff/29001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:324: > newly_bookmarked_ ? IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARKED > Update these strings to remove the colons ':' > Done. > https://codereview.chromium.org/16279002/ Note that I'm not uploading my current changes due to issues with my internet connection not working with codereview/git cl upload. I have attached my changes, in any case. -- Thiago
Now the completed diff (with cocoa, ios, and gtk changes included). It's against the latest patch set uploaded. -- Thiago
Mike! The ISP guys were here and changed my modem and that fixed the issue with uploading! Yay, finally. Please, review patch set 9! Thanks!
This is looking really good, almost as close to the mocks as possible. Sorry I haven't had time to update the combobox and textfield assets yet, you can probably land before I do that. https://codereview.chromium.org/16279002/diff/44001/chrome/app/bookmarks_stri... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/16279002/diff/44001/chrome/app/bookmarks_stri... chrome/app/bookmarks_strings.grdp:186: <message name="IDS_BOOKMARK_BUBBLE_REMOVE_BUTTON" desc="Title of the button for removing the bookmark"> Keep the IDS names the same for now, to make this change simpler. If you do this in a followup, drop the '_BUTTON', '_LABEL' postfixes. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:42: const int kMinBubbleWidth = 340; Sorry, I measured with old insets, increase this to 350. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:143: void BookmarkBubbleView::Init() { set_color(GetNativeTheme()->GetSystemColor( ui::NativeTheme::kColorId_DialogBackground)) https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:180: // We subtract 2 to account for the natural button padding, and Remove this comment and the '- 2' in the padding column below; it's 2px off. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: views::kUnrelatedControlLargeHorizontalSpacing)); This should be still be 18, result: set_margins(gfx::Insets(0, 19, 18, 18)); https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:324: GetBubbleFrameView()->bubble_border()->set_paint_arrow( Stefan is adding BubbleDelegateView::SetArrowPaintType in https://codereview.chromium.org/16909006/, use thatif you land after him.
Patch set 10 -> http://imgur.com/UHeELRx https://codereview.chromium.org/16279002/diff/44001/chrome/app/bookmarks_stri... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/16279002/diff/44001/chrome/app/bookmarks_stri... chrome/app/bookmarks_strings.grdp:186: <message name="IDS_BOOKMARK_BUBBLE_REMOVE_BUTTON" desc="Title of the button for removing the bookmark"> On 2013/06/15 20:51:44, msw wrote: > Keep the IDS names the same for now, to make this change simpler. > If you do this in a followup, drop the '_BUTTON', '_LABEL' postfixes. Done. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:42: const int kMinBubbleWidth = 340; On 2013/06/15 20:51:44, msw wrote: > Sorry, I measured with old insets, increase this to 350. Done. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:143: void BookmarkBubbleView::Init() { On 2013/06/15 20:51:44, msw wrote: > set_color(GetNativeTheme()->GetSystemColor( > ui::NativeTheme::kColorId_DialogBackground)) Done. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:180: // We subtract 2 to account for the natural button padding, and On 2013/06/15 20:51:44, msw wrote: > Remove this comment and the '- 2' in the padding column below; it's 2px off. Done. https://codereview.chromium.org/16279002/diff/44001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: views::kUnrelatedControlLargeHorizontalSpacing)); On 2013/06/15 20:51:44, msw wrote: > This should be still be 18, result: set_margins(gfx::Insets(0, 19, 18, 18)); Done.
https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:42: const int kMinBubbleWidth = 350; The width did not increase in your latest screenshot, can you investigate? https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:144: set_color(GetNativeTheme()->GetSystemColor( The latest screenshot shows the contents area white (while the frame is indeed gray), can you remove this view's white background, or use the same gray color for a solid background on this view? (or maybe just moving this call to the constructor would suffice?) https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: GetBubbleFrameView()->bubble_border()->set_paint_arrow( Stefan's change landed as http://crrev.com/206628 Please sync/rebase and use the base class' SetArrowPaintType
patch set 11 -> http://imgur.com/SDFFO0N https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:144: set_color(GetNativeTheme()->GetSystemColor( On 2013/06/16 22:14:31, msw wrote: > The latest screenshot shows the contents area white (while the frame is indeed > gray), can you remove this view's white background, or use the same gray color > for a solid background on this view? (or maybe just moving this call to the > constructor would suffice?) Done. https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:323: GetBubbleFrameView()->bubble_border()->set_paint_arrow( On 2013/06/16 22:14:31, msw wrote: > Stefan's change landed as http://crrev.com/206628 > Please sync/rebase and use the base class' SetArrowPaintType Done.
Looks great! LGTM with nits. Scott, please take a look. Should all platforms should use the new strings (no ':')? https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:42: const int kMinBubbleWidth = 350; On 2013/06/16 22:14:31, msw wrote: > The width did not increase in your latest screenshot, can you investigate? We can overlook this for now, if you file a bug to follow up. The bubble is just negligibly smaller than spec'ed. https://codereview.chromium.org/16279002/diff/63001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/63001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:232: const SkColor kBackgroundColor = SkColorSetRGB(251, 251, 251); Use the same color for both, not a hard-coded color, rename. const SkColor background_color = GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_DialogBackground) set_color(background_color); set_background(views::Background::CreateSolidBackground(background_color));
https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/53001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:42: const int kMinBubbleWidth = 350; On 2013/06/20 19:48:56, msw wrote: > On 2013/06/16 22:14:31, msw wrote: > > The width did not increase in your latest screenshot, can you investigate? > > We can overlook this for now, if you file a bug to follow up. > The bubble is just negligibly smaller than spec'ed. I'll file a bug for it. https://codereview.chromium.org/16279002/diff/63001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/63001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:232: const SkColor kBackgroundColor = SkColorSetRGB(251, 251, 251); On 2013/06/20 19:48:56, msw wrote: > Use the same color for both, not a hard-coded color, rename. > const SkColor background_color = > GetNativeTheme()->GetSystemColor(ui::NativeTheme::kColorId_DialogBackground) > set_color(background_color); > set_background(views::Background::CreateSolidBackground(background_color)); Done.
https://codereview.chromium.org/16279002/diff/69001/chrome/app/bookmarks_stri... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/16279002/diff/69001/chrome/app/bookmarks_stri... chrome/app/bookmarks_strings.grdp:186: <message name="IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK" desc="Title of the button for removing the bookmark"> Actually, we're in a string freeze for M-29 branch right now. Revert the string change for now, we'll follow up later. https://codereview.chromium.org/16279002/diff/69001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/69001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:236: set_anchor_view_insets(gfx::Insets(5, 0, 5, 0)); I just noticed that the bubble is 2px too low from spec. Change this to set_anchor_view_insets(gfx::Insets(7, 0, 7, 0));
Patch set 13 -> http://imgur.com/cMMHl9h https://codereview.chromium.org/16279002/diff/69001/chrome/app/bookmarks_stri... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/16279002/diff/69001/chrome/app/bookmarks_stri... chrome/app/bookmarks_strings.grdp:186: <message name="IDS_BOOKMARK_BUBBLE_REMOVE_BOOKMARK" desc="Title of the button for removing the bookmark"> On 2013/06/20 20:12:47, msw wrote: > Actually, we're in a string freeze for M-29 branch right now. > Revert the string change for now, we'll follow up later. Done. https://codereview.chromium.org/16279002/diff/69001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/69001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:236: set_anchor_view_insets(gfx::Insets(5, 0, 5, 0)); On 2013/06/20 20:12:47, msw wrote: > I just noticed that the bubble is 2px too low from spec. > Change this to set_anchor_view_insets(gfx::Insets(7, 0, 7, 0)); Done.
LGTM. Scott should review for OWNERS. Andrew and Sebastien should check that this is a good step.
Mike, are you making sure Andrew and Sebastien want this change?
On 2013/06/20 21:41:08, sky wrote: > Mike, are you making sure Andrew and Sebastien want this change? Andrew said we wanted it for M-29 a few days ago. He suggested that it'd be easy to *optionally* revert this change if the new dialog style is reverted from M-29. Both Andrew and Sebastien are CC'ed and have seen this WIP at some point, but I'll ping them again.
LGTM
sgabriel: "Yes we should make it a button"
Message was sent while issue was closed.
Committed patchset #13 manually as r207802 (presubmit successful).
Message was sent while issue was closed.
Mike, the SetTitle() thing broke a test: http://build.chromium.org/p/chromium.win/builders/Interactive%20Tests%20%28db... This will have to be reverted.
Message was sent while issue was closed.
On 2013/06/21 14:05:29, tfarina wrote: > Mike, the SetTitle() thing broke a test: > > http://build.chromium.org/p/chromium.win/builders/Interactive%2520Tests%2520%... > > This will have to be reverted. Dang, too bad the trybot runs in patch set 10 didn't run interactive ui tests... I guess the Win-specific border widget (and thus frame view) isn't created yet. Upload the patch to a new code review issue, then revert the title changes. Try to make the title and its padding from the content match the mocks.
Message was sent while issue was closed.
I'm working on a sync promo message in this bubble. I added a question to better understand your code. https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: set_margins(gfx::Insets(0, 19, 18, 18)); Why is 19px on the left and 18px on the right? Is it -1 on the right to compensate the text field/combo box/button border? Thanks.
Message was sent while issue was closed.
On 2013/06/25 19:01:28, fdoray wrote: > I'm working on a sync promo message in this bubble. I added a question to better > understand your code. > > https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): > > https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: > set_margins(gfx::Insets(0, 19, 18, 18)); > Why is 19px on the left and 18px on the right? Is it -1 on the right to > compensate the text field/combo box/button border? Thanks. Yeah, it's just to match the label (left) and button/combobox/textfield assets (right) insets to match the spec. We could probably change it to make them consistent if that matters for some reason.
Message was sent while issue was closed.
On 2013/06/25 19:01:28, fdoray wrote: > I'm working on a sync promo message in this bubble. I added a question to better > understand your code. > > https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... > File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): > > https://codereview.chromium.org/16279002/diff/74001/chrome/browser/ui/views/b... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:234: > set_margins(gfx::Insets(0, 19, 18, 18)); > Why is 19px on the left and 18px on the right? Is it -1 on the right to > compensate the text field/combo box/button border? Thanks. Yeah, it's just to match the label (left) and button/combobox/textfield assets (right) insets to match the spec. We could probably change it to make them consistent if that matters for some reason. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
