Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(364)

Issue 2816293002: Description: Replace layout constants in chrome/browser/extensions and chrome/browser/first_run (Closed)

Created:
3 years, 8 months ago by ananta
Modified:
3 years, 7 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tzik, kinuko+fileapi, nhiroki, extensions-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+236 lines, -828 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 23 24 25 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/BUILD.gn View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/file_system/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/file_system/file_system_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +15 lines, -3 lines 0 comments Download
D chrome/browser/extensions/api/file_system/request_file_system_dialog_view.h View 1 2 1 chunk +0 lines, -66 lines 0 comments Download
M chrome/browser/extensions/api/file_system/request_file_system_dialog_view.cc View 1 2 1 chunk +0 lines, -142 lines 0 comments Download
D chrome/browser/first_run/try_chrome_dialog_view.h View 1 2 1 chunk +0 lines, -126 lines 0 comments Download
M chrome/browser/first_run/try_chrome_dialog_view.cc View 1 2 1 chunk +0 lines, -369 lines 0 comments Download
M chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +34 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 2 chunks +13 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/extensions/request_file_system_dialog_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +41 lines, -0 lines 0 comments Download
A + chrome/browser/ui/views/extensions/request_file_system_dialog_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +10 lines, -17 lines 0 comments Download
A + chrome/browser/ui/views/extensions/request_file_system_dialog_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 7 chunks +24 lines, -39 lines 0 comments Download
A + chrome/browser/ui/views/try_chrome_dialog_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +27 lines, -7 lines 0 comments Download
A + chrome/browser/ui/views/try_chrome_dialog_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 13 chunks +64 lines, -53 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 117 (94 generated)
ananta
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 ...
3 years, 8 months ago (2017-04-15 03:39:04 UTC) #2
Peter Kasting
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#newcode78 chrome/browser/DEPS:78: "+chrome/browser/ui/views/harmony/chrome_layout_provider.h", I don't think adding this here is correct. ...
3 years, 8 months ago (2017-04-17 19:36:18 UTC) #14
ananta
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#newcode78 chrome/browser/DEPS:78: "+chrome/browser/ui/views/harmony/chrome_layout_provider.h", On 2017/04/17 19:36:18, Peter Kasting wrote: > I ...
3 years, 8 months ago (2017-04-26 02:37:29 UTC) #22
Peter Kasting
This seems good; the only thing I'm really missing are the test expansions to allow ...
3 years, 8 months ago (2017-04-26 18:26:25 UTC) #39
ananta
Added a dialog test for TryChromeDialogView. The RequestFileSystemView one is a touch complicated as it ...
3 years, 8 months ago (2017-04-26 19:42:21 UTC) #41
ananta
+lazyboy for extensions OWNERS
3 years, 8 months ago (2017-04-27 00:29:44 UTC) #58
Peter Kasting
https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensions/request_file_system_dialog_browsertest.cc File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensions/request_file_system_dialog_browsertest.cc#newcode24 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/extensions/request_file_system_dialog_browsertest.cc#newcode42 chrome/browser/extensions/request_file_system_dialog_browsertest.cc:42: ...
3 years, 8 months ago (2017-04-27 01:08:05 UTC) #65
ananta
https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensions/request_file_system_dialog_browsertest.cc File chrome/browser/extensions/request_file_system_dialog_browsertest.cc (right): https://codereview.chromium.org/2816293002/diff/240001/chrome/browser/extensions/request_file_system_dialog_browsertest.cc#newcode24 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 ...
3 years, 8 months ago (2017-04-27 01:29:53 UTC) #69
Peter Kasting
LGTM https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensions/api/file_system/file_system_api.cc File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensions/api/file_system/file_system_api.cc#newcode451 chrome/browser/extensions/api/file_system/file_system_api.cc:451: ((volume->volume_label().empty() == false) ? volume->volume_label() Discussed this in ...
3 years, 8 months ago (2017-04-27 01:50:33 UTC) #71
ananta
https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensions/api/file_system/file_system_api.cc File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/300001/chrome/browser/extensions/api/file_system/file_system_api.cc#newcode451 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 ...
3 years, 8 months ago (2017-04-27 01:55:16 UTC) #72
lazyboy
https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/DEPS File chrome/browser/extensions/api/file_system/DEPS (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/DEPS#newcode3 chrome/browser/extensions/api/file_system/DEPS:3: "+chrome/browser/ui/views/extensions", nit: sort https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/file_system_api.cc File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/file_system_api.cc#newcode443 chrome/browser/extensions/api/file_system/file_system_api.cc:443: ...
3 years, 7 months ago (2017-04-27 16:46:14 UTC) #77
ananta
https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/DEPS File chrome/browser/extensions/api/file_system/DEPS (right): https://codereview.chromium.org/2816293002/diff/320001/chrome/browser/extensions/api/file_system/DEPS#newcode3 chrome/browser/extensions/api/file_system/DEPS:3: "+chrome/browser/ui/views/extensions", On 2017/04/27 16:46:13, lazyboy wrote: > nit: sort ...
3 years, 7 months ago (2017-04-27 18:37:47 UTC) #78
lazyboy
lgtm
3 years, 7 months ago (2017-04-27 20:27:02 UTC) #83
Peter Kasting
Consult with tapted about how to handle modal dialogs? https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/380001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode182 chrome/browser/ui/views/try_chrome_dialog_view.cc:182: ...
3 years, 7 months ago (2017-04-28 06:16:41 UTC) #88
ananta
Will check with tapted about the dialog issue. I plan to land this patch once ...
3 years, 7 months ago (2017-04-28 14:57:01 UTC) #89
ananta
grt, please look at the TryChromeDialogView changes. Thanks
3 years, 7 months ago (2017-04-28 14:57:47 UTC) #91
ananta
+sky
3 years, 7 months ago (2017-04-28 20:42:54 UTC) #107
sky
LGTM https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode311 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/views/try_chrome_dialog_view.cc#newcode316 chrome/browser/ui/views/try_chrome_dialog_view.cc:316: gfx::Rect TryChromeDialogView::ComputeWindowPosition(gfx::Size ...
3 years, 7 months ago (2017-04-28 21:25:49 UTC) #108
ananta
https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2816293002/diff/500001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode311 chrome/browser/ui/views/try_chrome_dialog_view.cc:311: listener.Run(NULL); On 2017/04/28 21:25:49, sky wrote: > NULL -> ...
3 years, 7 months ago (2017-04-28 21:45:23 UTC) #109
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2816293002/520001
3 years, 7 months ago (2017-04-28 21:46:24 UTC) #112
commit-bot: I haz the power
Committed patchset #27 (id:520001) as https://chromium.googlesource.com/chromium/src/+/5ecf11b9c4f09559336a513b4d0486a3f74d2625
3 years, 7 months ago (2017-04-29 00:45:47 UTC) #115
findit-for-me
Findit(https://goo.gl/kROfz5) identified this CL at revision 468195 as the culprit for failures in the build ...
3 years, 7 months ago (2017-04-29 04:46:54 UTC) #116
vabr (Chromium)
3 years, 7 months ago (2017-05-02 13:47:54 UTC) #117
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..

Powered by Google App Engine
This is Rietveld 408576698