Inactive toast ux changes

chrome/browser/ui/views/try_chrome_dialog_view.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_TRY_CHROME_DIALOG_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_TRY_CHROME_DIALOG_VIEW_H_
 
 #include <stddef.h>
 
 #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.
+//
+// The layout is as follows:
 //
 //   +-----------------------------------------------+
-//   | |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?_                |
+//   | |icon| Header text.                       [x] |
+//   |                                               |
+//   |        Body text.                             |
+//   |        [ Open Chrome ] [No Thanks]            |
 //   +-----------------------------------------------+
 //
-// 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 ]                       |
-//   +-----------------------------------------------+
-//
-// 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.
   typedef base::Callback<void(gfx::NativeWindow active_dialog)>
       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.

     MODAL,     // Modal dialog.
     MODELESS,  // Modeless dialog.
   };
 
   // Indicates the usage type. Chrome or tests.
   enum class kUsageType {
     FOR_CHROME,
     FOR_TESTING,
   };
 
   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.
   // The |dialog_type| parameter indicates whether the dialog is modal.
   // The |usage_type| parameter indicates whether this is being invoked by
   // chrome or a test.
   Result ShowDialog(const ActiveModalDialogListener& listener,
                     kDialogType dialog_type,
                     kUsageType usage_type);
 
   // Returns a screen rectangle that is fit to show the window. In particular
   // it has the following properties: a) is visible and b) is attached to the
   // bottom of the working area. For LTR machines it returns a left side
   // rectangle and for RTL it returns a right side rectangle so that the dialog
   // 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.

 
-  // Controls which flavor of the heading text to use.
-  size_t flavor_;
+  // Font list used by text and buttons.
+  gfx::FontList font_list_;
 
-  // We don't own any of these pointers. The |popup_| owns itself and owns the
-  // other views.
+  // 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_;
+
+  // 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.

+
+  // 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);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_TRY_CHROME_DIALOG_VIEW_H_