Bring the feedback button to the Mac sad tab

chrome/browser/ui/sad_tab.h

 // Copyright 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_SAD_TAB_H_
 #define CHROME_BROWSER_UI_SAD_TAB_H_
 
 #include "base/process/kill.h"
 #include "chrome/browser/ui/sad_tab_types.h"
 
 namespace content {
 class WebContents;
 }
 
 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);
 
   // Returns true if the sad tab should be shown.
   static bool ShouldShow(base::TerminationStatus status);
 
   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();
 
-  // Closes the Sad tab.
-  virtual void Close() = 0;
+  // 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
+
+  DISALLOW_COPY_AND_ASSIGN(SadTab);
 };
 
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_UI_SAD_TAB_H_