Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660553005:
Harmony - convert hung renderer dialog. (Closed)
DescriptionHarmony - convert hung renderer dialog.
Also added the dialog to BrowserDialogTest. Also did some refactoring on
DialogClientView to avoid hardcoding the minimum button width and to apply
the button row insets more coherently.
BUG=652507
Review-Url: https://codereview.chromium.org/2660553005
Cr-Commit-Position: refs/heads/master@{#451497}
Committed: https://chromium.googlesource.com/chromium/src/+/b621d8cec36bec73ef3eacddbcd7c998aee80cae
Patch Set 1 #Patch Set 2 : wrong constant #Patch Set 3 : remove extra padding above buttons #Patch Set 4 : add hung renderer to dialog tests, WIP #Patch Set 5 : add workaround note #Patch Set 6 : remove test friends #
Total comments: 20
Patch Set 7 : comments 1 #
Total comments: 10
Patch Set 8 : merge #
Total comments: 9
Patch Set 9 : comments 2 #
Total comments: 17
Patch Set 10 : comments 3 #
Total comments: 20
Patch Set 11 : fix merge and imports #Patch Set 12 : comments 4 #
Total comments: 7
Patch Set 13 : adjust comment #
Total comments: 2
Patch Set 14 : change dialog_client_view layout #Patch Set 15 : disable dialog test on osx #Messages
Total messages: 51 (15 generated)
Description was changed from ========== Harmony - convert hung renderer dialog. BUG=652507 ========== to ========== Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also fixed an issue where BrowserDialogTest would not work on Windows without passing --single-process. BUG=652507 ==========
bsep@chromium.org changed reviewers: + msw@chromium.org, tapted@chromium.org
bsep@chromium.org changed reviewers: + pkasting@chromium.org
bsep@chromium.org changed reviewers: - msw@chromium.org
pkasting@: general review tapted@: DialogBrowserTest review https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:70: options.wait = true; This is the fix for having to pass in --single-process on Windows. https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:85: if (ViewsDelegate::GetInstance()) This is redundant with the first line of the function.
https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:70: options.wait = true; On 2017/02/01 00:05:30, Bret Sepulveda wrote: > This is the fix for having to pass in --single-process on Windows. Can you comment here? I don't know too much about how processes spawn on Windows, but I guess it's something like // Windows refuses to spawn the child process if the invoking process goes away too quickly, so always wait. This means the invoker will bring down the interactive process when it is killed after a timeout. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:23: // TestDialogInterface: // DialogBrowserTest: (sorry - I'll update that doc where this was taken from) https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:31: views::Widget* widget_; unused? https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:43: void SetUpCommandLine(base::CommandLine* command_line) override { I think this can be merged into the above test harness (unless we want another test case for non-MD, but I'd probably skip that). (the approach here is useful when there's an existing browser_test harness that's being adapted, but that's not the case for this one) https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:51: IN_PROC_BROWSER_TEST_F(HungRendererDialogViewBrowserTestMd, nit: add a comment (I try to ensure every TEST_F has a comment above it describing why the test exists). For these, I've been doing things like // Calls ShowDialog("default"). Interactive when run via browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=HungRendererDialogViewBrowserTest.InvokeDialog_default. https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:353: ? ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth() I feel like this isn't the best place to bake in the minimum width.. (i.e. it's easy for custom dialogs under src/chrome to "forget" about this) E.g. there is `kMinWidth = 48` in md_text_button.cc, which is used when MdTextButton::CreateSecondaryUIButton detects harmony and uses an MdTextButton* rather than a BlueButton* or LabelButton* Are there use cases where we have an MdTextButton* that is not a "dialog" button? (can we just override the default min width there?)
https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:70: options.wait = true; On 2017/02/01 00:52:48, tapted wrote: > On 2017/02/01 00:05:30, Bret Sepulveda wrote: > > This is the fix for having to pass in --single-process on Windows. > > Can you comment here? > > I don't know too much about how processes spawn on Windows, but I guess it's > something like > > // Windows refuses to spawn the child process if the invoking process goes away > too quickly, so always wait. This means the invoker will bring down the > interactive process when it is killed after a timeout. I actually don't know why this is necessary either, I figured it out on accident. Maybe Peter knows? Added a more generic comment in the meantime. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/02/01 00:52:48, tapted wrote: > nit: 2017 Done. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:23: // TestDialogInterface: On 2017/02/01 00:52:48, tapted wrote: > // DialogBrowserTest: > > (sorry - I'll update that doc where this was taken from) Done. I took it from the collected cookies version actually; updated there too. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:31: views::Widget* widget_; On 2017/02/01 00:52:48, tapted wrote: > unused? Oops yeah. Removed. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:43: void SetUpCommandLine(base::CommandLine* command_line) override { On 2017/02/01 00:52:48, tapted wrote: > I think this can be merged into the above test harness (unless we want another > test case for non-MD, but I'd probably skip that). (the approach here is useful > when there's an existing browser_test harness that's being adapted, but that's > not the case for this one) Done. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:51: IN_PROC_BROWSER_TEST_F(HungRendererDialogViewBrowserTestMd, On 2017/02/01 00:52:48, tapted wrote: > nit: add a comment (I try to ensure every TEST_F has a comment above it > describing why the test exists). For these, I've been doing things like > > // Calls ShowDialog("default"). Interactive when run via browser_tests > --gtest_filter=BrowserDialogTest.Invoke --interactive > --dialog=HungRendererDialogViewBrowserTest.InvokeDialog_default. I don't like the idea of baking the invocation into the comment. How's this? https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:353: ? ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth() On 2017/02/01 00:52:48, tapted wrote: > I feel like this isn't the best place to bake in the minimum width.. (i.e. it's > easy for custom dialogs under src/chrome to "forget" about this) E.g. there is > `kMinWidth = 48` in md_text_button.cc, which is used when > MdTextButton::CreateSecondaryUIButton detects harmony and uses an MdTextButton* > rather than a BlueButton* or LabelButton* > > Are there use cases where we have an MdTextButton* that is not a "dialog" > button? (can we just override the default min width there?) Oh you're right, that's odd. I was told to make the minimum width 70 (in crbug.com/652507#c4) so I'm not sure what the right answer is. Maybe Peter can shed some light? If not I'll have to ask a designer. But either way you're right it should probably go into md_text_button. I'll update and retest this when I have the right value.
Did not do a complete review, more some quick looks + responses to questions. https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:70: options.wait = true; On 2017/02/01 02:21:46, Bret Sepulveda wrote: > On 2017/02/01 00:52:48, tapted wrote: > > On 2017/02/01 00:05:30, Bret Sepulveda wrote: > > > This is the fix for having to pass in --single-process on Windows. > > > > Can you comment here? > > > > I don't know too much about how processes spawn on Windows, but I guess it's > > something like > > > > // Windows refuses to spawn the child process if the invoking process goes > away > > too quickly, so always wait. This means the invoker will bring down the > > interactive process when it is killed after a timeout. > > I actually don't know why this is necessary either, I figured it out on > accident. Maybe Peter knows? Added a more generic comment in the meantime. I don't know either, but it would be good to find out why, in case it indicates some more subtle bug that we need to fix elsewhere. Seems like I just saw someone get a refactoring award for MessageLoop stuff, I wonder if they would know about this? Otherwise, if you can turn this into a more detailed technical question, you could try posting on chrome-windows. https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:353: ? ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth() On 2017/02/01 02:21:46, Bret Sepulveda wrote: > On 2017/02/01 00:52:48, tapted wrote: > > I feel like this isn't the best place to bake in the minimum width.. (i.e. > it's > > easy for custom dialogs under src/chrome to "forget" about this) E.g. there is > > `kMinWidth = 48` in md_text_button.cc, which is used when > > MdTextButton::CreateSecondaryUIButton detects harmony and uses an > MdTextButton* > > rather than a BlueButton* or LabelButton* > > > > Are there use cases where we have an MdTextButton* that is not a "dialog" > > button? (can we just override the default min width there?) > > Oh you're right, that's odd. I was told to make the minimum width 70 (in > crbug.com/652507#c4) so I'm not sure what the right answer is. Maybe Peter can > shed some light? If not I'll have to ask a designer. But either way you're right > it should probably go into md_text_button. I'll update and retest this when I > have the right value. The spec I have ( https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... ) says buttons are minimum 64 DIP wide (16 DIP "boundary" on each end, and 32 DIP minimum for text), but can be wider if the text is wider than 32 DIP. I'm not sure where Jayson got the 70 value. I don't have a specific mock for this dialog in the tracking spreadsheet (the mock linked in comment 0 of the bug is out of date). Separately, note that the button set here probably is affected by https://bugs.chromium.org/p/chromium/issues/detail?id=671820 , but I imagine that will get fixed separately. The 48 value in md_text_button.cc comes from https://chromium.googlesource.com/chromium/src/+/0faeac167d0af1df458fd5edf988... , which was for https://bugs.chromium.org/p/chromium/issues/detail?id=571500 , which was working off the spec in comment 0 of that bug, which is from a very early version of secondary UI MD. I think that should be replaced with the 64 value here (and probably not hardcoded into that file). Maybe the ViewsDelegate method should be more like "GetLabelButtonMinimumWidth()" instead of "GetDialogButtonMinimumWidth()" and be applied inside all LabelButtons? https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:553: LayoutDelegate::LayoutDistanceType::RELATED_BUTTON_HORIZONTAL_SPACING); I think these two functions' changes should be landed already as of last night. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:26: UNRELATED_CONTROL_LARGE_HORIZONTAL_SPACING, Once you resync with upstream, you'll want to rename this to follow the pattern I did with the LARGE_VERTICAL_SPACING (i.e. UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE) and ensure it's sorted in alphabetical order. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:347: // The kill button doesn't look like a normal dialog button pre-harmony. Oh? It looks like one in my build. Say more? What would applying this min size unconditionally do? https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:356: // respond to accelerator keys, so it remains an extra view. Why doesn't it respond to accelerator keys? Are we worried about accidentally killing something? This behavior sounds questionable. https://codereview.chromium.org/2660553005/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:180: buttons_row_height += GetButtonsAndExtraViewRowTopPadding(); Shouldn't this be implemented by returning 0 from GetButtonsAndExtraViewRowTopPadding() instead?
https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... File chrome/browser/ui/test/browser_dialog_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/100001/chrome/browser/ui/test... chrome/browser/ui/test/browser_dialog_browsertest.cc:70: options.wait = true; On 2017/02/02 00:32:30, Peter Kasting wrote: > On 2017/02/01 02:21:46, Bret Sepulveda wrote: > > On 2017/02/01 00:52:48, tapted wrote: > > > On 2017/02/01 00:05:30, Bret Sepulveda wrote: > > > > This is the fix for having to pass in --single-process on Windows. > > > > > > Can you comment here? > > > > > > I don't know too much about how processes spawn on Windows, but I guess it's > > > something like > > > > > > // Windows refuses to spawn the child process if the invoking process goes > > away > > > too quickly, so always wait. This means the invoker will bring down the > > > interactive process when it is killed after a timeout. > > > > I actually don't know why this is necessary either, I figured it out on > > accident. Maybe Peter knows? Added a more generic comment in the meantime. > > I don't know either, but it would be good to find out why, in case it indicates > some more subtle bug that we need to fix elsewhere. > > Seems like I just saw someone get a refactoring award for MessageLoop stuff, I > wonder if they would know about this? Otherwise, if you can turn this into a > more detailed technical question, you could try posting on chrome-windows. This is way outside of my wheelhouse, so I'd rather investigate this separately from this patch. I can revert this change for this patch too if you want but I figured it was better to put in the workaround right away. https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:353: ? ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth() On 2017/02/02 00:32:30, Peter Kasting wrote: > On 2017/02/01 02:21:46, Bret Sepulveda wrote: > > On 2017/02/01 00:52:48, tapted wrote: > > > I feel like this isn't the best place to bake in the minimum width.. (i.e. > > it's > > > easy for custom dialogs under src/chrome to "forget" about this) E.g. there > is > > > `kMinWidth = 48` in md_text_button.cc, which is used when > > > MdTextButton::CreateSecondaryUIButton detects harmony and uses an > > MdTextButton* > > > rather than a BlueButton* or LabelButton* > > > > > > Are there use cases where we have an MdTextButton* that is not a "dialog" > > > button? (can we just override the default min width there?) > > > > Oh you're right, that's odd. I was told to make the minimum width 70 (in > > crbug.com/652507#c4) so I'm not sure what the right answer is. Maybe Peter can > > shed some light? If not I'll have to ask a designer. But either way you're > right > > it should probably go into md_text_button. I'll update and retest this when I > > have the right value. > > The spec I have ( > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(... > ) says buttons are minimum 64 DIP wide (16 DIP "boundary" on each end, and 32 > DIP minimum for text), but can be wider if the text is wider than 32 DIP. > > I'm not sure where Jayson got the 70 value. I don't have a specific mock for > this dialog in the tracking spreadsheet (the mock linked in comment 0 of the bug > is out of date). Separately, note that the button set here probably is affected > by https://bugs.chromium.org/p/chromium/issues/detail?id=671820 , but I imagine > that will get fixed separately. > > The 48 value in md_text_button.cc comes from > https://chromium.googlesource.com/chromium/src/+/0faeac167d0af1df458fd5edf988... > , which was for https://bugs.chromium.org/p/chromium/issues/detail?id=571500 , > which was working off the spec in comment 0 of that bug, which is from a very > early version of secondary UI MD. I think that should be replaced with the 64 > value here (and probably not hardcoded into that file). > > Maybe the ViewsDelegate method should be more like > "GetLabelButtonMinimumWidth()" instead of "GetDialogButtonMinimumWidth()" and be > applied inside all LabelButtons? I think I understand. I changed the minimum width to 64 and the wait button is still 70px wide, so the string "Wait" is just long enough to be above minimum. I guess Jayson was saying that the buttons should be the same size in that case. Since that logic is in progress elsewhere I just changed the minimum to 64px in md_text_button. I left it hardcoded because it's not actually needed anywhere else yet; I'd rather wait for the plumbing to become necessary to refactor. That also means the GetDialogButtonMinimumWidth method I added isn't necessary any more either. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:553: LayoutDelegate::LayoutDistanceType::RELATED_BUTTON_HORIZONTAL_SPACING); On 2017/02/02 00:32:30, Peter Kasting wrote: > I think these two functions' changes should be landed already as of last night. Done, all merged. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:26: UNRELATED_CONTROL_LARGE_HORIZONTAL_SPACING, On 2017/02/02 00:32:30, Peter Kasting wrote: > Once you resync with upstream, you'll want to rename this to follow the pattern > I did with the LARGE_VERTICAL_SPACING (i.e. > UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE) and ensure it's sorted in > alphabetical order. Done. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:347: // The kill button doesn't look like a normal dialog button pre-harmony. On 2017/02/02 00:32:30, Peter Kasting wrote: > Oh? It looks like one in my build. Say more? What would applying this min > size unconditionally do? I meant that it's an extra view, off to the left, and Jayson wanted it on the right, as if it were the cancel button. So I'm trying to preserve the original look for pre-harmony. I figured it was okay if pre-harmony stuff was a little weird because it will go away eventually. https://codereview.chromium.org/2660553005/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:356: // respond to accelerator keys, so it remains an extra view. On 2017/02/02 00:32:30, Peter Kasting wrote: > Why doesn't it respond to accelerator keys? Are we worried about accidentally > killing something? This behavior sounds questionable. I dunno, that's the only reason I can think of that it's an extra dialog, other than maybe someone REALLY wanted it to appear on the left. I actually think it would kinda be weird if you pressed ESC and it killed the page, instead of waiting. But I don't have a strong opinion. https://codereview.chromium.org/2660553005/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:180: buttons_row_height += GetButtonsAndExtraViewRowTopPadding(); On 2017/02/02 00:32:30, Peter Kasting wrote: > Shouldn't this be implemented by returning 0 from > GetButtonsAndExtraViewRowTopPadding() instead? Done.
lgtm % nits. you'll need pkasting too https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/coll... File chrome/browser/ui/collected_cookies_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/coll... chrome/browser/ui/collected_cookies_browsertest.cc:25: // DialogBrowserTest: Thanks! I had this fix in a patch that got reverted :/ still got one more yak to shave before I can reland that.. https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:25: #include "chrome/browser/ui/views/harmony/layout_delegate.h" remove? https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: remove (c) https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:18: // pass --disable-gpu as a workaround. perhaps merge in the stuff from https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... (with the comment applied). Currently you will conflict there anyway because of the .wait thing https://codereview.chromium.org/2660553005/diff/140001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/140001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:224: LayoutButton(extra_view_, &row_bounds, button_height); I think |extra_view_| is sometimes not a button -- e.g. Add to Desktop. I guess this is OK so long as GroupExtraViewWithButtons() is only overridden to return true when the extra view is actually a button. (and perhaps it is OK even if it isn't a button). So this looks fine.
https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate.cc:25: #include "chrome/browser/ui/views/harmony/layout_delegate.h" On 2017/02/03 02:07:54, tapted wrote: > remove? Done. https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/03 02:07:54, tapted wrote: > nit: remove (c) Done. https://codereview.chromium.org/2660553005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:18: // pass --disable-gpu as a workaround. On 2017/02/03 02:07:54, tapted wrote: > perhaps merge in the stuff from > https://codereview.chromium.org/2668833003/diff/100001/chrome/browser/ui/test... > (with the comment applied). Currently you will conflict there anyway because of > the .wait thing Okay I coordinated with Allen and pulled in his workaround too, so I deleted this comment. https://codereview.chromium.org/2660553005/diff/140001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/140001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:224: LayoutButton(extra_view_, &row_bounds, button_height); On 2017/02/03 02:07:54, tapted wrote: > I think |extra_view_| is sometimes not a button -- e.g. Add to Desktop. I guess > this is OK so long as GroupExtraViewWithButtons() is only overridden to return > true when the extra view is actually a button. (and perhaps it is OK even if it > isn't a button). So this looks fine. Right, LayoutButton is agnostic to whether the view is an actual button. I considered renaming it, but I figured it would be really odd for someone to want to group a view that wasn't a button, so I left it.
lgtm - I think patchset 9 is good to land, but you need some OWNERS cookies. (also note this won't bring up the dialog test on Mac yet -- I need to move views/hung_renderer_view.cc into the mac build first -- but I don't see any thing in the dialog test that will it difficult to move that at the same time).
https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:352: return LayoutDelegate::Get()->IsHarmonyMode(); After the previous discussion, I feel like we should just avoid adding this override, and move this over to be with the other buttons as a Real Dialog Button, and be OK with changing that pre-Harmony as well. https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:24: browser()->tab_strip_model()->GetActiveWebContents(); Nit: Can fit on one line (and be just as clear) with auto: auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:31: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); Do we need/want to force this here? Seems OK to just have the user pass this if they want? https://codereview.chromium.org/2660553005/diff/160001/ui/views/controls/butt... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2660553005/diff/160001/ui/views/controls/butt... ui/views/controls/button/md_text_button.cc:30: const int kMinWidth = 64; Nit: I think this should be 4 * HarmonyLayoutDelegate::kHarmonyLayoutUnit. https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:86: button_row_insets_ = ViewsDelegate::GetInstance()->GetDialogButtonInsets(); It's not obvious to me why removing this is correct. https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:350: button->SetMinSize(gfx::Size(kDialogMinimumButtonWidth, 0)); I think this should be plumbed off the ViewsDelegate/LayoutDelegate (and should return 0 in Harmony, in which case this code should just not call SetMinSize()), rather than be explicitly conditional on IsSecondaryUiMaterial(). https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:377: return 0; Similarly, maybe this should be plumbed through ViewsDelegate/LayoutDelegate as something like GetDefaultDialogButtonRowTopPadding(). https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:386: spacing = ViewsDelegate::GetInstance() (Not clear to me if this can fail here. I sent an email to sky about this.)
https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:86: button_row_insets_ = ViewsDelegate::GetInstance()->GetDialogButtonInsets(); On 2017/02/06 21:44:51, Peter Kasting wrote: > It's not obvious to me why removing this is correct. Oh nm, I see it's duplicating stuff just above. Scott says we are clear to remove ViewsDelegate null-checks, so I'd rip out both this block and the one above and then just init this directly in the initializer list.
https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:352: return LayoutDelegate::Get()->IsHarmonyMode(); On 2017/02/06 21:44:50, Peter Kasting wrote: > After the previous discussion, I feel like we should just avoid adding this > override, and move this over to be with the other buttons as a Real Dialog > Button, and be OK with changing that pre-Harmony as well. Done. I made the Wait button OK and the Kill button Cancel, then overrode Close to call Accept so that you still have to explicitly click the Kill button to kill the renderer and any keypresses will choose "Wait" instead. I didn't realize this causes the buttons to be in the correct OS-dependent ok/cancel order, so that's a cool side effect. https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:24: browser()->tab_strip_model()->GetActiveWebContents(); On 2017/02/06 21:44:51, Peter Kasting wrote: > Nit: Can fit on one line (and be just as clear) with auto: > auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); Done. https://codereview.chromium.org/2660553005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:31: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); On 2017/02/06 21:44:51, Peter Kasting wrote: > Do we need/want to force this here? Seems OK to just have the user pass this if > they want? I felt like people who are using this suite don't care about the old version. But I guess it's better to just let the user decide with their switches, so this is removed. https://codereview.chromium.org/2660553005/diff/160001/ui/views/controls/butt... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2660553005/diff/160001/ui/views/controls/butt... ui/views/controls/button/md_text_button.cc:30: const int kMinWidth = 64; On 2017/02/06 21:44:51, Peter Kasting wrote: > Nit: I think this should be 4 * HarmonyLayoutDelegate::kHarmonyLayoutUnit. Since I'm adding this to LayoutDelegate anyway I just used that instead (via ViewsDelegate). https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:86: button_row_insets_ = ViewsDelegate::GetInstance()->GetDialogButtonInsets(); On 2017/02/07 02:37:15, Peter Kasting wrote: > On 2017/02/06 21:44:51, Peter Kasting wrote: > > It's not obvious to me why removing this is correct. > > Oh nm, I see it's duplicating stuff just above. > > Scott says we are clear to remove ViewsDelegate null-checks, so I'd rip out both > this block and the one above and then just init this directly in the initializer > list. Thank god. Removed everywhere in this class. https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:350: button->SetMinSize(gfx::Size(kDialogMinimumButtonWidth, 0)); On 2017/02/06 21:44:51, Peter Kasting wrote: > I think this should be plumbed off the ViewsDelegate/LayoutDelegate (and should > return 0 in Harmony, in which case this code should just not call SetMinSize()), > rather than be explicitly conditional on IsSecondaryUiMaterial(). Done. I also put in a TODO to delete this along with pre-harmony, because otherwise it's not clear it's redundant. https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:377: return 0; On 2017/02/06 21:44:51, Peter Kasting (OOO Feb. 8-9) wrote: > Similarly, maybe this should be plumbed through ViewsDelegate/LayoutDelegate as > something like GetDefaultDialogButtonRowTopPadding(). This function didn't really make sense to me so I just rolled the special logic into GetButtonRowInsets above and then deleted it. I think this might change some pre-harmony layouts slightly, but we seem less concerned with that lately; I suppose it's all going away soon anyway. https://codereview.chromium.org/2660553005/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:386: spacing = ViewsDelegate::GetInstance() On 2017/02/06 21:44:51, Peter Kasting wrote: > (Not clear to me if this can fail here. I sent an email to sky about this.) I assume this was written before the above comment.
yay - nice update - still lgtm https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:13: #include "chrome/browser/ui/views/hung_renderer_view.h" nit: this one should move this all the way to the top per go/cppguide#Names_and_Order_of_Includes
Still waiting for review from pkasting@ https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view_browsertest.cc (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view_browsertest.cc:13: #include "chrome/browser/ui/views/hung_renderer_view.h" On 2017/02/10 05:37:03, tapted wrote: > nit: this one should move this all the way to the top per > go/cppguide#Names_and_Order_of_Includes Done.
On 2017/02/11 23:43:16, Bret Sepulveda wrote: > Still waiting for review from pkasting@ I know. I was gone the whole middle of the week, and yesterday I managed to get through something like 9/13 code reviews. I didn't make it to this one.
The DialogClientView changes are nice, yet scary. Otherwise the main question left is around how we implement button sizing. I could be argued into punting that question further down the road if need be. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:20: // pre-Harmony, defies easy explanation. Nit: This comment might be simplifiable (to just the theoretical explanation) if your dialog client view changes stick. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:41: // Larger horizontal spacing between unrelated controls. Nit: I wonder if you should just go ahead and add UNRELATED_CONTROL_HORIZONTAL_SPACING, since it would make this comment make sense, and we're going to want it anyway (my infobar patch uses it IIRC). https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:334: DCHECK_NE(button, ui::DIALOG_BUTTON_NONE); Nit: (expected, actual) But honestly I'd probably just kill this DCHECK and assume no caller is dumb enough to pass NONE for the button, since it makes no sense. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:337: return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END); Nit: Less code duplication: return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ? IDS_BROWSER_HANGMONITOR_RENDERER_WAIT : IDS_BROWSER_HANGMONITOR_RENDERER_END); https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:343: if (!rph) Nit: Up to you, but to me it makes sense in this case to do: if (rph) { ... } return true; Since the semantic meaning is basically "if there's a RPH, shut it down; then, unconditionally close this view." https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:371: if (hung_pages_table_model_->GetRenderViewHost()) { Nit: Factor to a temp for less duplication and shorter code: auto* rvh = hung_pages_table_model_->GetRenderViewHost(); if (rvh) rvh->GetWidget()->RestartHangMonitorTimeoutIfNecessary(); https://codereview.chromium.org/2660553005/diff/180001/ui/views/controls/butt... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2660553005/diff/180001/ui/views/controls/butt... ui/views/controls/button/md_text_button.cc:243: const int kHorizontalPadding = 16; Seems like this interacts with the minimum button width. Really, we want kHarmonyLayoutUnit DIP of horizontal padding on each end, plus a minimum text width of 2 * kHarmonyLayoutUnit. This adds up to the 4 * kHarmonyLayoutUnit minimum width total, but rather than fix that using SetMinSize() in the constructor, it seems like it would be better to plumb those constants here (as a "button horizontal padding" and a "button minimum label width"? Although maybe the "horizontal padding" should really be a "button insets", which would require adding a function to LayoutDelegate to return a gfx::Insets for a given enum, which I suspect we want anyway based on the history of chrome/browser/ui/layout_constants.h). This raises the question of whether the early-out atop this function when the label is empty is correct. I don't know when we use an MdTextButton with no text. We should find one and decide what the right thing per the spec is so we know how to handle this. This would apply even if we wanted to keep the SetMinSize() implementation. https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:183: size.Enlarge(0, buttons_height + GetButtonsAndExtraViewRowTopPadding()); The changes to eliminate this function and its callers below make this insane code more sane. But are they correct? It makes my head hurt to try and think through this, so I'm punting and just asking whether you thought really carefully through both Harmony and pre-Harmony layout to ensure that either this is identical to the previous behavior, or else it actually fixes bugs in the previous layout. (I could believe, for example, that pre-Harmony layout used to double-space the gap between content and button row, and this single-spaces it, but we want that.) https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:18: #include "ui/views/layout/layout_constants.h" Nit: Can this go away?
https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:20: // pre-Harmony, defies easy explanation. On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: This comment might be simplifiable (to just the theoretical explanation) if > your dialog client view changes stick. Done. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:41: // Larger horizontal spacing between unrelated controls. On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: I wonder if you should just go ahead and add > UNRELATED_CONTROL_HORIZONTAL_SPACING, since it would make this comment make > sense, and we're going to want it anyway (my infobar patch uses it IIRC). Done. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/hung_renderer_view.cc (right): https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:334: DCHECK_NE(button, ui::DIALOG_BUTTON_NONE); On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: (expected, actual) > > But honestly I'd probably just kill this DCHECK and assume no caller is dumb > enough to pass NONE for the button, since it makes no sense. Done (deleted). https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:337: return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END); On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: Less code duplication: > > return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) > ? IDS_BROWSER_HANGMONITOR_RENDERER_WAIT > : IDS_BROWSER_HANGMONITOR_RENDERER_END); Done. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:343: if (!rph) On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: Up to you, but to me it makes sense in this case to do: > > if (rph) { > ... > } > return true; > > Since the semantic meaning is basically "if there's a RPH, shut it down; then, > unconditionally close this view." Done. https://codereview.chromium.org/2660553005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/hung_renderer_view.cc:371: if (hung_pages_table_model_->GetRenderViewHost()) { On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: Factor to a temp for less duplication and shorter code: > > auto* rvh = hung_pages_table_model_->GetRenderViewHost(); > if (rvh) > rvh->GetWidget()->RestartHangMonitorTimeoutIfNecessary(); Done. https://codereview.chromium.org/2660553005/diff/180001/ui/views/controls/butt... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2660553005/diff/180001/ui/views/controls/butt... ui/views/controls/button/md_text_button.cc:243: const int kHorizontalPadding = 16; On 2017/02/13 23:47:31, Peter Kasting wrote: > Seems like this interacts with the minimum button width. Really, we want > kHarmonyLayoutUnit DIP of horizontal padding on each end, plus a minimum text > width of 2 * kHarmonyLayoutUnit. This adds up to the 4 * kHarmonyLayoutUnit > minimum width total, but rather than fix that using SetMinSize() in the > constructor, it seems like it would be better to plumb those constants here (as > a "button horizontal padding" and a "button minimum label width"? Although > maybe the "horizontal padding" should really be a "button insets", which would > require adding a function to LayoutDelegate to return a gfx::Insets for a given > enum, which I suspect we want anyway based on the history of > chrome/browser/ui/layout_constants.h). > > This raises the question of whether the early-out atop this function when the > label is empty is correct. I don't know when we use an MdTextButton with no > text. We should find one and decide what the right thing per the spec is so we > know how to handle this. This would apply even if we wanted to keep the > SetMinSize() implementation. The problem here is the label width is controlled by LabelButton, not here, and it doesn't have a concept of "minimum label size" right now. Plus most subclasses of LabelButton set their padding ad hoc, so it wouldn't be much use to add it outside of harmony. That said, I plumbed in the horizontal padding from ViewsDelegate in the interest of centralizing constants and defining everything in terms of layout units. I also changed the definition of DIALOG_BUTTON_MINIMUM_WIDTH to use the new BUTTON_HORIZONTAL_PADDING. As far as the early-out above, it seems weird to me that we would ever make an MdTextButton with no text. I'm guessing it's more for the case where the object has been created but not fully initialized by the parent view. https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:183: size.Enlarge(0, buttons_height + GetButtonsAndExtraViewRowTopPadding()); On 2017/02/13 23:47:31, Peter Kasting wrote: > The changes to eliminate this function and its callers below make this insane > code more sane. But are they correct? It makes my head hurt to try and think > through this, so I'm punting and just asking whether you thought really > carefully through both Harmony and pre-Harmony layout to ensure that either this > is identical to the previous behavior, or else it actually fixes bugs in the > previous layout. > > (I could believe, for example, that pre-Harmony layout used to double-space the > gap between content and button row, and this single-spaces it, but we want > that.) Yes, this is pretty wild, which is why I tried to punt on it earlier by just returning 0 in harmony. But having actually gone through it carefully I'm pretty sure this is correct. There are basically only two cases to consider: the button_row_insets_.top() == 0 and button_row_insets_.top() > 0. Since the former case is the default that means the actual top margin will be RELATED_CONTROL_VERTICAL_SPACING in almost every case. In the very small minority of cases where the subclass explicitly sets button_row_insets_.top() to >0 we would apply that inset twice, and I think it was actually introduced when we started refactoring for harmony so I highly doubt it was intentional. As for the changes in Layout, the only reason we were adding GetButtonsAndExtraViewRowTopPadding at the end was because it wasn't accounted for in GetButtonsAndExtraViewRowHeight, and now it is. https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:18: #include "ui/views/layout/layout_constants.h" On 2017/02/13 23:47:31, Peter Kasting wrote: > Nit: Can this go away? Yes, done.
LGTM https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:25: // Minimum label size plus padding. Maybe we should just expose the minimum label width directly? I dunno https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:20: // This is the spacing between a dialog button and the content above it. Nit: Remove "This is the" https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:337: // this can be deleted when harmony is always on. We use MdTextButton even pre-Harmony. Seems like this could be deleted now.
https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right): https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:25: // Minimum label size plus padding. On 2017/02/16 01:14:47, Peter Kasting wrote: > Maybe we should just expose the minimum label width directly? I dunno I think this is okay. https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2660553005/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:20: // This is the spacing between a dialog button and the content above it. On 2017/02/16 01:14:47, Peter Kasting wrote: > Nit: Remove "This is the" Done. https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:337: // this can be deleted when harmony is always on. On 2017/02/16 01:14:47, Peter Kasting wrote: > We use MdTextButton even pre-Harmony. Seems like this could be deleted now. I thought that too at first, but CreateSecondaryUiButton goes to LabelButton directly if harmony is disabled. See https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button....
(in case you're waiting for me) still lgtm - I did a quick scan
https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/220001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:337: // this can be deleted when harmony is always on. On 2017/02/16 01:31:04, Bret Sepulveda wrote: > On 2017/02/16 01:14:47, Peter Kasting wrote: > > We use MdTextButton even pre-Harmony. Seems like this could be deleted now. > > I thought that too at first, but CreateSecondaryUiButton goes to LabelButton > directly if harmony is disabled. See > https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button.... Well that seems dumb. Honestly I don't know why we even have MdTextButton at all instead of just making LabelButton be styled correctly for Harmony vs. not. Maybe I should file a bug to resolve that :/
Just a heads-up - I've started basing CLs off this to avoid merge conflicts later on. Looks like there's a test failure that you'll encounter in the CQ - possibly a bug in GetPreferredSize() (and it not matching what happens in Layout()) - but I haven't dug in. See comment. I did explore a couple of things for the refactoring I'm doing in https://codereview.chromium.org/2625083003/ . One weird thing: DialogClientViewTest calls a different constructor - calling the "real" one needs a ViewsDelegate, and there's some shuffling to do in DialogClientViewTest::SetUp to ensure the ViewsDelegate exists when the constructor is called. That didn't seem to fix the failing test sadly, but there might still be some useful stuff in https://codereview.chromium.org/2625083003/diff/60001/ui/views/window/dialog_... to help get the unit test into shape, if that's needed to get this through the CQ. https://codereview.chromium.org/2660553005/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:168: size.Enlarge(0, buttons_height); [ RUN ] DialogClientViewTest.LayoutWithButtons ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure Value of: this->bounds().height() Actual: 8 Expected: preferred_size.height() Which is: 0 ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure Value of: this->bounds().height() Actual: 8 Expected: preferred_size.height() Which is: 0 ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure Value of: this->bounds().height() Actual: 8 Expected: preferred_size.height() Which is: 0 ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure Value of: this->bounds().height() Actual: 8 Expected: preferred_size.height() Which is: 0 [ FAILED ] DialogClientViewTest.LayoutWithButtons (3 ms)
bsep@chromium.org changed reviewers: + sky@chromium.org
Fixing the test required some changes to DialogClientView::Layout, PTAL Adding sky@ for ui/views OWNERS https://codereview.chromium.org/2660553005/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2660553005/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:168: size.Enlarge(0, buttons_height); On 2017/02/16 11:12:35, tapted wrote: > > [ RUN ] DialogClientViewTest.LayoutWithButtons > ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure > Value of: this->bounds().height() > Actual: 8 > Expected: preferred_size.height() > Which is: 0 > ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure > Value of: this->bounds().height() > Actual: 8 > Expected: preferred_size.height() > Which is: 0 > ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure > Value of: this->bounds().height() > Actual: 8 > Expected: preferred_size.height() > Which is: 0 > ../../ui/views/window/dialog_client_view_unittest.cc:88: Failure > Value of: this->bounds().height() > Actual: 8 > Expected: preferred_size.height() > Which is: 0 > [ FAILED ] DialogClientViewTest.LayoutWithButtons (3 ms) I looked into this and I had made a small mistake in DialogClientView::Layout. The button row top inset controls the padding between the buttons and the content, of course. In Layout I accidentally applied the button row insets to the top of the dialog instead of above the buttons. But the content y isn't actually set from the dialog bounds, which means failing to apply the button row insets above the buttons meant the content was just that much taller, and thus there was no visual difference. But the test complained that the content was technically too large, so I've now fixed it. By the way, the layout model here is more incoherent than I realized, it could probably use further refactoring but this CL has expanded enough already. Basically the content is at the origin of the dialog, and controls its own border on the left and top. The button row insets on the other hand control the border around the right and bottom. I didn't investigate whether these were being set from the same value, hopefully they are, but in any case they at least happen to be the same right now.
LGTM If you see cleanup that makes sense to do separately, I definitely suggest filing a bug (against yourself?) with the details -- I agree that the layout and insets are incoherently handled.
lgtm2 % CL description: the sentence about --single-process can be dropped, since that stuff has landed separately now. However, it would be nice to summarize the DialogClientView changes. On 2017/02/17 02:37:24, Peter Kasting wrote: > LGTM > > If you see cleanup that makes sense to do separately, I definitely suggest > filing a bug (against yourself?) with the details -- I agree that the layout and > insets are incoherently handled. My plan is to convert DialogClientView to use GridLayout as part of http://crbug.com/671820, which should result in most of DCV::Layout() and DCV::GetPreferredSize() magically disappearing (what could possibly go wrong...). There are some weird use-cases for DCV though - I'm having to get creative.
LGTM
Description was changed from ========== Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also fixed an issue where BrowserDialogTest would not work on Windows without passing --single-process. BUG=652507 ========== to ========== Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also did some refactoring on DialogClientView to avoid hardcoding the minimum button width and to apply the button row insets more coherently. BUG=652507 ==========
On 2017/02/17 03:52:03, tapted wrote: > lgtm2 % CL description: the sentence about --single-process can be dropped, > since that stuff has landed separately now. However, it would be nice to > summarize the DialogClientView changes. Updated CL description. Thanks everyone.
The CQ bit was checked by bsep@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/2660553005/#ps260001 (title: "change dialog_client_view layout")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by bsep@chromium.org
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_TIMED_OUT, no build URL)
On 2017/02/17 23:26:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build > URL) Hmm, the test failure on mac_chromium_rel_ng is from my new dialog test. It looks like the spawned dialog is not being picked up by the framework. I had that issue earlier on Windows, but tapted@ fixed it already. I don't have a mac to dig into it. Should I just disable the test on OSX for now?
On 2017/02/18 00:49:09, Bret Sepulveda wrote: > On 2017/02/17 23:26:33, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no > build > > URL) > > Hmm, the test failure on mac_chromium_rel_ng is from my new dialog test. It > looks like the spawned dialog is not being picked up by the framework. I had > that issue earlier on Windows, but tapted@ fixed it already. I don't have a mac > to dig into it. Should I just disable the test on OSX for now? Fine by me -- I'll get it going in a follow up (I had it passing in that CL I forked off, so I'm not expecting major changes)
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2660553005/#ps280001 (title: "disable dialog test on osx")
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": 280001, "attempt_start_ts": 1487453808793920,
"parent_rev": "4ce7ff3ecc5d5b638cfdd38b956e80671426742a", "commit_rev":
"b621d8cec36bec73ef3eacddbcd7c998aee80cae"}
Message was sent while issue was closed.
Description was changed from ========== Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also did some refactoring on DialogClientView to avoid hardcoding the minimum button width and to apply the button row insets more coherently. BUG=652507 ========== to ========== Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also did some refactoring on DialogClientView to avoid hardcoding the minimum button width and to apply the button row insets more coherently. BUG=652507 Review-Url: https://codereview.chromium.org/2660553005 Cr-Commit-Position: refs/heads/master@{#451497} Committed: https://chromium.googlesource.com/chromium/src/+/b621d8cec36bec73ef3eacddbcd7... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/b621d8cec36bec73ef3eacddbcd7...
Message was sent while issue was closed.
Hi Bret, I believe that this change caused a test failure on one of our bots: https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/7533/st... Still trying to reproduce locally, but do you have any idea what the problem might be? Peter
Message was sent while issue was closed.
On 2017/02/22 00:26:58, pcc1 wrote: > Hi Bret, > > I believe that this change caused a test failure on one of our bots: > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/7533/st... > > Still trying to reproduce locally, but do you have any idea what the problem > might be? > > Peter I haven't investigated the failure, but I'm disabling the test here: https://codereview.chromium.org/2710633003 Sorry for the trouble! |
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-04a-buttons-light-theme.png%3Fz=width){kind=link}