Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795133002:
Chrome Cleaner UI: Add a dialog class for the new UI (Closed)
|
Created:
3 years, 8 months ago by alito Modified:
3 years, 8 months ago CC:
chromium-reviews, vakh+watch_chromium.org, tfarina, joenotcharles+watch_chromium.org, timvolodine, grt+watch_chromium.org, ftirelo+watch_chromium.org, alito+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome Cleaner UI: Add a dialog class for the new UI
Adds a new dialog class based on our current UX mocks. The intention
is to iterate with the UX team to come up with a UI for the Chrome
Cleanup tool.
Some functionality is currently missing and will be added in
subsequent CLs:
- Checkbox to ask for logs upload permissions
- Animation when the details section is expanded/folded
BUG=690020
Review-Url: https://codereview.chromium.org/2795133002
Cr-Commit-Position: refs/heads/master@{#462669}
Committed: https://chromium.googlesource.com/chromium/src/+/e82b14ba802b65c88c6775838d9d2d7d1315eac8
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Fabio's comments #
Total comments: 35
Patch Set 3 : Addressed sky@'s comments #Patch Set 4 : No longer subclassing View for the details section #
Total comments: 2
Patch Set 5 : Adressed more comments. #Patch Set 6 : Rebase #
Messages
Total messages: 27 (10 generated)
alito@chromium.org changed reviewers: + ftirelo@chromium.org, robertshield@google.com
Adding a first draft of the new Cleaner UI. PTAL. To see what the dialog currently looks like with some dummy strings, you can patch this CL on windows and run: browser_tests.exe --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=SRTPromptDialogTest.InvokeDialog_default Note: this is largely influenced by the the following CL (whose code was later greatly modified based on UX team's work): https://codereview.chromium.org/2701313002/ (see the file chrome/browser/ui/views/settings_reset_prompt_dialog.cc)
LGTM with a few nits. https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.cc:94: if (!(first_label || (is_bullet && last_label_was_bullet))) Please rewrite as: !first_label && (!is_bullet || !last_label_was_bullet) https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.h:28: // 3. Checkbox asking for permissions to upload logs (not yet implemented). Please explain the relationship with the controller. https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.h:32: explicit SRTPromptDialog(safe_browsing::SRTPromptController* controller); Nit: please explain who owns the controller pointer. https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc:10: // #include "content/public/test/browser_test.h" Please delete this.
Thanks Fabio. Scott, could you PTAL? This is largely influenced by another CL that you reviewed earlier (but the code was later almost completely rewritten due to UX changes): https://codereview.chromium.org/2701313002/ (see the file chrome/browser/ui/views/settings_reset_prompt_dialog.cc) To see what the dialog looks like, you can patch it on windows and run: browser_tests.exe --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=SRTPromptDialogTest.InvokeDialog_default /ali https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.cc:94: if (!(first_label || (is_bullet && last_label_was_bullet))) On 2017/04/05 18:03:40, ftirelo wrote: > Please rewrite as: > !first_label && (!is_bullet || !last_label_was_bullet) After much consternation and discussion, we have a readable form of this :-) https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.h:28: // 3. Checkbox asking for permissions to upload logs (not yet implemented). On 2017/04/05 18:03:40, ftirelo wrote: > Please explain the relationship with the controller. Done. https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog.h:32: explicit SRTPromptDialog(safe_browsing::SRTPromptController* controller); On 2017/04/05 18:03:40, ftirelo wrote: > Nit: please explain who owns the controller pointer. Done. https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... File chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc (right): https://codereview.chromium.org/2795133002/diff/1/chrome/browser/ui/views/srt... chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc:10: // #include "content/public/test/browser_test.h" On 2017/04/05 18:03:40, ftirelo wrote: > Please delete this. Actually, I think we need this include. I just forgot to uncomment the line.
alito@chromium.org changed reviewers: + sky@chromium.org
Now actually adding sky@ as reviewer. Please see description below.
https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_prompt_controller.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_prompt_controller.h:48: static void ShowSRTPrompt(Browser* browser, SRTPromptController* controller); This is a weird place to put this code. Move to browser_dialogs https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:51: static constexpr int kMainColumSetId = 0; Be consistent. No need for the static here. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:65: layout->AddColumnSet(0)->AddColumn( GridLayout seems like overkill here. Isn't this the same as a FillLayout and EmptyBorder? https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:165: class SRTPromptDialog::ExpandableMessageView : public views::View { It seems like you're subclassing here for convenience and not specialization. The only thing you really need is having visibility effect preferred size. Consider remove all child views and then adding back as necessary. That way you don't need a subclass. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:229: expand_details_button_color_(GetNativeTheme()->GetSystemColor( This variables is only used in this function. No need for it to be a member. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:231: expand_icon_( Why do you need to cache these icons? Can you look them up as needed? https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:265: Close(); This class is owned by the widget and deleted when the widget is destroyed. You should not need this call here. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:269: DCHECK(browser); DCHECK(!browser_)? https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:272: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_); Why do you need to get the BrowserView for browser_? You can get the get the NativeWindow by way of browser_->window()->GetNativeWindow(). https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:315: if (!interaction_done_) { If the underlying widget is destroyed outside of cancel/accept then neither accept or cancel is called. If you care, you should deal with that in the destructor of this class. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:36: class SRTPromptDialog : public views::DialogDelegateView, Current naming rules suggest this should SrtPromptDialog (see threads on chromium-dev). https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:39: // The |controller| object manages its own lifetime and is not owned by If controller manages its own lifetime how do you know controller outlives this object? https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:70: bool interaction_done_; Add description. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc:9: #include "chrome/browser/ui/views/srt_prompt_dialog.h" This should be your first include, then a newline (see style guide).
Thanks scott. I've addressed your comments. PTAnL! /ali https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_prompt_controller.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_prompt_controller.h:48: static void ShowSRTPrompt(Browser* browser, SRTPromptController* controller); On 2017/04/05 21:17:42, sky wrote: > This is a weird place to put this code. Move to browser_dialogs Done. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:51: static constexpr int kMainColumSetId = 0; On 2017/04/05 21:17:42, sky wrote: > Be consistent. No need for the static here. Done. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:65: layout->AddColumnSet(0)->AddColumn( On 2017/04/05 21:17:42, sky wrote: > GridLayout seems like overkill here. Isn't this the same as a FillLayout and > EmptyBorder? Indeed. I had tried adding an empty border to the view before, but hadn't realized that you also need to provide a layout manager. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:165: class SRTPromptDialog::ExpandableMessageView : public views::View { On 2017/04/05 21:17:42, sky wrote: > It seems like you're subclassing here for convenience and not specialization. > The only thing you really need is having visibility effect preferred size. > Consider remove all child views and then adding back as necessary. That way you > don't need a subclass. I plan to add animation to the expansion and folding of the details section. Given that, I think it makes sense to keep this as a class? https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:229: expand_details_button_color_(GetNativeTheme()->GetSystemColor( On 2017/04/05 21:17:42, sky wrote: > This variables is only used in this function. No need for it to be a member. A private member function now returns the color (after removing the icons as members, I need to get the color in two functions). https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:231: expand_icon_( On 2017/04/05 21:17:42, sky wrote: > Why do you need to cache these icons? Can you look them up as needed? Changed to look them up as needed. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:265: Close(); On 2017/04/05 21:17:42, sky wrote: > This class is owned by the widget and deleted when the widget is destroyed. You > should not need this call here. This call ensures that the controller is notified about the dialog being closed even when the dialog is somehow closed without user interaction. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:269: DCHECK(browser); On 2017/04/05 21:17:42, sky wrote: > DCHECK(!browser_)? Done. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:272: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_); On 2017/04/05 21:17:42, sky wrote: > Why do you need to get the BrowserView for browser_? You can get the get the > NativeWindow by way of browser_->window()->GetNativeWindow(). Ah! That's a shortcut I hadn't noticed. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:315: if (!interaction_done_) { On 2017/04/05 21:17:42, sky wrote: > If the underlying widget is destroyed outside of cancel/accept then neither > accept or cancel is called. If you care, you should deal with that in the > destructor of this class. This is already handled in the destructor, which calls the Close() function. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:36: class SRTPromptDialog : public views::DialogDelegateView, On 2017/04/05 21:17:42, sky wrote: > Current naming rules suggest this should SrtPromptDialog (see threads on > chromium-dev). There is a legacy problem in that we have a lot of classes and functions that use the SRT all caps naming scheme. See for example any of chrome/browser/safe_browsing/srt_*.h files. So for consistency with all those other cases, I'd like to keep this as is. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:39: // The |controller| object manages its own lifetime and is not owned by On 2017/04/05 21:17:42, sky wrote: > If controller manages its own lifetime how do you know controller outlives this > object? The contract (described by the controller class's declaration) is that the controller's Accept() or Cancel() functions must be called, after which the controller can delete itself when it sees fit. Those functions are called from this class when the dialog is closed (either via user action or because the widget is destroyed somehow). https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:70: bool interaction_done_; On 2017/04/05 21:17:42, sky wrote: > Add description. Done. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog_browsertest.cc:9: #include "chrome/browser/ui/views/srt_prompt_dialog.h" On 2017/04/05 21:17:42, sky wrote: > This should be your first include, then a newline (see style guide). Done.
https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:165: class SRTPromptDialog::ExpandableMessageView : public views::View { On 2017/04/06 00:20:22, alito wrote: > On 2017/04/05 21:17:42, sky wrote: > > It seems like you're subclassing here for convenience and not specialization. > > The only thing you really need is having visibility effect preferred size. > > Consider remove all child views and then adding back as necessary. That way > you > > don't need a subclass. > > I plan to add animation to the expansion and folding of the details section. > Given that, I think it makes sense to keep this as a class? It's easy to add it back when you get there, vs once code is landed it's easy to ignore. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:265: Close(); On 2017/04/06 00:20:22, alito wrote: > On 2017/04/05 21:17:42, sky wrote: > > This class is owned by the widget and deleted when the widget is destroyed. > You > > should not need this call here. > > This call ensures that the controller is notified about the dialog being closed > even when the dialog is somehow closed without user interaction. It's better to be explicit rather than relying on the implementation of Close() and the current implementation of Cancel(). For example, it's easy to miss (as I did) that Close() calls Cancel() and may lead to some confusion. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:315: if (!interaction_done_) { On 2017/04/06 00:20:22, alito wrote: > On 2017/04/05 21:17:42, sky wrote: > > If the underlying widget is destroyed outside of cancel/accept then neither > > accept or cancel is called. If you care, you should deal with that in the > > destructor of this class. > > This is already handled in the destructor, which calls the Close() function. Acknowledged.
Thanks Scott. I've removed the ExpandableMessageView class. PTAnL. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:165: class SRTPromptDialog::ExpandableMessageView : public views::View { On 2017/04/06 02:10:02, sky wrote: > On 2017/04/06 00:20:22, alito wrote: > > On 2017/04/05 21:17:42, sky wrote: > > > It seems like you're subclassing here for convenience and not > specialization. > > > The only thing you really need is having visibility effect preferred size. > > > Consider remove all child views and then adding back as necessary. That way > > you > > > don't need a subclass. > > > > I plan to add animation to the expansion and folding of the details section. > > Given that, I think it makes sense to keep this as a class? > > It's easy to add it back when you get there, vs once code is landed it's easy to > ignore. Fair enough. I've changed this to populate the details section when needed. https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:265: Close(); On 2017/04/06 02:10:02, sky wrote: > On 2017/04/06 00:20:22, alito wrote: > > On 2017/04/05 21:17:42, sky wrote: > > > This class is owned by the widget and deleted when the widget is destroyed. > > You > > > should not need this call here. > > > > This call ensures that the controller is notified about the dialog being > closed > > even when the dialog is somehow closed without user interaction. > > It's better to be explicit rather than relying on the implementation of Close() > and the current implementation of Cancel(). For example, it's easy to miss (as I > did) that Close() calls Cancel() and may lead to some confusion. Changed to explicitly notify the controller if needed.
https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:39: // The |controller| object manages its own lifetime and is not owned by On 2017/04/06 00:20:22, alito wrote: > On 2017/04/05 21:17:42, sky wrote: > > If controller manages its own lifetime how do you know controller outlives > this > > object? > > The contract (described by the controller class's declaration) is that the > controller's Accept() or Cancel() functions must be called, after which the > controller can delete itself when it sees fit. Those functions are called from > this class when the dialog is closed (either via user action or because the > widget is destroyed somehow). In that case, it seems you should null out controller_ when accent/cancel is called? In which case I think you could remove interaction_done_. https://codereview.chromium.org/2795133002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:177: details_view_ = new views::View(); move to member initializer?
Addressed your comments Scott. PTAnL. Thx. /ali https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.h (right): https://codereview.chromium.org/2795133002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.h:39: // The |controller| object manages its own lifetime and is not owned by On 2017/04/06 16:01:02, sky wrote: > On 2017/04/06 00:20:22, alito wrote: > > On 2017/04/05 21:17:42, sky wrote: > > > If controller manages its own lifetime how do you know controller outlives > > this > > > object? > > > > The contract (described by the controller class's declaration) is that the > > controller's Accept() or Cancel() functions must be called, after which the > > controller can delete itself when it sees fit. Those functions are called from > > this class when the dialog is closed (either via user action or because the > > widget is destroyed somehow). > > In that case, it seems you should null out controller_ when accent/cancel is > called? In which case I think you could remove interaction_done_. Done. https://codereview.chromium.org/2795133002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/srt_prompt_dialog.cc (right): https://codereview.chromium.org/2795133002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/srt_prompt_dialog.cc:177: details_view_ = new views::View(); On 2017/04/06 16:01:02, sky wrote: > move to member initializer? Done.
LGTM
Thanks Scott for the review! Robert, do you have any comments?
robertshield@chromium.org changed reviewers: + robertshield@chromium.org
lgtm
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@chromium.org Link to the patchset: https://codereview.chromium.org/2795133002/#ps80001 (title: "Adressed more comments.")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@chromium.org, robertshield@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2795133002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491516058625920,
"parent_rev": "e9d564bfe77be2833f5aa818ebc064f261692b06", "commit_rev":
"e82b14ba802b65c88c6775838d9d2d7d1315eac8"}
Message was sent while issue was closed.
Description was changed from ========== Chrome Cleaner UI: Add a dialog class for the new UI Adds a new dialog class based on our current UX mocks. The intention is to iterate with the UX team to come up with a UI for the Chrome Cleanup tool. Some functionality is currently missing and will be added in subsequent CLs: - Checkbox to ask for logs upload permissions - Animation when the details section is expanded/folded BUG=690020 ========== to ========== Chrome Cleaner UI: Add a dialog class for the new UI Adds a new dialog class based on our current UX mocks. The intention is to iterate with the UX team to come up with a UI for the Chrome Cleanup tool. Some functionality is currently missing and will be added in subsequent CLs: - Checkbox to ask for logs upload permissions - Animation when the details section is expanded/folded BUG=690020 Review-Url: https://codereview.chromium.org/2795133002 Cr-Commit-Position: refs/heads/master@{#462669} Committed: https://chromium.googlesource.com/chromium/src/+/e82b14ba802b65c88c6775838d9d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e82b14ba802b65c88c6775838d9d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
