Inactive toast ux changes

chrome/browser/ui/views/try_chrome_dialog_view.h

Index: chrome/browser/ui/views/try_chrome_dialog_view.h
diff --git a/chrome/browser/ui/views/try_chrome_dialog_view.h b/chrome/browser/ui/views/try_chrome_dialog_view.h
index 004cbbc63c1cfd2c72494a5863e108934b29bf11..4fc01244a5c94e7f4f3524d676cb5e7ae55ad175 100644
--- a/chrome/browser/ui/views/try_chrome_dialog_view.h
+++ b/chrome/browser/ui/views/try_chrome_dialog_view.h
@@ -9,52 +9,34 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/time/time.h"
+#include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/installer/util/experiment_storage.h"
+#include "ui/gfx/font_list.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/link_listener.h"
 
 namespace views {
-class RadioButton;
-class Checkbox;
 class Widget;
 }
 
 // This class displays a modal dialog using the views system. The dialog asks
-// the user to give chrome another try. This class only handles the UI so the
-// resulting actions are up to the caller. One flavor looks like this:
+// the user to give Chrome another try. This class only handles the UI so the
+// resulting actions are up to the caller.
 //
-//   +-----------------------------------------------+
-//   | |icon| There is a new, safer version      [x] |
-//   | |icon| of Google Chrome available             |
-//   |        [o] Try it out (already installed)     |
-//   |        [ ] Uninstall Google Chrome            |
-//   |        [ OK ] [Don't bug me]                  |
-//   |        _why_am_I_seeing this?_                |
-//   +-----------------------------------------------+
+// The layout is as follows:
 //
-// Another flavor looks like:
 //   +-----------------------------------------------+
-//   | |icon| There is a new, safer version      [x] |
-//   | |icon| of Google Chrome available             |
-//   |        [o] Try it out (already installed)     |
-//   |        [ ] Don't bug me                       |
-//   |                  [ OK ]                       |
+//   | |icon| Header text.                       [x] |
+//   |                                               |
+//   |        Body text.                             |
+//   |        [ Open Chrome ] [No Thanks]            |
 //   +-----------------------------------------------+
 //
-// And the 2013 version looks like:
-//   +-----------------------------------------------+
-//   | |icon| There is a new version of          [x] |
-//   | |icon| Google Chrome available                |
-//   |        [o] Try it out (already installed)     |
-//   |        [ ] Don't bug me                       |
-//   | --------------------------------------------- |
-//   | [x] Make it the default browser       [ OK ]  |
-//   +-----------------------------------------------+
-
-class TryChromeDialogView : public views::ButtonListener,
-                            public views::LinkListener {
+// Some variants do not have body text, or only have one button.
+class TryChromeDialogView : public views::ButtonListener {

[sky, 2017/06/23 17:45:52]

I would expect a class named View to be a View, but that isn't the case here.
Name TryChromeDialogController or perhaps TryChromeDialog.

[sky, 2017/06/23 17:45:52]

Is there a test for this class? At a minimum you should have an interactive
test. See
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro...
.

[skare_, 2017/06/24 01:08:59]

yes, there is a preexisting test, though it's the minimum:
src/chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc 
(now renamed in this cl)
I'll look at updating the test to run more than the 0-group experiment varient.

[skare_, 2017/06/24 01:09:00]

Named TryChromeDialog within this file, renamed the file. If this loses comment
context I can rename back until everything's resolved.

  public:
   // Receives a handle to the active modal dialog, or NULL when the active
   // dialog is dismissed.
@@ -62,25 +44,25 @@ class TryChromeDialogView : public views::ButtonListener,
       ActiveModalDialogListener;
 
   enum Result {
-    TRY_CHROME,             // Launch chrome right now.
-    TRY_CHROME_AS_DEFAULT,  // Launch chrome and make it the default.
-    NOT_NOW,                // Don't launch chrome. Exit now.
-    UNINSTALL_CHROME,       // Initiate chrome uninstall and exit.
-    DIALOG_ERROR,           // An error occurred creating the dialog.
+    OPEN_CHROME,   // Launch chrome right now.
+    NOT_NOW,       // Don't launch chrome. Exit now.
+    DIALOG_ERROR,  // An error occurred creating the dialog.
     COUNT
   };
 
   // Shows a modal dialog asking the user to give chrome another try. See
   // above for the possible outcomes of the function. This is an experimental,
   // non-localized dialog.
-  // |flavor| can be 0, 1, 2 or 3 and selects what strings to present.
+  // |group| selects what strings to present and what controls are shown.

[sky, 2017/06/23 17:45:53]

Can group be an enum, that helps document what the type is.

[skare_, 2017/06/24 01:09:00]

It's passed in as an integer through the experiment framework. It could be an
enum for clarify here, but would effectively be passed in as an int. 
It's also 14 groups that vary the dialog in relatively subtle ways, so naming
might be nonintuitive (see the spec slides linked in the .cc)
wdyt?
@nikunjb @grt, this comment FYI.

[grt (UTC plus 2), 2017/06/24 20:55:11]

kExperiments (in the .cc file) effectively describes each group by mapping a
group index to its UX-specific properties. while we could make an enum for the
group identifier and give them names, i'm not sure doing so would add much. the
name of a group alone doesn't tell the reader much.

if |group| by itself is too ambiguous, how about changing |group| here to
|group_index| so that it is at least clear that the value is an index into some
table somewhere?

[sky, 2017/06/26 16:12:05]

Agreed, keep it as an int.

   // |listener| will be notified when the dialog becomes active and when it is
   // dismissed.
   // Note that the dialog has no parent and it will position itself in a lower
-  // corner of the screen. The dialog does not steal focus and does not have an
-  // entry in the taskbar.
-  static Result Show(size_t flavor,
-                     const ActiveModalDialogListener& listener);
+  // corner of the screen or near the Chrome taskbar button.
+  // The dialog does not steal focus and does not have an entry in the taskbar.
+  static Result Show(size_t group,
+                     const ActiveModalDialogListener& listener,

[sky, 2017/06/23 17:45:52]

The callback should take the welcome_page as a value. That way there aren't any
worries about lifetime of a pointer passed into this function (especially for
the !modal case).

[skare_, 2017/06/24 01:09:00]

I'll change Result to carry this info.

[grt (UTC plus 2), 2017/06/24 20:55:11]

i don't understand the suggestion. by "the callback", do you mean the |listener|
argument?

+                     StartupBrowserCreator::WelcomeBackPage* welcome_page);
+
  private:
   // Indicates whether the dialog is modal
   enum class kDialogType {

[sky, 2017/06/23 17:45:52]

This is super confusing as enums should not have k in the name.

[skare_, 2017/06/24 01:09:00]

Done.

@@ -96,7 +78,7 @@ class TryChromeDialogView : public views::ButtonListener,
 
   friend class TryChromeDialogTest;
 
-  explicit TryChromeDialogView(size_t flavor);
+  explicit TryChromeDialogView(size_t group);

[sky, 2017/06/23 17:45:53]

Document what group is here.

[skare_, 2017/06/24 01:09:00]

Done.

   ~TryChromeDialogView() override;
 
   // Helper function to show the dialog.
@@ -114,30 +96,31 @@ class TryChromeDialogView : public views::ButtonListener,
   // does not compete with the standard place of the start menu.
   gfx::Rect ComputeWindowPosition(const gfx::Size& size, bool is_RTL);
 
-  // Create a windows region that looks like a toast of width |w| and height
-  // |h|. This is best effort, so we don't care much if the operation fails.
-  void SetToastRegion(HWND window, int w, int h);
-
   // views::ButtonListener:
   // We have two buttons and according to what the user clicked we set |result_|
   // and we should always close and end the modal loop.
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // views::LinkListener:
-  // If the user selects the link we need to fire off the default browser that
-  // by some convoluted logic should not be chrome.
-  void LinkClicked(views::Link* source, int event_flags) override;
+  // Controls which experiment group to use for varying the layout and controls.
+  size_t group_;

[sky, 2017/06/23 17:45:52]

const

[skare_, 2017/06/24 01:09:00]

Done.

+
+  // Font list used by text and buttons.
+  gfx::FontList font_list_;
+
+  // Bounds for Chrome icon on taskbar if pinned. Empty if not pinned.
+  // TODO(skare): Pull from https://codereview.chromium.org/2900653002
+  gfx::Rect chrome_taskbar_rect_;
 
-  // Controls which flavor of the heading text to use.
-  size_t flavor_;
+  // Time when the toast was displayed.
+  base::Time time_shown_;

[sky, 2017/06/23 17:45:52]

const.
And remember that time can go backwards. When measuring duration it's better to
use TimeTicks, which can't go backwards.

[skare_, 2017/06/24 01:09:00]

(const done, ->TimeTicks outstanding) - 

@nikunjb FYI - can we use TimeTicks in the experiment code? Or I can internally
and still send you a base::Time in the experiment reporting.

[sky, 2017/07/18 17:10:31]

Was this comment resolved? Again, I think you should be using timeticks for this
code, otherwise the time delta may be negative.

[skare_, 2017/07/21 03:11:35]

used ticks to compute the delta, Time::Now() for the single-point time-shown
requested by the experiment data interface.

 
-  // We don't own any of these pointers. The |popup_| owns itself and owns the
-  // other views.
+  // Unowned; |popup_| owns itself.
   views::Widget* popup_;
-  views::RadioButton* try_chrome_;
-  views::RadioButton* kill_chrome_;
-  views::RadioButton* dont_try_chrome_;
-  views::Checkbox* make_default_;
+
+  // Experiment feedback interface.
+  installer::ExperimentStorage storage_;
+
+  // Result of displaying the dialog: accepted, dismissed, etc.
   Result result_;
 
   DISALLOW_COPY_AND_ASSIGN(TryChromeDialogView);