Bring the feedback button to the Mac sad tab

chrome/browser/ui/sad_tab.h

Index: chrome/browser/ui/sad_tab.h
diff --git a/chrome/browser/ui/sad_tab.h b/chrome/browser/ui/sad_tab.h
index e22b14948072cf051583412bce85faf415b8bf81..fee977fca727b1b8f78257c7f73d21cb50d72b13 100644
--- a/chrome/browser/ui/sad_tab.h
+++ b/chrome/browser/ui/sad_tab.h
@@ -17,6 +17,11 @@ namespace chrome {
 // Cross-platform interface to show the Sad tab UI.
 class SadTab {
  public:
+  enum class Action {
+    BUTTON,
+    HELP_LINK,
+  };
+
   // Factory function to create the platform specific implementations.
   static SadTab* Create(content::WebContents* web_contents, SadTabKind kind);
 
@@ -25,11 +30,29 @@ class SadTab {
 
   virtual ~SadTab() {}
 
-  // Shows the Sad tab.
-  virtual void Show() = 0;
+  int GetTitle();

[James Cook, 2016/09/08 20:15:50]

nit: Document these functions, especially what the returned int means.

[Sidney San Martín, 2016/09/09 01:36:00]

Done, let me know how these comments look.

[James Cook, 2016/09/09 04:37:09]

Very helpful, especially the note that they vary.

+  int GetMessage();
+  int GetButtonTitle();
+  int GetHelpLinkTitle();
+  const char* GetHelpLinkURL();
+
+  // Virtual for testing.
+  virtual void RecordFirstPaint();
+  virtual void PerformAction(Action);
+
+ protected:
+  SadTab(content::WebContents* web_contents, SadTabKind kind);
+
+ private:
+  content::WebContents* web_contents_;
+  SadTabKind kind_;
+  bool want_feedback_;

[James Cook, 2016/09/08 20:15:50]

nit: Document and/or rename. Who wants feedback about what? Is this really
|show_feedback_link_|?

[Sidney San Martín, 2016/09/09 01:36:00]

We, the developers, want feedback about what caused the sad tab.

I don't love want_feedback_, but I avoided names like show_feedback_link_
because there are other differences (the help link changes, too). Looking at it
now, I agree that this name is extra confusing, so I changed it to
show_feedback_button_. Can you think of something better?

+
+#if DCHECK_IS_ON()

[James Cook, 2016/09/08 20:15:50]

Does this need #if guards? Is it for unused variable warnings? If not, just
record it all the time -- skipping a single assignment isn't worth the decrease
in readability.

[Sidney San Martín, 2016/09/09 01:36:00]

Huh, I thought it improved readability: you're allowed to assume that anything
inside the guard doesn't affect Chrome's behavior (but may offer hints about how
it *should* behave). If that's off base, I can nuke it.

[James Cook, 2016/09/09 04:37:09]

I found it harder to read. Frankly, I've been in the chrome code for 5 years and
I didn't know this macro existed. It's weird because looks like a method call,
as opposed to all our other #if defined(CONDITION) macros. It's obvious what it
means, but it makes me think you're skipping some big block of validation code,
but here it's just setting a bool.

It's also a little odd that recorded_paint_ is initialized here, but
show_feedback_button_ is not.

[Sidney San Martín, 2016/09/09 15:39:44]

Done. I also find it strange that this is a macro function.

[Sidney San Martín, 2016/09/09 15:39:44]

Done. recorded_paint_ always starts false, and show_feedback_button_ (and
everything else in this class) starts with a dynamic value… but I buy that it
feels odd.

[Nico, 2016/09/09 15:45:30]

FWIW this is because if it were a macro and you forgot to include logging.h,
then `#if DCHECK_IS_ON` will silently do the wrong thing.

[Sidney San Martín, 2016/09/09 15:47:18]

Ah, thanks. That makes sense.

+  bool recorded_paint_ = false;
+#endif
 
-  // Closes the Sad tab.
-  virtual void Close() = 0;
+  DISALLOW_COPY_AND_ASSIGN(SadTab);
 };
 
 }  // namespace chrome