[Android] Stub for account management screen.

chrome/browser/signin/signin_header_helper.cc

Index: chrome/browser/signin/signin_header_helper.cc
diff --git a/chrome/browser/signin/signin_header_helper.cc b/chrome/browser/signin/signin_header_helper.cc
index be6bcf41b65f59f22838ad9b34c788c346d3ca91..54696996ce818f25e71960cf629552de0a6a5fb3 100644
--- a/chrome/browser/signin/signin_header_helper.cc
+++ b/chrome/browser/signin/signin_header_helper.cc
@@ -16,6 +16,10 @@
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 
+#if defined(OS_ANDROID)
+#include "chrome/browser/android/signin/account_management_screen_helper.h"
+#endif  // defined(OS_ANDROID)
+
 namespace {
 
 const char kChromeConnectedHeader[] = "X-Chrome-Connected";
@@ -36,6 +40,8 @@ void ShowAvatarBubbleUIThread(int child_id, int route_id) {
   if (browser)
     browser->window()->ShowAvatarBubbleFromAvatarButton();
   // TODO(guohui): need to handle the case when avatar button is not available.
+#else  // defined(OS_ANDROID)
+  AccountManagementScreenHelper::OpenAccountManagementScreen();

[Andrew T Wilson (Slow), 2014/01/16 10:00:45]

Seems like a better way to do this is to define SigninHeaderHelperAndroid and
make ShowAvatarBubbleUIThread a virtual method which is implemented on the
Android side by opening the acct mgmt screen. You could then also get rid of
AccountManagementScreenHelper::OpenAccountManagementScreen() in favor of just
having the code inline in SigninHeaderHelperAndroid::ShowAvatarBubbleUIThread()
implementation.

[aruslan, 2014/01/16 15:26:13]

I have contemplated converting the code in this file to the actual class with
some sensibly defined responsibilities, but that seems to be an overkill.

If you strongly believe that the code in this file should be restructured to go
from the set of namespace-scoped functions to something instantiateable -- let's
involve gouhui@ (original author of this code).  I don't think extraction of one
function into a class etc etc would be worth it.

[guohui, 2014/01/16 19:11:01]

I don't have a strong preference myself, it really depends on the amount of
android specific code here. If it is more than a dozen of lines, or if we expect
it to grow in the future, then I agree with Andrew that it should be extracted
into platform-specific subclasses, otherwise ifdefine seems more appropriate.

[aruslan, 2014/01/16 19:24:57]

Currently that "dozen lines" of Android-specific code is already in a separate
Android-specific class ("AccountManagementScreenHelper").
Consequently, this file has only one line of Android-specific code and that is
not going to change anytime soon.

AFAIU, Andrew suggests that the free-standing namespace functions in this file
(and where necessary) should be refactored to form a cohesive SigninHeaderHelper
class with a pure virtual "ShowAvatarBubbleUIThread" function.

Then we could introduce two subclasses -- one for Android (where I'd move the
code from AccountManagementScreenHelper), and one for other platforms (where
we'd move the code from the current  ShowAvatarBubbleUIThread).

I'd say it is an overkill unless we'd have to do this anyway for example to
support testing or something.

WDYT?

[guohui, 2014/01/16 19:48:37]

if you are confident that this single line is the only android specific code we
need in this class in a foreseeable future, then i do think it is an overkill to
extract it into a separate class. And we could always do refactoring as
suggested by Andrew later when there is a real need.

 #endif // OS_ANDROID
 }