Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132033005:
[Android] Stub for account management screen. (Closed)
|
Created:
6 years, 11 months ago by aruslan Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Stub for account management screen.
BUG=316637
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245631
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Renames. #Patch Set 4 : Review comments addressed. #
Total comments: 8
Patch Set 5 : Grrr. #
Total comments: 8
Messages
Total messages: 20 (0 generated)
PTAL thanks!
https://codereview.chromium.org/132033005/diff/30001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java (right): https://codereview.chromium.org/132033005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2014 now :-) https://codereview.chromium.org/132033005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:15: public class AccountManagementScreen { Some suggestions about naming. Maybe AccountManagementScreenHelper and maybe AccountManagementScreenManager or something for the interface. This makes me think that it should be a UI component. And Impl on the interface seems like it should be implementing something. Really, the one who implements the interface should be the Impl (i.e. AccountManagementScreenManagerImpl or some ridiculously long name). Kind of has a trickle effect across the change. https://codereview.chromium.org/132033005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:17: private static AccountManagementScreenImpl sImplementation = null; = null is not necessary. https://codereview.chromium.org/132033005/diff/30001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:41: if (sImplementation == null) return; Should we have an assert false here to ensure we don't miss this places? Or is returning null fine and it will use the fallback behavior? Just wondering if we should be tracking this somehow.
Thanks, Ted, PTAL. https://chromiumcodereview.appspot.com/132033005/diff/30001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java (right): https://chromiumcodereview.appspot.com/132033005/diff/30001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/14 20:29:13, Ted C wrote: > 2014 now :-) Done. https://chromiumcodereview.appspot.com/132033005/diff/30001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:15: public class AccountManagementScreen { On 2014/01/14 20:29:13, Ted C wrote: > Some suggestions about naming. > > Maybe AccountManagementScreenHelper and maybe AccountManagementScreenManager or > something for the interface. > > This makes me think that it should be a UI component. And Impl on the interface > seems like it should be implementing something. Really, the one who implements > the interface should be the Impl (i.e. AccountManagementScreenManagerImpl or > some ridiculously long name). > > Kind of has a trickle effect across the change. Done. https://chromiumcodereview.appspot.com/132033005/diff/30001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:17: private static AccountManagementScreenImpl sImplementation = null; On 2014/01/14 20:29:13, Ted C wrote: > = null is not necessary. Done. https://chromiumcodereview.appspot.com/132033005/diff/30001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreen.java:41: if (sImplementation == null) return; On 2014/01/14 20:29:13, Ted C wrote: > Should we have an assert false here to ensure we don't miss this places? Or is > returning null fine and it will use the fallback behavior? > > Just wondering if we should be tracking this somehow. It is safe to ignore the call if an implementation isn't provided.
lgtm w/ some naming/comment nits https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java (right): https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:20: * Defines the account management screen interface to be implemented by the implementors. Update? https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:31: * Sets the implementation of the account management UI. Sets the "manager" ... https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:32: * @param manager An implementation of the AccountManagementScreenImpl interface. Update class name https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:34: public static void setImplementation(AccountManagementScreenManager manager) { setManager seems better now
https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java (right): https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:20: * Defines the account management screen interface to be implemented by the implementors. On 2014/01/15 01:46:20, Ted C wrote: > Update? Done. https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:31: * Sets the implementation of the account management UI. On 2014/01/15 01:46:20, Ted C wrote: > Sets the "manager" ... Done. https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:32: * @param manager An implementation of the AccountManagementScreenImpl interface. On 2014/01/15 01:46:20, Ted C wrote: > Update class name Done. https://chromiumcodereview.appspot.com/132033005/diff/180001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountManagementScreenHelper.java:34: public static void setImplementation(AccountManagementScreenManager manager) { On 2014/01/15 01:46:20, Ted C wrote: > setManager seems better now Done.
Andrew -- please review chrome/browser/signin/ changes and apply the OWNERS seal if appropriate, thanks!
https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... File chrome/browser/android/signin/account_management_screen_helper.h (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... chrome/browser/android/signin/account_management_screen_helper.h:16: static bool Register(JNIEnv* env); This is a little odd (defining a class with only static methods). Perhaps this should just be in a namespace or something? https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... File chrome/browser/signin/signin_header_helper.cc (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... chrome/browser/signin/signin_header_helper.cc:44: AccountManagementScreenHelper::OpenAccountManagementScreen(); 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.
https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... File chrome/browser/android/signin/account_management_screen_helper.h (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... chrome/browser/android/signin/account_management_screen_helper.h:16: static bool Register(JNIEnv* env); On 2014/01/16 10:00:45, Andrew T Wilson wrote: > This is a little odd (defining a class with only static methods). Perhaps this > should just be in a namespace or something? I haven't found cases of namespace-based implementations, and it seems to be always implemented through class+statics: http://src.chromium.org/viewvc/chrome/trunk/src/base/android/memory_pressure_... Let me talk with Yaron to find out if anyone cares -- personally I prefer the namespaces. https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... File chrome/browser/signin/signin_header_helper.cc (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... chrome/browser/signin/signin_header_helper.cc:44: AccountManagementScreenHelper::OpenAccountManagementScreen(); On 2014/01/16 10:00:45, Andrew T Wilson wrote: > 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. 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.
On 2014/01/16 15:26:13, aruslan wrote: > https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... > File chrome/browser/android/signin/account_management_screen_helper.h (right): > > https://codereview.chromium.org/132033005/diff/240001/chrome/browser/android/... > chrome/browser/android/signin/account_management_screen_helper.h:16: static bool > Register(JNIEnv* env); > On 2014/01/16 10:00:45, Andrew T Wilson wrote: > > This is a little odd (defining a class with only static methods). Perhaps this > > should just be in a namespace or something? > > I haven't found cases of namespace-based implementations, and it seems to be > always implemented through class+statics: > http://src.chromium.org/viewvc/chrome/trunk/src/base/android/memory_pressure_... > > Let me talk with Yaron to find out if anyone cares -- personally I prefer the > namespaces. > > https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... > File chrome/browser/signin/signin_header_helper.cc (right): > > https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... > chrome/browser/signin/signin_header_helper.cc:44: > AccountManagementScreenHelper::OpenAccountManagementScreen(); > On 2014/01/16 10:00:45, Andrew T Wilson wrote: > > 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. > > 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. I have a mild preference for moving this code entirely into an Android-specific subclass of SigninHeaderHelper, but I'm not overly dogmatic. Let's hear what gouhui thinks.
Hi Hui -- PTAL at signin_header_helper.cc and Andrew's comments and let us know what you think and what might be the best way to extract platform-neutral and platform-specific bits of the code. BTW, how do we test it? Thanks!
https://chromiumcodereview.appspot.com/132033005/diff/240001/chrome/browser/a... File chrome/browser/android/signin/account_management_screen_helper.h (right): https://chromiumcodereview.appspot.com/132033005/diff/240001/chrome/browser/a... chrome/browser/android/signin/account_management_screen_helper.h:16: static bool Register(JNIEnv* env); On 2014/01/16 15:26:13, aruslan wrote: > On 2014/01/16 10:00:45, Andrew T Wilson wrote: > > This is a little odd (defining a class with only static methods). Perhaps this > > should just be in a namespace or something? > > I haven't found cases of namespace-based implementations, and it seems to be > always implemented through class+statics: > http://src.chromium.org/viewvc/chrome/trunk/src/base/android/memory_pressure_... > > Let me talk with Yaron to find out if anyone cares -- personally I prefer the > namespaces. Yaron said it will be slightly inconsistent with the rest of the code, but no one really cares. However, I've just realized that class->namespace means that the name becomes underscored_c_style, and that definitely looks weird and inconsistent in the JNI registration code. I would respectfully keep it in a class if you don't mind.
https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... File chrome/browser/signin/signin_header_helper.cc (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... chrome/browser/signin/signin_header_helper.cc:44: AccountManagementScreenHelper::OpenAccountManagementScreen(); 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.
https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... File chrome/browser/signin/signin_header_helper.cc (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... chrome/browser/signin/signin_header_helper.cc:44: AccountManagementScreenHelper::OpenAccountManagementScreen(); On 2014/01/16 19:11:01, guohui wrote: > 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. 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?
lgtm https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... File chrome/browser/signin/signin_header_helper.cc (right): https://codereview.chromium.org/132033005/diff/240001/chrome/browser/signin/s... chrome/browser/signin/signin_header_helper.cc:44: AccountManagementScreenHelper::OpenAccountManagementScreen(); On 2014/01/16 19:24:57, aruslan wrote: > On 2014/01/16 19:11:01, guohui wrote: > > 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. > > 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? 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.
Thanks, Hui. Andrew -- do you feel it's a go?
On 2014/01/16 23:29:11, aruslan wrote: > Thanks, Hui. > Andrew -- do you feel it's a go? LGTM given the comments above.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/132033005/240001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/132033005/240001
Message was sent while issue was closed.
Change committed as 245631 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
