|
|
DescriptionDescription: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run
Changes in this patch as below:
1. Replace references to constants in ui/views/layout/layout_constants.h with their
equivalents using ChromeLayoutProvider.
2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of
a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for
simplicity. Removed the dependency on Extension and Volume datatypes from this class, which
enables us to easily test the dialog. The extension name, volume label and id are passed in.
We maintain the contract of invoking the cancel callback if the volume label is empty. That is
done in the caller ConsentProviderDelegate::ShowDialog().
3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs.
4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView class. This allows it to
instantiate an instance of this class. The TryChromeDialogView class no longer puts up a modal
dialog by default. This is controlled by the DialogType enum, which for chrome is always MODAL.
For the browser tests we pass in MODELESS here, as the loop is run by the test. The ShowModal() method
is renamed to ShowDialog(). It takes an additional enum UsageType which controls whether we are in testing
mode. In this case we always want the dialog to be displayed.
BUG=691897
TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default
TBR=grt
Review-Url: https://codereview.chromium.org/2816293002
Cr-Commit-Position: refs/heads/master@{#468195}
Committed: https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d0486a3f74d2625
Patch Set 1 #
Total comments: 1
Patch Set 2 : git cl try #
Total comments: 12
Patch Set 3 : Move try_chrome_dialog_view.cc/.h to chrome/browser/ui/views #Patch Set 4 : Fix build.gn #Patch Set 5 : Fix compile failures #Patch Set 6 : Changed RequestFileSystemDialogView to subclass DialogDelegateView and use FillLayout #Patch Set 7 : Fix compile failures #Patch Set 8 : Fix compile failures on chrome_os #Patch Set 9 : Fix dumb error #
Total comments: 9
Patch Set 10 : Address review comments and add test to display the TryChromeDialog #Patch Set 11 : Fix build errors #Patch Set 12 : Fix compile failures on chrome os #Patch Set 13 : Update DEPS #
Total comments: 10
Patch Set 14 : Fix compile failures on chromeos #Patch Set 15 : Fix tests #Patch Set 16 : Next round of review comments #
Total comments: 4
Patch Set 17 : Review comments #
Total comments: 10
Patch Set 18 : Review comments #Patch Set 19 : git cl format #Patch Set 20 : Got the TryChromeDialog to show up. Its bounds need to be specified in DIPs #
Total comments: 8
Patch Set 21 : Disable the TryChromeDialogTest for now. Will renable once I figure out how to handle modal dialogs. #Patch Set 22 : Remove includes #Patch Set 23 : Add a force parameter to the TryChromeDialogView::Show() function. This forces the dialog to always… #Patch Set 24 : Fix build error #Patch Set 25 : Remove newlines #Patch Set 26 : Enable the TryChromeDialogTest.InvokeDialog_default test. The TryChromeDialogTest is now a friend o… #
Total comments: 6
Patch Set 27 : Address review comments #Messages
Total messages: 117 (94 generated)
ananta@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2816293002/diff/1/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2816293002/diff/1/chrome/browser/DEPS#newcode78 chrome/browser/DEPS:78: "+chrome/browser/ui/views/harmony/chrome_layout_provider.h", pkasting, Please check if this DEPS update is reasonable.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ananta@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 ananta@chromium.org
The CQ bit was checked by ananta@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.
https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:78: "+chrome/browser/ui/views/harmony/chrome_layout_provider.h", I don't think adding this here is correct. Instead, the other two files you modify should both move to subdirectories of c/b/ui/views/. For example, that directory already has multiple files called first_run_xxx; maybe we should have a c/b/ui/views/first_run/ that they, and try_chrome_dialog_view.cc, all move to. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/file_system/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/file_system/request_file_system_dialog_view.cc:139: provider->GetDistanceMetric(DISTANCE_PANEL_CONTENT_MARGIN); This combination is basically INSETS_PANEL. We could ask for a GridLayout with these insets using GridLayout::CreatePanel(). But this is overkill, since there's only one child. It seems like we don't even need a BoxLayout, as we could just set |contents_view_|'s insets to INSETS_PANEL and then use a FillLayout. This would require that |contents_view_| returns the correct preferred size. The easiest way to accomplish that seems like it would be to change this class to subclass DialogDelegateView instead of DialogDelegate, and nuke |contents_view_| entirely. Then we could override GetPreferredSize() to return a width of 320 and a height calculated using GetHeightForWidth() on the label. You'd have to double-check that this sizes the label properly. The overall result of those changes ought to be a noticeably simpler class. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:119: columns->AddPaddingColumn(0, related_spacing); Seems like this should be using RELATED_LABEL, given the comment above. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:142: columns->AddPaddingColumn(0, icon_size.width()); It looks to me as if this was supposed to have another padding column in here, and is missing it. What does this dialog look like? This suggests that instead of adding two padding columns, we should have computed one "icon + padding" width above and then used it for all these padding places below. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:240: // In this flavor we have some veritical space, then a separator line Nit: While here: correct "veritical" misspelling https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:242: layout->AddPaddingRow(0, unrelated_spacing); This is a behavior change; you went from a vertical constant to a horizontal one, and the two didn't have the same values. (2 places)
The CQ bit was checked by ananta@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 checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 ananta@chromium.org to run a CQ dry run
https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/DEPS#new... chrome/browser/DEPS:78: "+chrome/browser/ui/views/harmony/chrome_layout_provider.h", On 2017/04/17 19:36:18, Peter Kasting wrote: > I don't think adding this here is correct. Instead, the other two files you > modify should both move to subdirectories of c/b/ui/views/. For example, that > directory already has multiple files called first_run_xxx; maybe we should have > a c/b/ui/views/first_run/ that they, and try_chrome_dialog_view.cc, all move to. I moved try_chrome_dialog_view.cc to chrome/browser/ui/views. Will move the first run code from ui/views to ui/views/first_run in a later patchset. Moved request_file_system_dialog_view.cc/.h to chrome/browser/ui/views/extensions https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/file_system/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/file_system/request_file_system_dialog_view.cc:139: provider->GetDistanceMetric(DISTANCE_PANEL_CONTENT_MARGIN); On 2017/04/17 19:36:18, Peter Kasting wrote: > This combination is basically INSETS_PANEL. > > We could ask for a GridLayout with these insets using GridLayout::CreatePanel(). > But this is overkill, since there's only one child. It seems like we don't > even need a BoxLayout, as we could just set |contents_view_|'s insets to > INSETS_PANEL and then use a FillLayout. This would require that > |contents_view_| returns the correct preferred size. > > The easiest way to accomplish that seems like it would be to change this class > to subclass DialogDelegateView instead of DialogDelegate, and nuke > |contents_view_| entirely. Then we could override GetPreferredSize() to return > a width of 320 and a height calculated using GetHeightForWidth() on the label. > You'd have to double-check that this sizes the label properly. > > The overall result of those changes ought to be a noticeably simpler class. Changed to use FillLayout. To return the insets I added an override for GetInsets() in the dialog and returned panel insets. I am not sure if this is the right way to do it. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:119: columns->AddPaddingColumn(0, related_spacing); On 2017/04/17 19:36:18, Peter Kasting wrote: > Seems like this should be using RELATED_LABEL, given the comment above. Done. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:142: columns->AddPaddingColumn(0, icon_size.width()); On 2017/04/17 19:36:18, Peter Kasting wrote: > It looks to me as if this was supposed to have another padding column in here, > and is missing it. What does this dialog look like? > > This suggests that instead of adding two padding columns, we should have > computed one "icon + padding" width above and then used it for all these padding > places below. Thanks done. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:240: // In this flavor we have some veritical space, then a separator line On 2017/04/17 19:36:18, Peter Kasting wrote: > Nit: While here: correct "veritical" misspelling Done. https://codereview.chromium.org/2816293002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/try_chrome_dialog_view.cc:242: layout->AddPaddingRow(0, unrelated_spacing); On 2017/04/17 19:36:18, Peter Kasting wrote: > This is a behavior change; you went from a vertical constant to a horizontal > one, and the two didn't have the same values. (2 places) Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
This seems good; the only thing I'm really missing are the test expansions to allow us to programmatically trigger these. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/DEPS#ne... chrome/browser/DEPS:77: "+chrome/browser/ui/views/try_chrome_dialog_view.h", You may want to run this by jam to see if he has an idea for how to clean up chrome_browser_main.cc's direct dependency on this UI, which seems icky. That's probably doable in a separate CL. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:131: views::FillLayout* const layout = new views::FillLayout(); Nit: Or just inline into next line. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.h:50: // View overrides: Nit: You don't subclass View, just DialogDelegateView, so these can stay in the block above without a separate section. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:127: // Optional second row: [pad][pad][radio 1]. Nit: This and comments below that say [pad][pad] should now only have one leading [pad]. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:151: columns->AddPaddingColumn(0, label_spacing); Nit: This should be one padding column of size |icon_padding|.
Description was changed from ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 ========== to ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity BUG=691897 ==========
Added a dialog test for TryChromeDialogView. The RequestFileSystemView one is a touch complicated as it requires an extension and a volume manager for the dialog to show up. Would it be ok to do that in a later patchset?. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:131: views::FillLayout* const layout = new views::FillLayout(); On 2017/04/26 18:26:25, Peter Kasting wrote: > Nit: Or just inline into next line. Done. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.h:50: // View overrides: On 2017/04/26 18:26:25, Peter Kasting wrote: > Nit: You don't subclass View, just DialogDelegateView, so these can stay in the > block above without a separate section. Done. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:127: // Optional second row: [pad][pad][radio 1]. On 2017/04/26 18:26:25, Peter Kasting wrote: > Nit: This and comments below that say [pad][pad] should now only have one > leading [pad]. Done. https://codereview.chromium.org/2816293002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:151: columns->AddPaddingColumn(0, label_spacing); On 2017/04/26 18:26:25, Peter Kasting wrote: > Nit: This should be one padding column of size |icon_padding|. Thanks done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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-...) linux_chromium_chromeos_ozone_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 ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity BUG=691897 ========== to ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ananta@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...
ananta@chromium.org changed reviewers: + lazyboy@chromium.org
+lazyboy for extensions OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 ananta@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 checked by ananta@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...
https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:24: auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); Nit: Unused local https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:42: // --dialog=TryChromeDialogTest.InvokeDialog_default Nit: Looks like most other test files omit this comment, so I would here too. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... File chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc:71: 1000, base::Bind(&TryChromeDialogTest::SetActiveModalDialog)); It looks to me like instead of passing 1000 here, we want to provide several different InvokeDialog cases below which invoke all the different flavors of this dialog? But it's not clear what "all those flavors" are. I'm confused by the flavors code. It seems like there ought to be some kind of flavors enum. Also boo for "flavors" being passed in as a size_t but then silently truncated to int to pass to a later function... but that's a pre-existing bug, not your fault. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc:85: // --dialog=TryChromeDialogTest.InvokeDialog_default Nit: Looks like most other test files omit this comment, so I would here too. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:106: base::UTF8ToUTF16(!volume_label.empty() ? volume_label : volume_id); Nit: It seems like it'd be better to just pass in one |volume_name| parameter, and have the caller do this determination. Also, reverse conditional, so it doesn't read like a mental double-negative.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 ananta@chromium.org to run a CQ dry run
https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:24: auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); On 2017/04/27 01:08:05, Peter Kasting wrote: > Nit: Unused local Done. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:42: // --dialog=TryChromeDialogTest.InvokeDialog_default On 2017/04/27 01:08:05, Peter Kasting wrote: > Nit: Looks like most other test files omit this comment, so I would here too. Done. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... File chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc:71: 1000, base::Bind(&TryChromeDialogTest::SetActiveModalDialog)); On 2017/04/27 01:08:05, Peter Kasting wrote: > It looks to me like instead of passing 1000 here, we want to provide several > different InvokeDialog cases below which invoke all the different flavors of > this dialog? But it's not clear what "all those flavors" are. I'm confused by > the flavors code. It seems like there ought to be some kind of flavors enum. > Also boo for "flavors" being passed in as a size_t but then silently truncated > to int to pass to a later function... but that's a pre-existing bug, not your > fault. Yeah. Sorry about that. I was chatting with Greg. He said 0 works for him. I ended up typing 1000 without thinking here. In any case I will add the other flavors once the dialog starts showing up. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/first_r... chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc:85: // --dialog=TryChromeDialogTest.InvokeDialog_default On 2017/04/27 01:08:05, Peter Kasting wrote: > Nit: Looks like most other test files omit this comment, so I would here too. Done. https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:106: base::UTF8ToUTF16(!volume_label.empty() ? volume_label : volume_id); On 2017/04/27 01:08:05, Peter Kasting wrote: > Nit: It seems like it'd be better to just pass in one |volume_name| parameter, > and have the caller do this determination. > > Also, reverse conditional, so it doesn't read like a mental double-negative. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:451: ((volume->volume_label().empty() == false) ? volume->volume_label() Discussed this in person :) https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.h:48: const std::string& extension_name, Nit: I don't care whether we use "name" or "id" for this, but ShowDialog() uses one and this uses the other. Be consistent, here and in the .cc file.
https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:451: ((volume->volume_label().empty() == false) ? volume->volume_label() On 2017/04/27 01:50:33, Peter Kasting wrote: > Discussed this in person :) Done. https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.h:48: const std::string& extension_name, On 2017/04/27 01:50:33, Peter Kasting wrote: > Nit: I don't care whether we use "name" or "id" for this, but ShowDialog() uses > one and this uses the other. Be consistent, here and in the .cc file. Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/DEPS (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/DEPS:3: "+chrome/browser/ui/views/extensions", nit: sort https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:443: if (!volume.get()) { Thanks for moving this out of RequestFileSystemDialogView constructor. https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Should this file be moved to the directory where request_file_system_dialog_view.h resides? https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:32: void DialogCallback(ui::DialogButton button) {} nit: private, static https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:34: const std::string volume_label, const std::string&
https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/DEPS (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/DEPS:3: "+chrome/browser/ui/views/extensions", On 2017/04/27 16:46:13, lazyboy wrote: > nit: sort Done. https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:443: if (!volume.get()) { On 2017/04/27 16:46:13, lazyboy wrote: > Thanks for moving this out of RequestFileSystemDialogView constructor. Sure thing https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/27 16:46:13, lazyboy wrote: > Should this file be moved to the directory where > request_file_system_dialog_view.h resides? Done. https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensi... chrome/browser/extensions/request_file_system_dialog_browsertest.cc:32: void DialogCallback(ui::DialogButton button) {} On 2017/04/27 16:46:13, lazyboy wrote: > nit: private, static Done. https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc:34: const std::string volume_label, On 2017/04/27 16:46:14, lazyboy wrote: > const std::string& Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...) 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_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_...)
lgtm
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Consult with tapted about how to handle modal dialogs? https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:182: if ((!for_testing_ && !install_static::SupportsRetentionExperiments()) || I don't think we should use a private static for this. It seems better to inject it as an optional constructor argument. This would avoid the need for a friend declaration and for toggling this on and off in your test code. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:291: // Bounds have to be in DIPs. This comment can probably be omitted since all bounds everywhere in views are always assumed to be in DIPs. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:292: popup_->SetBounds(display::Screen::GetScreen()->ScreenToDIPRectInWindow( This conversion should probably be done in ComputeWindowPosition() instead. That function should probably be using other methods to to do most of its work (e.g. DesktopWindowTreeHostWin::GetWorkAreaBoundsInScreen(), or at least Screen::GetPrimaryDisplay() to get the primary monitor), and in general looks relatively scary to me. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:111: friend class TryChromeDialogTest; Nit: Friend declarations typically go at the top of the section
Will check with tapted about the dialog issue. I plan to land this patch once the bots agree. Will enable the test in a later patchset. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:182: if ((!for_testing_ && !install_static::SupportsRetentionExperiments()) || On 2017/04/28 06:16:41, Peter Kasting (OOO thru May 3) wrote: > I don't think we should use a private static for this. It seems better to > inject it as an optional constructor argument. > > This would avoid the need for a friend declaration and for toggling this on and > off in your test code. I added a force parameter to the Show() function which is a public static. The ctor is private and so is the ShowModal() function. The force flag is passed through to ShowModal. Set to true only for tests. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:291: // Bounds have to be in DIPs. On 2017/04/28 06:16:41, Peter Kasting (OOO thru May 3) wrote: > This comment can probably be omitted since all bounds everywhere in views are > always assumed to be in DIPs. Done. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:292: popup_->SetBounds(display::Screen::GetScreen()->ScreenToDIPRectInWindow( On 2017/04/28 06:16:41, Peter Kasting (OOO thru May 3) wrote: > This conversion should probably be done in ComputeWindowPosition() instead. > > That function should probably be using other methods to to do most of its work > (e.g. DesktopWindowTreeHostWin::GetWorkAreaBoundsInScreen(), or at least > Screen::GetPrimaryDisplay() to get the primary monitor), and in general looks > relatively scary to me. Thanks. Done. https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:111: friend class TryChromeDialogTest; On 2017/04/28 06:16:41, Peter Kasting (OOO thru May 3) wrote: > Nit: Friend declarations typically go at the top of the section no longer needed with the force parameter.
ananta@chromium.org changed reviewers: + grt@chromium.org
grt, please look at the TryChromeDialogView changes. Thanks
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default ========== to ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default TBR=grt ==========
The CQ bit was checked by ananta@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.
Description was changed from ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default TBR=grt ========== to ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView class. This allows it to instantiate an instance of this class. The TryChromeDialogView class no longer puts up a modal dialog by default. This is controlled by the DialogType enum, which for chrome is always MODAL. For the browser tests we pass in MODELESS here, as the loop is run by the test. The ShowModal() method is renamed to ShowDialog(). It takes an additional enum UsageType which controls whether we are in testing mode. In this case we always want the dialog to be displayed. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default TBR=grt ==========
The CQ bit was checked by ananta@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...
ananta@chromium.org changed reviewers: + sky@chromium.org
+sky
LGTM https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:311: listener.Run(NULL); NULL -> nullptr https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:316: gfx::Rect TryChromeDialogView::ComputeWindowPosition(gfx::Size size, const gfx::Size& https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:86: enum DialogType { Use enum class. Also, we have moved to kFoo for enums (*SIGH*).
https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:311: listener.Run(NULL); On 2017/04/28 21:25:49, sky wrote: > NULL -> nullptr Done. https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:316: gfx::Rect TryChromeDialogView::ComputeWindowPosition(gfx::Size size, On 2017/04/28 21:25:49, sky wrote: > const gfx::Size& Done. https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:86: enum DialogType { On 2017/04/28 21:25:49, sky wrote: > Use enum class. Also, we have moved to kFoo for enums (*SIGH*). Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, lazyboy@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2816293002/#ps520001 (title: "Address review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 520001, "attempt_start_ts": 1493415935176560, "parent_rev": "38ce97ec22ab1faecb1523f2d00eef0e17568043", "commit_rev": "5ecf11b9c4f09559336a513b4d0486a3f74d2625"}
Message was sent while issue was closed.
Description was changed from ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView class. This allows it to instantiate an instance of this class. The TryChromeDialogView class no longer puts up a modal dialog by default. This is controlled by the DialogType enum, which for chrome is always MODAL. For the browser tests we pass in MODELESS here, as the loop is run by the test. The ShowModal() method is renamed to ShowDialog(). It takes an additional enum UsageType which controls whether we are in testing mode. In this case we always want the dialog to be displayed. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default TBR=grt ========== to ========== Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run Changes in this patch as below: 1. Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. 2. Changed the RequestFileSystemDialogView class to use a simpler FillLayout instead of a BoxLayout. This class now subclasses DialogDelegateView instead of DialogDelegate for simplicity. Removed the dependency on Extension and Volume datatypes from this class, which enables us to easily test the dialog. The extension name, volume label and id are passed in. We maintain the contract of invoking the cancel callback if the volume label is empty. That is done in the caller ConsentProviderDelegate::ShowDialog(). 3. Added browser tests for displaying the TryChromeDialogView and RequestFileSystemDialogView dialogs. 4. Made the TryChromeDialogTest class a friend of the TryChromeDialogView class. This allows it to instantiate an instance of this class. The TryChromeDialogView class no longer puts up a modal dialog by default. This is controlled by the DialogType enum, which for chrome is always MODAL. For the browser tests we pass in MODELESS here, as the loop is run by the test. The ShowModal() method is renamed to ShowDialog(). It takes an additional enum UsageType which controls whether we are in testing mode. In this case we always want the dialog to be displayed. BUG=691897 TEST=Covered by tests RequestFileSystemDialogTest.InvokeDialog_default and TryChromeDialogTest.InvokeDialog_default TBR=grt Review-Url: https://codereview.chromium.org/2816293002 Cr-Commit-Position: refs/heads/master@{#468195} Committed: https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d04... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001) as https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d04...
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) identified this CL at revision 468195 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:520001) has been created in https://codereview.chromium.org/2856883002/ by vabr@chromium.org. The reason for reverting is: Likely broke some tests on sanitizers. More info at https://crbug.com/691897#c14.. |