Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2869683003:
Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. (Closed)
DescriptionViews/Harmony: Remove references to layout constants in c/b/u/v/passwords.
Remove unnecessary includes for ui/views/layout/layout_constants.h and do some
refactoring to remove references to layout constants in
chrome/browser/ui/views/passwords/*.
This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and
ManagePasswordsBubbleDialogViewTest.
BUG=691897
Review-Url: https://codereview.chromium.org/2869683003
Cr-Commit-Position: refs/heads/master@{#472733}
Committed: https://chromium.googlesource.com/chromium/src/+/f195c43cff20208422063e0d9179b4a29415c28d
Patch Set 1 #
Total comments: 56
Patch Set 2 : Review comments + other whitespace changes. #
Total comments: 20
Patch Set 3 : Review comments. #
Total comments: 22
Patch Set 4 : Review comments. #
Total comments: 2
Patch Set 5 : Review comments. #Messages
Total messages: 39 (28 generated)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views/Harmony: Remove references to layout constants in c/b/u/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 ========== to ========== Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/views/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 ==========
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hey Trent, PTAL?
Description was changed from ========== Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/views/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 ========== to ========== Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/views/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vasilii@ should take a look too when it's ready (and will give you OWNERS) And below, I've tried to just suggest names that seem to make sense in the surrounding context rather than things that happen to have matching values. I think we want a CL that doesn't change any layout or fonts when --secondary-ui-md isn't set, so we may need to substitute some of the suggestions with a comment. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_test.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_test.cc:28: std::string kTestOrigin = "https://www.example.com"; these will make static initializers (basically every test run will need convert ASCII->utf and malloc these strings) - `constexpr char kFooBar[] = ".."` instead https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_test.cc:31: } // namespace https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:44: bool padding = (type == SINGLE_VIEW_COLUMN_SET); padding -> add_padding? (or in fact I'd suggest still adding columns, but making it zero for SINGLE_VIEW_COLUMN_SET_NO_PADDING -- this ensures the columns have the same indexes, which can be important for identifying them later) https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:46: DISTANCE_DIALOG_BUTTON_MARGIN); (I mentioned that DISTANCE_PANEL_CONTENT_MARGIN could just be GetInsetsMetric(views::INSETS_DIALOG_BUTTON).left/right(), but I guess it's OK to use DISTANCE_PANEL_CONTENT_MARGIN, so long as it's just used for horizontal padding) https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:215: layout_provider->GetDistanceMetric(DISTANCE_PANEL_CONTENT_MARGIN)); I don't think DISTANCE_PANEL_CONTENT_MARGIN is right here should it be GetInsetsMetric(views::INSETS_BUBBLE_TITLE).top() ? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:227: // DialogClientView adds more vertical padding for space above the buttons. above -> below? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:229: views::DISTANCE_RELATED_CONTROL_VERTICAL)); similar thing here - should it be GetInsetsMetric(views::INSETS_DIALOG_BUTTON).bottom()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:158: title_label->SetFontList(views::style::GetFont( this is a vanilla views::Label, so views::style::CONTEXT_DIALOG_TITLE can be passed in to the constructor two lines above (and this line removed) https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:160: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_margin); INSETS_BUBBLE_TITLE.top() https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:169: views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY)); I think this needs to be CONTEXT_DEPRECATED_SMALL to retain the old behaviour https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:178: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_padding); bring const int vertical_padding = .. DISTANCE_UNRELATED_CONTROL_VERTICAL down here. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:183: layout->StartRowWithPadding(0, DOUBLE_BUTTON_COLUMN_SET, 0, vertical_padding); The old code was 3*related, not 2*related, so there is a behaviour change here - is that intentional? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:190: layout->AddPaddingRow(0, vertical_margin); GetInsetsMetric(views::INSETS_DIALOG_BUTTON).bottom() https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/credentials_selection_view.cc:22: views::Label* label = new views::Label(form.password_value); CONTEXT_DEPRECATED_SMALL to the constructor https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:75: new views::Label(GetDisplayUsername(form))); CONTEXT_DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:90: std::unique_ptr<views::Label> label(new views::Label(text)); CONTEXT_DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:116: std::unique_ptr<views::Label> text(new views::Label( CONTEXT_DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:130: undo_link->SetFontList(views::style::GetFont(views::style::CONTEXT_LABEL, CONTEXT_DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:299: layout->AddPaddingRow(0, ChromeLayoutProvider::Get()->GetDistanceMetric( ChromeLayoutProvider::Get() -> local var https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:189: DISTANCE_UNRELATED_CONTROL_VERTICAL)); INSETS_BUBBLE_CONTENTS.top()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:343: DISTANCE_UNRELATED_CONTROL_VERTICAL)); INSETS_BUBBLE_CONTENTS.bottom()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:420: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_padding); INSETS_BUBBLE_CONTENTS.top()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:431: l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_NO_PASSWORDS)); DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:449: layout->StartRowWithPadding(0, LINK_BUTTON_COLUMN_SET, 0, vertical_padding); INSETS_BUBBLE_CONTENTS.bottom()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:511: views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY)); DEPRECATED_SMALL https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:676: DISTANCE_UNRELATED_CONTROL_VERTICAL)); INSETS_BUBBLE_CONTENTS.bottom()? https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:473: controller()->OnPromptEnableAutoSignin(); make sure there's no path that results in a message loop being run via this function - see http://crbug.com/716681
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Trent, PTAL? I made a few other padding changes as well. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_test.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_test.cc:28: std::string kTestOrigin = "https://www.example.com"; On 2017/05/10 05:30:10, tapted wrote: > these will make static initializers (basically every test run will need convert > ASCII->utf and malloc these strings) - `constexpr char kFooBar[] = ".."` instead Ooh, I didn't know that, thanks! Fixed. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_test.cc:31: } On 2017/05/10 05:30:11, tapted wrote: > // namespace Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:44: bool padding = (type == SINGLE_VIEW_COLUMN_SET); On 2017/05/10 05:30:11, tapted wrote: > padding -> add_padding? > > (or in fact I'd suggest still adding columns, but making it zero for > SINGLE_VIEW_COLUMN_SET_NO_PADDING -- this ensures the columns have the same > indexes, which can be important for identifying them later) Done (implemented your second option :) ) https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:46: DISTANCE_DIALOG_BUTTON_MARGIN); On 2017/05/10 05:30:11, tapted wrote: > (I mentioned that DISTANCE_PANEL_CONTENT_MARGIN could just be > GetInsetsMetric(views::INSETS_DIALOG_BUTTON).left/right(), but I guess it's OK > to use DISTANCE_PANEL_CONTENT_MARGIN, so long as it's just used for horizontal > padding) Acknowledged. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:215: layout_provider->GetDistanceMetric(DISTANCE_PANEL_CONTENT_MARGIN)); On 2017/05/10 05:30:11, tapted wrote: > I don't think DISTANCE_PANEL_CONTENT_MARGIN is right here > > should it be GetInsetsMetric(views::INSETS_BUBBLE_TITLE).top() ? I think they're the same? I switched to use the insets though. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:227: // DialogClientView adds more vertical padding for space above the buttons. On 2017/05/10 05:30:11, tapted wrote: > above -> below? No - it's using GetDialogButtons() later on, so the bottom padding is taken care of. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:229: views::DISTANCE_RELATED_CONTROL_VERTICAL)); On 2017/05/10 05:30:11, tapted wrote: > similar thing here - should it be > GetInsetsMetric(views::INSETS_DIALOG_BUTTON).bottom()? Done, + bubble content insets bottom. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:158: title_label->SetFontList(views::style::GetFont( On 2017/05/10 05:30:11, tapted wrote: > this is a vanilla views::Label, so views::style::CONTEXT_DIALOG_TITLE can be > passed in to the constructor two lines above (and this line removed) Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:160: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_margin); On 2017/05/10 05:30:11, tapted wrote: > INSETS_BUBBLE_TITLE.top() Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:169: views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY)); On 2017/05/10 05:30:11, tapted wrote: > I think this needs to be CONTEXT_DEPRECATED_SMALL to retain the old behaviour Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:178: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_padding); On 2017/05/10 05:30:11, tapted wrote: > bring const int vertical_padding = .. DISTANCE_UNRELATED_CONTROL_VERTICAL down > here. Done, I separated the view creation and layout. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:183: layout->StartRowWithPadding(0, DOUBLE_BUTTON_COLUMN_SET, 0, vertical_padding); On 2017/05/10 05:30:11, tapted wrote: > The old code was 3*related, not 2*related, so there is a behaviour change here - > is that intentional? Kinda? I guess I was trying to following a precedent pkasting set in a previous CL where he was OK with changing the current behaviour if it was more "correct", and the multiplier here seemed kinda arbitrary to me. I'll defer to your opinion, so let me know what you think? (For now, I switched to using the top inset for dialog buttons.) https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:190: layout->AddPaddingRow(0, vertical_margin); On 2017/05/10 05:30:11, tapted wrote: > GetInsetsMetric(views::INSETS_DIALOG_BUTTON).bottom() Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/credentials_selection_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/credentials_selection_view.cc:22: views::Label* label = new views::Label(form.password_value); On 2017/05/10 05:30:11, tapted wrote: > CONTEXT_DEPRECATED_SMALL to the constructor Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/credentials_selection_view.cc:61: views::DISTANCE_RELATED_CONTROL_VERTICAL)); Deleted this, because I realised that this is always beneath the title (which already has padding under it, either handled by the dialog delegate or by manage_passwords_bubble_view.cc's AddTitleRowLink()). https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:75: new views::Label(GetDisplayUsername(form))); On 2017/05/10 05:30:11, tapted wrote: > CONTEXT_DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:90: std::unique_ptr<views::Label> label(new views::Label(text)); On 2017/05/10 05:30:11, tapted wrote: > CONTEXT_DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:116: std::unique_ptr<views::Label> text(new views::Label( On 2017/05/10 05:30:11, tapted wrote: > CONTEXT_DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:130: undo_link->SetFontList(views::style::GetFont(views::style::CONTEXT_LABEL, On 2017/05/10 05:30:11, tapted wrote: > CONTEXT_DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_password_items_view.cc:299: layout->AddPaddingRow(0, ChromeLayoutProvider::Get()->GetDistanceMetric( On 2017/05/10 05:30:11, tapted wrote: > ChromeLayoutProvider::Get() -> local var Oops, thank you. I made DISTANCE_RELATED_CONTROL_VERTICAL a local var instead. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:189: DISTANCE_UNRELATED_CONTROL_VERTICAL)); On 2017/05/10 05:30:12, tapted wrote: > INSETS_BUBBLE_CONTENTS.top()? Done. I also went ahead and changed other places where there was padding between the title and content / content and buttons. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:343: DISTANCE_UNRELATED_CONTROL_VERTICAL)); On 2017/05/10 05:30:12, tapted wrote: > INSETS_BUBBLE_CONTENTS.bottom()? Done, + button insets top below. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:420: layout->StartRowWithPadding(0, SINGLE_VIEW_COLUMN_SET, 0, vertical_padding); On 2017/05/10 05:30:12, tapted wrote: > INSETS_BUBBLE_CONTENTS.top()? Deleted this, because this is also always beneath the title (which already has padding under it, either handled by the dialog delegate or by manage_passwords_bubble_view.cc's AddTitleRowLink()). https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:431: l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_NO_PASSWORDS)); On 2017/05/10 05:30:12, tapted wrote: > DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:449: layout->StartRowWithPadding(0, LINK_BUTTON_COLUMN_SET, 0, vertical_padding); On 2017/05/10 05:30:12, tapted wrote: > INSETS_BUBBLE_CONTENTS.bottom()? Done, + button insets top https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:511: views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY)); On 2017/05/10 05:30:12, tapted wrote: > DEPRECATED_SMALL Done. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:525: views::DISTANCE_RELATED_CONTROL_VERTICAL)); Changed this to bubble contents bottom + button row top. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:676: DISTANCE_UNRELATED_CONTROL_VERTICAL)); On 2017/05/10 05:30:12, tapted wrote: > INSETS_BUBBLE_CONTENTS.bottom()? Done + button row top. https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2869683003/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:473: controller()->OnPromptEnableAutoSignin(); On 2017/05/10 05:30:12, tapted wrote: > make sure there's no path that results in a message loop being run via this > function - see http://crbug.com/716681 Looks OK, thanks for the FYI :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with the following https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:229: // DialogClientView adds more vertical padding for space above the buttons. nit: this comment probably not needed, since `bubble_insets.bottom()` makes sense on its own IMO https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:231: layout->AddPaddingRow( did you mean to keep this second row? (if DialogClientView should add this if GetDialogButtons returns something, otherwise this should be added closer to where the buttons are added as subviews) https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:195: button_insets.top()); nice! https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:76: new views::Label(GetDisplayUsername(form), CONTEXT_DEPRECATED_SMALL)); might as well use base::MakeUnique per the recent chromium-dev thread -- https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:90: new views::Label(text, CONTEXT_DEPRECATED_SMALL)); same here https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:115: new views::Label(l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_DELETED), and here https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:297: if (row != password_forms_rows_[0]) { nit: curlies not required https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:187: title_label->AddStyleRange(gfx::Range(0, 3), GetLinkStyle()); stray line? https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:419: ChromeLayoutProvider* layout_provider = ChromeLayoutProvider::Get(); nit: move closer to first use https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:512: ChromeLayoutProvider* layout_provider = ChromeLayoutProvider::Get(); nit: move closer to first use
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
patricialor@chromium.org changed reviewers: + pkasting@chromium.org
Thanks Trent! pkasting, PTAL? CCing vasillii as FYI. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:229: // DialogClientView adds more vertical padding for space above the buttons. On 2017/05/12 03:36:10, tapted wrote: > nit: this comment probably not needed, since `bubble_insets.bottom()` makes > sense on its own IMO Done. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:231: layout->AddPaddingRow( On 2017/05/12 03:36:10, tapted wrote: > did you mean to keep this second row? (if DialogClientView should add this if > GetDialogButtons returns something, otherwise this should be added closer to > where the buttons are added as subviews) Deleted! Thanks :) https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:195: button_insets.top()); On 2017/05/12 03:36:10, tapted wrote: > nice! Thanks? Haha :) https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:76: new views::Label(GetDisplayUsername(form), CONTEXT_DEPRECATED_SMALL)); On 2017/05/12 03:36:10, tapted wrote: > might as well use base::MakeUnique per the recent chromium-dev thread -- > https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... Done, thanks for the FYI! https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:90: new views::Label(text, CONTEXT_DEPRECATED_SMALL)); On 2017/05/12 03:36:10, tapted wrote: > same here Done. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:115: new views::Label(l10n_util::GetStringUTF16(IDS_MANAGE_PASSWORDS_DELETED), On 2017/05/12 03:36:10, tapted wrote: > and here Done. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:297: if (row != password_forms_rows_[0]) { On 2017/05/12 03:36:10, tapted wrote: > nit: curlies not required Done. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:187: title_label->AddStyleRange(gfx::Range(0, 3), GetLinkStyle()); On 2017/05/12 03:36:10, tapted wrote: > stray line? Whoops, yep. Deleted :O https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:419: ChromeLayoutProvider* layout_provider = ChromeLayoutProvider::Get(); On 2017/05/12 03:36:10, tapted wrote: > nit: move closer to first use Done. https://codereview.chromium.org/2869683003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:512: ChromeLayoutProvider* layout_provider = ChromeLayoutProvider::Get(); On 2017/05/12 03:36:10, tapted wrote: > nit: move closer to first use Done.
https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:89: DISTANCE_DIALOG_BUTTON_MARGIN); Maybe this should be doing something like (pseudocode): insets = GetInsetsMetric(views::INSETS_PANEL); views::CreateEmptyBorder(..., insets.left(), ..., insets.right()); ...instead of using DIALOG_BUTTON_MARGIN directly, since you're basically trying to match the code below. (See also comment there about using INSETS_PANEL instead of the bubble-related insets.) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:91: views::CreateEmptyBorder(kVerticalAvatarMargin, horizontal_padding, Seems like kVerticalAvatarMargin should maybe be DISTANCE_RELATED_CONTROL_VERTICAL? (They have the same value AFAICT, at least pre-Harmony.) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:216: layout_provider->GetInsetsMetric(views::INSETS_BUBBLE_TITLE).top()); I think this should have been INSETS_DIALOG_TITLE, and the one below INSETS_PANEL. Nothing in here should talk about bubbles. Given how you're using these, where you're only pulling the vertical margins out, it doesn't matter. But conceptually, this is a dialog, not a bubble, and the use of DISTANCE_DIALOG_BUTTON_MARGIN in the credentials view matches that. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:37: layout_provider->GetDistanceMetric(DISTANCE_DIALOG_BUTTON_MARGIN); Nit: Same idea about maybe using the left/right INSETS_PANEL values instead of using this directly. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:173: // Buttons. Can we fix the fact that this dialog is creating its own buttons and adding them manually, instead of using GetDialogButtons()? I mean, maybe we can't in this CL, but can we in a separate one? That would remove the need for the column set enum in this file entirely AFAICT. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:185: layout_provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS); Again, this should be INSETS_PANEL. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:75: auto label(base::MakeUnique<views::Label>(GetDisplayUsername(form), Nit: Use = over () for simple initializations like this; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... (3 places) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: ->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS) Pretty sure all of these should be INSETS_PANEL too. (5 places) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:456: layout_provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON).top()); This looks like another case where I'm wondering if we can avoid rolling our own buttons. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:692: layout_provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON).top()); And here
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks Peter, PTAL https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:89: DISTANCE_DIALOG_BUTTON_MARGIN); On 2017/05/15 19:51:41, Peter Kasting wrote: > Maybe this should be doing something like (pseudocode): > > insets = GetInsetsMetric(views::INSETS_PANEL); > views::CreateEmptyBorder(..., insets.left(), ..., insets.right()); > > ...instead of using DIALOG_BUTTON_MARGIN directly, since you're basically trying > to match the code below. (See also comment there about using INSETS_PANEL > instead of the bubble-related insets.) Done. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:91: views::CreateEmptyBorder(kVerticalAvatarMargin, horizontal_padding, On 2017/05/15 19:51:41, Peter Kasting wrote: > Seems like kVerticalAvatarMargin should maybe be > DISTANCE_RELATED_CONTROL_VERTICAL? (They have the same value AFAICT, at least > pre-Harmony.) Done. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/account_chooser_dialog_view.cc:216: layout_provider->GetInsetsMetric(views::INSETS_BUBBLE_TITLE).top()); On 2017/05/15 19:51:41, Peter Kasting wrote: > I think this should have been INSETS_DIALOG_TITLE, and the one below > INSETS_PANEL. Nothing in here should talk about bubbles. > > Given how you're using these, where you're only pulling the vertical margins > out, it doesn't matter. But conceptually, this is a dialog, not a bubble, and > the use of DISTANCE_DIALOG_BUTTON_MARGIN in the credentials view matches that. Oops - I think I got all mixed up from some of the other files in this CL being bubbles and started using those metrics here, too - thanks for picking this up! Fixed. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:37: layout_provider->GetDistanceMetric(DISTANCE_DIALOG_BUTTON_MARGIN); On 2017/05/15 19:51:42, Peter Kasting wrote: > Nit: Same idea about maybe using the left/right INSETS_PANEL values instead of > using this directly. Done. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:173: // Buttons. On 2017/05/15 19:51:42, Peter Kasting wrote: > Can we fix the fact that this dialog is creating its own buttons and adding them > manually, instead of using GetDialogButtons()? > > I mean, maybe we can't in this CL, but can we in a separate one? That would > remove the need for the column set enum in this file entirely AFAICT. I filed crbug.com/723495 for it so I won't forget, but at the moment it's queued up after finishing replacing all the layout constants first - if someone else hasn't got it before then, I'll work on it. If you think it's more important I can work on it for my next Harmony thing - let me know :) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:185: layout_provider->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS); On 2017/05/15 19:51:42, Peter Kasting wrote: > Again, this should be INSETS_PANEL. Done. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_password_items_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_password_items_view.cc:75: auto label(base::MakeUnique<views::Label>(GetDisplayUsername(form), On 2017/05/15 19:51:42, Peter Kasting wrote: > Nit: Use = over () for simple initializations like this; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > (3 places) Done, thank you for the link as well :) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: ->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS) On 2017/05/15 19:51:42, Peter Kasting wrote: > Pretty sure all of these should be INSETS_PANEL too. (5 places) I think all of these instances are in bubbles? It's this thing: https://drive.google.com/file/d/0BzEa5HU1aAqBUjdEREZGcDBZMzQ/view?usp=sharing (& variants). https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:456: layout_provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON).top()); On 2017/05/15 19:51:42, Peter Kasting wrote: > This looks like another case where I'm wondering if we can avoid rolling our own > buttons. See previous comment. https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:692: layout_provider->GetInsetsMetric(views::INSETS_DIALOG_BUTTON).top()); On 2017/05/15 19:51:42, Peter Kasting wrote: > And here See previous comment.
LGTM https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: ->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS) On 2017/05/17 07:55:27, Patti Lor wrote: > On 2017/05/15 19:51:42, Peter Kasting wrote: > > Pretty sure all of these should be INSETS_PANEL too. (5 places) > > I think all of these instances are in bubbles? It's this thing: > https://drive.google.com/file/d/0BzEa5HU1aAqBUjdEREZGcDBZMzQ/view?usp=sharing (& > variants). I'd call that a dialog because it has buttons. The key is actually the horizontal insets; the vertical ones are the same between panels and dialogs (which is why my suggestions have no visible effect), but the horizontal ones aren't. If we change them to not be the same someday, though, I think the rule is more "does it have buttons" than "is it anchored" or similar. https://codereview.chromium.org/2869683003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:184: const gfx::Insets panel_insets = Nit: Bret is renaming PANEL to DIALOG_CONTENTS in https://codereview.chromium.org/2888563004 , so I'd call this temp |dialog_insets| like you did in the previous file.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks Peter for the review :) https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2869683003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:190: ->GetInsetsMetric(views::INSETS_BUBBLE_CONTENTS) On 2017/05/17 23:53:33, Peter Kasting wrote: > On 2017/05/17 07:55:27, Patti Lor wrote: > > On 2017/05/15 19:51:42, Peter Kasting wrote: > > > Pretty sure all of these should be INSETS_PANEL too. (5 places) > > > > I think all of these instances are in bubbles? It's this thing: > > https://drive.google.com/file/d/0BzEa5HU1aAqBUjdEREZGcDBZMzQ/view?usp=sharing > (& > > variants). > > I'd call that a dialog because it has buttons. The key is actually the > horizontal insets; the vertical ones are the same between panels and dialogs > (which is why my suggestions have no visible effect), but the horizontal ones > aren't. If we change them to not be the same someday, though, I think the rule > is more "does it have buttons" than "is it anchored" or similar. Ah ok, that makes sense - will keep that in mind. Fixed! https://codereview.chromium.org/2869683003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc (right): https://codereview.chromium.org/2869683003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/auto_signin_first_run_dialog_view.cc:184: const gfx::Insets panel_insets = On 2017/05/17 23:53:34, Peter Kasting wrote: > Nit: Bret is renaming PANEL to DIALOG_CONTENTS in > https://codereview.chromium.org/2888563004 , so I'd call this temp > |dialog_insets| like you did in the previous file. Went ahead and changed this everywhere (in this patch), thanks!
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2869683003/#ps80001 (title: "Review comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495094046183960,
"parent_rev": "9a13b0fc00635fddc8fd1e073f267b37648e3b10", "commit_rev":
"f195c43cff20208422063e0d9179b4a29415c28d"}
Message was sent while issue was closed.
Description was changed from ========== Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/views/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 ========== to ========== Views/Harmony: Remove references to layout constants in c/b/u/v/passwords. Remove unnecessary includes for ui/views/layout/layout_constants.h and do some refactoring to remove references to layout constants in chrome/browser/ui/views/passwords/*. This CL also improves the TestBrowserDialog tests for PasswordDialogViewTest and ManagePasswordsBubbleDialogViewTest. BUG=691897 Review-Url: https://codereview.chromium.org/2869683003 Cr-Commit-Position: refs/heads/master@{#472733} Committed: https://chromium.googlesource.com/chromium/src/+/f195c43cff20208422063e0d9179... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f195c43cff20208422063e0d9179... |
