Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821413002:
views: support dialog width snapping once and for all (Closed)
|
Created:
3 years, 8 months ago by Elly Fong-Jones Modified:
3 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, msw+watch_chromium.org, tfarina, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, chromium-apps-reviews_chromium.org, rouslan+bubble_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews: support dialog width snapping once and for all
This change:
1) Adds LayoutProvider::GetSnappedDialogWidth() to implement
the snapping;
2) Adds a unit test BubbleFrameViewTest.WidthSnaps to
ensure snapping actually works.
BUG=635173
Review-Url: https://codereview.chromium.org/2821413002
Cr-Commit-Position: refs/heads/master@{#467031}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9d455e92cf41f171d31093cee055e41d634ea9
Patch Set 1 #
Total comments: 43
Patch Set 2 : first round of fixes #
Total comments: 2
Patch Set 3 : DialogDelegateView -> BubbleDialogDelegateView #
Total comments: 2
Patch Set 4 : remove DialogWidth #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : fix failing unittest #Messages
Total messages: 35 (18 generated)
Description was changed from ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds DialogDelegateView::ShouldSnapFrameWidth() to let individual dialogs opt out; 3) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 ========== to ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds DialogDelegateView::ShouldSnapFrameWidth() to let individual dialogs opt out; 3) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 ==========
ellyjones@chromium.org changed reviewers: + pkasting@chromium.org, tapted@chromium.org
pkasting, tapted: ptal? :)
How come you didn't upload this to https://codereview.chromium.org/2750063002/ ? https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right): https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:87: int ChromeLayoutProvider::GetSnappedDialogWidth(int min_width) const { Nit: Definition order must match declaration order (see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... ) https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:88: return min_width; Nit: This is the same as the base class implementation; avoid overriding here as it's unnecessary at best and a maintenance risk at worst https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.h:69: const views::TypographyProvider& GetTypographyProvider() const override; Nit: Let's just kill the blank lines between these override declarations (would be nice to offset them with "// views::LayoutProvider:" as well) https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:539: if (dialog_delegate && dialog_delegate->ShouldSnapFrameWidth()) { Nit: No {} https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:536: // LayoutProvider. Nit: I feel like even talking about HarmonyLayoutProvider here is a bit of a misnomer, and besides, the test layout provider here gives you more control over this. I'd just remove this comment. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:540: new TestBubbleDialogDelegateView()); Nit: Prefer = base::MakeUnique to bare new https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:548: // This is a bit awkward: |anchor| needs to outlive |delegate|, but |delegate| I'm confused. If |anchor| needs to outlive |delegate|, this code won't do it correctly -- you created |anchor| last, so it will be destroyed first. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:555: const int kTestWidth = 300; Nit: Prefer constexpr for compile-time constants https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:559: EXPECT_NE(w0->GetWindowBoundsInScreen().width(), kTestWidth); Nit: (expected, actual) (2 places) https://codereview.chromium.org/2821413002/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.h:93: virtual bool ShouldSnapFrameWidth() const; Do we have a specific dialog that we're going to set this false for? If so, let's add that in this CL so it's motivated. If not, I wonder if we should just omit this entirely for now and add it later if we need it.
https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:502: void SetAnchorView(View* anchor_view) { `using BubbleDialogDelegateView::SetAnchorView;` (that's a clearer signal that we just want to expose a protected member from the base class) https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:507: // itself here. nit: here, // BubbleDialogDelegateView: I'd put the comment above into the method body https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:509: We should override GetDialogButtons() as well to return ui::DIALOG_BUTTON_NONE (otherwise the width of buttons and default strings that are put there also have an influence on the dialog size). https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:519: int GetSnappedDialogWidth(int min_width) const override { nit: // LayoutProvider: https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:533: TEST_F(BubbleFrameViewTest, WidthSnaps) { nit: comment describing the test purpose https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:540: new TestBubbleDialogDelegateView()); could this just go on the stack? TestBubbleDialogDelegateView delegate; Then pass &delegate below https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:542: std::unique_ptr<Widget> anchor(new Widget()); this can probably go on the stack too https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:544: params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; nit: `views::` not required https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:553: delegate->SetAnchorView(anchor->GetContentsView()); Why do we need an anchor view? can we just leave it null? Or call SetAnchorRect(..) instead? https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:559: EXPECT_NE(w0->GetWindowBoundsInScreen().width(), kTestWidth); Can we add a TestBubbleDialogDelegateView::GetPreferredSize() override (e.g. 200,200) and test against that size explicitly? delegate.set_margins(gfx::Insets()); should simplify the width calculations, although it still depends on button widths without GetDialogButtons() overridden https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:568: } w0->CloseNow(); w1->CloseNow(); (otherwise.. I think you currently have a UAF during the Widget destruction done when the root window is torn down and Widget wants to call delegate.DeleteDelegate()) which would be deleted by the unique_ptr here)
pkasting: I didn't upload this as part of my previous CL because I forgot to 'git cl issue ...' before uploading it :\ mistake on my part, basically. tapted, pkasting: ptal? :) https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right): https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:87: int ChromeLayoutProvider::GetSnappedDialogWidth(int min_width) const { On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: Definition order must match declaration order (see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > ) Acknowledged. https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.cc:88: return min_width; On 2017/04/19 19:26:12, Peter Kasting (OOO until 4-24) wrote: > Nit: This is the same as the base class implementation; avoid overriding here as > it's unnecessary at best and a maintenance risk at worst Ooh, good point - deleted :) https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2821413002/diff/1/chrome/browser/ui/views/har... chrome/browser/ui/views/harmony/chrome_layout_provider.h:69: const views::TypographyProvider& GetTypographyProvider() const override; On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: Let's just kill the blank lines between these override declarations (would > be nice to offset them with "// views::LayoutProvider:" as well) Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:539: if (dialog_delegate && dialog_delegate->ShouldSnapFrameWidth()) { On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: No {} Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:502: void SetAnchorView(View* anchor_view) { On 2017/04/20 01:12:56, tapted wrote: > `using BubbleDialogDelegateView::SetAnchorView;` > > (that's a clearer signal that we just want to expose a protected member from the > base class) Ooh, I didn't even know you could do this! TIL :) https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:507: // itself here. On 2017/04/20 01:12:56, tapted wrote: > nit: here, // BubbleDialogDelegateView: > > I'd put the comment above into the method body Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:509: On 2017/04/20 01:12:56, tapted wrote: > We should override GetDialogButtons() as well to return ui::DIALOG_BUTTON_NONE > (otherwise the width of buttons and default strings that are put there also have > an influence on the dialog size). Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:519: int GetSnappedDialogWidth(int min_width) const override { On 2017/04/20 01:12:56, tapted wrote: > nit: // LayoutProvider: Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:533: TEST_F(BubbleFrameViewTest, WidthSnaps) { On 2017/04/20 01:12:56, tapted wrote: > nit: comment describing the test purpose Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:536: // LayoutProvider. On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: I feel like even talking about HarmonyLayoutProvider here is a bit of a > misnomer, and besides, the test layout provider here gives you more control over > this. I'd just remove this comment. Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:540: new TestBubbleDialogDelegateView()); On 2017/04/20 01:12:56, tapted wrote: > could this just go on the stack? > > TestBubbleDialogDelegateView delegate; > > Then pass &delegate below Ooh, it can. Done :) https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:540: new TestBubbleDialogDelegateView()); On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: Prefer = base::MakeUnique to bare new Acknowledged. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:542: std::unique_ptr<Widget> anchor(new Widget()); On 2017/04/20 01:12:56, tapted wrote: > this can probably go on the stack too Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:544: params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; On 2017/04/20 01:12:56, tapted wrote: > nit: `views::` not required Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:548: // This is a bit awkward: |anchor| needs to outlive |delegate|, but |delegate| On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > I'm confused. If |anchor| needs to outlive |delegate|, this code won't do it > correctly -- you created |anchor| last, so it will be destroyed first. After more thought, this entire comment was nonsense, so I've removed it. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:553: delegate->SetAnchorView(anchor->GetContentsView()); On 2017/04/20 01:12:56, tapted wrote: > Why do we need an anchor view? can we just leave it null? Or call > SetAnchorRect(..) instead? I'm surprised that it works without an anchor view O_o. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:555: const int kTestWidth = 300; On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: Prefer constexpr for compile-time constants Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:559: EXPECT_NE(w0->GetWindowBoundsInScreen().width(), kTestWidth); On 2017/04/20 01:12:56, tapted wrote: > Can we add a TestBubbleDialogDelegateView::GetPreferredSize() override (e.g. > 200,200) and test against that size explicitly? > > delegate.set_margins(gfx::Insets()); > > should simplify the width calculations, although it still depends on button > widths without GetDialogButtons() overridden nice :D Done. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:559: EXPECT_NE(w0->GetWindowBoundsInScreen().width(), kTestWidth); On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Nit: (expected, actual) (2 places) Acknowledged. https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:568: } On 2017/04/20 01:12:56, tapted wrote: > w0->CloseNow(); > w1->CloseNow(); > > (otherwise.. I think you currently have a UAF during the Widget destruction done > when the root window is torn down and Widget wants to call > delegate.DeleteDelegate()) which would be deleted by the unique_ptr here) Does this also serve to delete w0 and w1? I can't tell if I'm leaking them right now. https://codereview.chromium.org/2821413002/diff/1/ui/views/window/dialog_dele... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/window/dialog_dele... ui/views/window/dialog_delegate.h:93: virtual bool ShouldSnapFrameWidth() const; On 2017/04/19 19:26:13, Peter Kasting (OOO until 4-24) wrote: > Do we have a specific dialog that we're going to set this false for? If so, > let's add that in this CL so it's motivated. If not, I wonder if we should just > omit this entirely for now and add it later if we need it. Done.
lgtm with an updated CL description (still mentions GetSnappedDialogWidth()) https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:568: } On 2017/04/20 19:53:59, Elly Fong-Jones wrote: > On 2017/04/20 01:12:56, tapted wrote: > > w0->CloseNow(); > > w1->CloseNow(); > > > > (otherwise.. I think you currently have a UAF during the Widget destruction > done > > when the root window is torn down and Widget wants to call > > delegate.DeleteDelegate()) which would be deleted by the unique_ptr here) > > Does this also serve to delete w0 and w1? I can't tell if I'm leaking them right > now. Yup. Widget destruction triggers a CloseNow() when WIDGET_OWNS_NATIVE_WIDGET; CreateBubble() doesn't make widgets in this mode, but instead NATIVE_WIDGET_OWNS_WIDGET, so a CloseNow() on the Widget will synchronously delete the Widget* instead. Note I think DCHECKS (or trybots) or `is_asan = true` should complain about the earlier code, but a regular release build might not. E.g. (on Mac) there's a DCHECK at https://cs.chromium.org/chromium/src/ui/views/test/views_test_helper_mac.mm?q... that should get hit. Maybe I should make that a CHECK... https://codereview.chromium.org/2821413002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:503: // DialogDelegateView: // BubbleDialogDelegateView: peter has a preference to name the "visible" base class on the list of parents on the class declaration. (I used to go for "ancestor where these are declared" since that's where the "missing" documentation should be - which would be 3 blocks here: WidgetDelegate/DialogDelegate/View). But DialogDelegateView is none of these :)
Description was changed from ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds DialogDelegateView::ShouldSnapFrameWidth() to let individual dialogs opt out; 3) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 ========== to ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 ==========
description updated :) https://codereview.chromium.org/2821413002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:503: // DialogDelegateView: On 2017/04/21 00:20:31, tapted (OOO soon) wrote: > // BubbleDialogDelegateView: > > peter has a preference to name the "visible" base class on the list of parents > on the class declaration. (I used to go for "ancestor where these are declared" > since that's where the "missing" documentation should be - which would be 3 > blocks here: WidgetDelegate/DialogDelegate/View). But DialogDelegateView is none > of these :) Okay, changed to BubbleDialogDelegateView :)
kylixrd@chromium.org changed reviewers: + kylixrd@chromium.org
Please excuse the drive-by :)... https://codereview.chromium.org/2821413002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2821413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/chrome_layout_provider.h:53: enum class DialogWidth { This can now be removed since GetDialogPreferredWidth() is also gone.
kylixrd: thanks :) https://codereview.chromium.org/2821413002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right): https://codereview.chromium.org/2821413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/chrome_layout_provider.h:53: enum class DialogWidth { On 2017/04/21 17:45:24, kylix_rd wrote: > This can now be removed since GetDialogPreferredWidth() is also gone. Well-spotted, thank you :) removed.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2821413002/#ps60001 (title: "remove DialogWidth")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2821413002/#ps80001 (title: "rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/04/24 16:25:05, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) pkasting: ptal as c/b/ui/views owner? :)
LGTM, nice https://codereview.chromium.org/2821413002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:545: constexpr int kTestWidth = 300; Nit: Declare as close as possible to first use (see http://google.github.io/styleguide/cppguide.html#Local_Variables )
https://codereview.chromium.org/2821413002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2821413002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:545: constexpr int kTestWidth = 300; On 2017/04/24 23:06:31, Peter Kasting wrote: > Nit: Declare as close as possible to first use (see > http://google.github.io/styleguide/cppguide.html#Local_Variables ) Done.
The CQ bit was checked by ellyjones@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: This issue passed the CQ dry run.
The CQ bit was checked by ellyjones@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/2821413002/#ps100001 (title: "fix failing unittest")
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": 100001, "attempt_start_ts": 1493140462738140,
"parent_rev": "aeb180b1bc8066c2095c124d55e744be0124f020", "commit_rev":
"ce9d455e92cf41f171d31093cee055e41d634ea9"}
Message was sent while issue was closed.
Description was changed from ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 ========== to ========== views: support dialog width snapping once and for all This change: 1) Adds LayoutProvider::GetSnappedDialogWidth() to implement the snapping; 2) Adds a unit test BubbleFrameViewTest.WidthSnaps to ensure snapping actually works. BUG=635173 Review-Url: https://codereview.chromium.org/2821413002 Cr-Commit-Position: refs/heads/master@{#467031} Committed: https://chromium.googlesource.com/chromium/src/+/ce9d455e92cf41f171d31093cee0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ce9d455e92cf41f171d31093cee0... |
