Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1234063002:
[Smart Lock] Auto sign-in snackbar UI. (Closed)
Description[Smart Lock] Auto sign-in snackbar UI.
Web site can use credential manager API in order to get user credentials from password store. So in some cases user will be auto signed in to the web site. This commit introduces the UI for the Clank, which will inform the users that they were automatically signed in.
BUG=454815
Committed: https://crrev.com/abfc5f025e187896f68b7183064ac3a5a662ee7e
Cr-Commit-Position: refs/heads/master@{#344860}
Patch Set 1 : WIP. Do not review. #Patch Set 2 : WIP. Do not review. Change how snackbar is called. #Patch Set 3 : WIP. Do not review. Setting button in snackbar was removed and changes were rebased on top of maste… #Patch Set 4 : #
Total comments: 14
Patch Set 5 : WIP #Patch Set 6 : #
Total comments: 1
Patch Set 7 : Rebased on top of master #
Total comments: 18
Patch Set 8 : #
Total comments: 4
Patch Set 9 : Adressed comments #Patch Set 10 : Dot in the licence disclaimer. #
Total comments: 10
Patch Set 11 : Adressed comments #
Total comments: 2
Patch Set 12 : git grep -l 'RegisterAutoSigninSnackbarControllerHelper' | xargs sed -i 's/RegisterAutoSigninSnackb… #Messages
Total messages: 39 (17 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
melandory@chromium.org changed reviewers: + newt@chromium.org
I have a question about approach I implemented, but I don't want to submit code for the review yet. I'm creating an instance of AutoSignInSnackbarController in ChromeActivity.java, then on the event which is happening in the native code, I want to show a snackbar (call method show on instance of AutoSignInSnackbarController). One approach I considered was to add @CalledByNative method showAutoSigninToast(...) to ChromeActivity. And then call this method from native code. But it seems to me that showAutoSigninToast doesn't feet to ChromeActivity API. Approach I implemented is: I created a class AutoSigninToast, which holds a ref to the AutoSignInSnackbarController and calls show on it. Can you advice on what is a proper solution? Thanks in advance.
Sorry, I meant to look at this earlier. Here are my thoughts: Don't use a static variable to temporarily store a reference. i.e. don't keep the code in AutoSigninToast. This will cause bugs in multi-instance mode, and is very prone to memory leaks (for example, the current code appears to leak the entire activity via the Context). Does manage_passwords_ui_controller have a reference to the ChromeActivity? If so, calling ChromeActivity.showAutoSigninSnackbar() is reasonable. Otherwise, you can go through WebContents and Tab, e.g. TabAndroid::FromWebContents(web_contents())->ShowAutoSigninSnackbar() Also, side-note: Use "snackbar" consistently; not "toast" or "infobar". (A "toast" doesn't have an action button and is created in a different way. "Infobars" are a totally different beast). See http://www.google.com/design/spec/components/snackbars-toasts.html
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Hi Newton, PTAL. Thanks!
Two small notes: - Please replace all occurrences of "toast" and "infobar" with snackbar. There are still a few remaining, including in the commit message. - Update the commit message to explain what the snackbar says and when it appears. Someone reading just the commit message should have a clear high-level understanding of the CL. Larger comment: I think we need a better path between manage_passwords_ui_controller.cc and AutoSigninSnackbarController. The current path touches classes that really shouldn't need to know about auto sign-in snackbars: ChromeActivity, Tab, and especially TabObserver. Here's an idea. Add a method getSnackbarManager() to Tab (which just returns mActivity.getSnackbarManager()). manage_passwords_ui_controller.cc can call into AutoSigninSnackbarController.create() (which will need a C++ side) and pass it the Tab. AutoSigninSnackbarController.create() will call tab.getSnackbarManager().showSnackbar(). If you want the snackbar to disappear when the tab is hidden or destroyed, then the AutoSigninSnackbarController can attach a temporary TabObserver to listen to those events (be sure to remove the TabObserver when the snackbar is dismissed). https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:41: public void show(Object credential) { s/Object/Credential https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:43: mSnackbarManager.showSnackbar( All this indentation is a sign that maybe you should split this into multiple statements, e.g. Snackbar snackbar = Snackbar.make(account.getUsername(), this) .setTemplateText(mContext.getString(R.string.passwords_auto_signin_description)); mSnackbarManager.showSnackbar(snackbar); https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:285: Signed in as <ph name="USERNAME">%1$s<ex>don.john.lemon@gmail.com</ex></ph> indent this two more spaces to match the other strings in this file. Also indent the string below by 1 more space. https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:287: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown when user auto signins to site"> "when the user is automatically signed in to a site" https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:288: Automatically sign in to websites using stored credentials. When the feature is off, you'll be asked for verification every time before signing in to a website. Swap this message with the above message? The message descriptions and names don't match the contents currently. https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102: credential = CreateNativeCredential( Instead of passing the Credential object to Java (and incurring the extra JNI overhead of doing so), how about just passing the username? https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.h:203: virtual void UpdateAutoSigninToastVisibility(); comment
On 2015/08/13 16:48:04, newt wrote: > Two small notes: > - Please replace all occurrences of "toast" and "infobar" with snackbar. There > are still a few remaining, including in the commit message. > - Update the commit message to explain what the snackbar says and when it > appears. Someone reading just the commit message should have a clear high-level > understanding of the CL. > > Larger comment: > > I think we need a better path between manage_passwords_ui_controller.cc and > AutoSigninSnackbarController. The current path touches classes that really > shouldn't need to know about auto sign-in snackbars: ChromeActivity, Tab, and > especially TabObserver. > > Here's an idea. Add a method getSnackbarManager() to Tab (which just returns > mActivity.getSnackbarManager()). manage_passwords_ui_controller.cc can call into > AutoSigninSnackbarController.create() (which will need a C++ side) and pass it > the Tab. AutoSigninSnackbarController.create() will call > tab.getSnackbarManager().showSnackbar(). I also need a context in AutoSigninSnackbarController: I can also define getContext method in Tab or is there a better approach to get it?
On 2015/08/18 20:32:45, melandory wrote: > On 2015/08/13 16:48:04, newt wrote: > > Two small notes: > > - Please replace all occurrences of "toast" and "infobar" with snackbar. > There > > are still a few remaining, including in the commit message. > > - Update the commit message to explain what the snackbar says and when it > > appears. Someone reading just the commit message should have a clear > high-level > > understanding of the CL. > > > > Larger comment: > > > > I think we need a better path between manage_passwords_ui_controller.cc and > > AutoSigninSnackbarController. The current path touches classes that really > > shouldn't need to know about auto sign-in snackbars: ChromeActivity, Tab, and > > especially TabObserver. > > > > Here's an idea. Add a method getSnackbarManager() to Tab (which just returns > > mActivity.getSnackbarManager()). manage_passwords_ui_controller.cc can call > into > > AutoSigninSnackbarController.create() (which will need a C++ side) and pass it > > the Tab. AutoSigninSnackbarController.create() will call > > tab.getSnackbarManager().showSnackbar(). > > I also need a context in AutoSigninSnackbarController: I can also define > getContext method in Tab or is there a better approach to get it? More concrete: make getApplicationContext method public, not private.
On 2015/08/19 15:14:03, melandory wrote: > On 2015/08/18 20:32:45, melandory wrote: > > On 2015/08/13 16:48:04, newt wrote: > > > Two small notes: > > > - Please replace all occurrences of "toast" and "infobar" with snackbar. > > There > > > are still a few remaining, including in the commit message. > > > - Update the commit message to explain what the snackbar says and when it > > > appears. Someone reading just the commit message should have a clear > > high-level > > > understanding of the CL. > > > > > > Larger comment: > > > > > > I think we need a better path between manage_passwords_ui_controller.cc and > > > AutoSigninSnackbarController. The current path touches classes that really > > > shouldn't need to know about auto sign-in snackbars: ChromeActivity, Tab, > and > > > especially TabObserver. > > > > > > Here's an idea. Add a method getSnackbarManager() to Tab (which just returns > > > mActivity.getSnackbarManager()). manage_passwords_ui_controller.cc can call > > into > > > AutoSigninSnackbarController.create() (which will need a C++ side) and pass > it > > > the Tab. AutoSigninSnackbarController.create() will call > > > tab.getSnackbarManager().showSnackbar(). > > > > I also need a context in AutoSigninSnackbarController: I can also define > > getContext method in Tab or is there a better approach to get it? > > More concrete: make getApplicationContext method public, not private. Tab.getWindowAndroid().getApplicationContext() should work for this.
Patchset #5 (id:180001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Hey Newton, PTAL. Thanks! https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:41: public void show(Object credential) { On 2015/08/13 16:48:04, newt wrote: > s/Object/Credential Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:43: mSnackbarManager.showSnackbar( On 2015/08/13 16:48:04, newt wrote: > All this indentation is a sign that maybe you should split this into multiple > statements, e.g. > > Snackbar snackbar = Snackbar.make(account.getUsername(), this) > > .setTemplateText(mContext.getString(R.string.passwords_auto_signin_description)); > mSnackbarManager.showSnackbar(snackbar); Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:285: Signed in as <ph name="USERNAME">%1$s<ex>don.john.lemon@gmail.com</ex></ph> On 2015/08/13 16:48:04, newt wrote: > indent this two more spaces to match the other strings in this file. Also indent > the string below by 1 more space. Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:287: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown when user auto signins to site"> On 2015/08/13 16:48:04, newt wrote: > "when the user is automatically signed in to a site" Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:288: Automatically sign in to websites using stored credentials. When the feature is off, you'll be asked for verification every time before signing in to a website. On 2015/08/13 16:48:04, newt wrote: > Swap this message with the above message? The message descriptions and names > don't match the contents currently. Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102: credential = CreateNativeCredential( On 2015/08/13 16:48:04, newt wrote: > Instead of passing the Credential object to Java (and incurring the extra JNI > overhead of doing so), how about just passing the username? Done. https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right): https://codereview.chromium.org/1234063002/diff/160001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.h:203: virtual void UpdateAutoSigninToastVisibility(); On 2015/08/13 16:48:04, newt wrote: > comment Done.
https://codereview.chromium.org/1234063002/diff/300001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/1234063002/diff/300001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:107: AutoSigninSnackbarController* controller = new AutoSigninSnackbarController( I'm not sure about this solution. What I can do alternatively: make ManagePasswordsUIController owner for AutoSigninSnackbarController*. WDYT?
https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:31: public static AutoSigninSnackbarController create(Tab tab, long nativePtr) { Make this private. Same for the other methods in this class that are only called by the native side of this object. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:42: mSnackbarManager = (SnackbarManager) tab.getSnackbarManager(); don't use cast; fix the return type of getSnackbarManager() instead. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:77: public void onAction(Object actionData) {} group all the SnackbarController methods together https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:96: private void onShutDown() { I'd call this "onDismiss". "shutdown" typically refers to the entire program shutting down. "dismiss" is the common terminology for windows, snackbars, or other UI elements going away. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2245: * Returns the Snackbar Manager. "Returns the SnackbarManager for the activity that owns this Tab, if any. May return null." https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2247: public Object getSnackbarManager() { return type should be SnackbarManager, not object. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:284: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown when user auto signins to site"> "... when the user is automatically signed in to a site" https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller.h (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller.h:19: class AutoSigninSnackbarController { It appears you don't actually need a native AutoSigninSnackbarController object. You could just have a single static method in C++: void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username) { // call a static Java method to show the snackbar } Then in Java, you'd have a method @CalledByNative private void showAutoSigninSnackbar(Tab tab, String username) { // create a controller and show the snackbar } This would simplify things and avoid the tricky Java/C++ memory management. https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:94: void TestManagePasswordsUIController::UpdateAutoSigninSnackbarVisibility() { Which test(s) are actually testing this feature?
Patchset #8 (id:340001) has been deleted
melandory@chromium.org changed reviewers: + miguelg@chromium.org
miguelg@chromium.org: Please review changes in chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc Thanks! https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:31: public static AutoSigninSnackbarController create(Tab tab, long nativePtr) { On 2015/08/21 05:40:39, newt wrote: > Make this private. Same for the other methods in this class that are only called > by the native side of this object. Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:42: mSnackbarManager = (SnackbarManager) tab.getSnackbarManager(); On 2015/08/21 05:40:39, newt wrote: > don't use cast; fix the return type of getSnackbarManager() instead. Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:77: public void onAction(Object actionData) {} On 2015/08/21 05:40:39, newt wrote: > group all the SnackbarController methods together Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:96: private void onShutDown() { On 2015/08/21 05:40:39, newt wrote: > I'd call this "onDismiss". "shutdown" typically refers to the entire program > shutting down. "dismiss" is the common terminology for windows, snackbars, or > other UI elements going away. Don't need the method anymore, but thank for clarification. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2245: * Returns the Snackbar Manager. On 2015/08/21 05:40:39, newt wrote: > "Returns the SnackbarManager for the activity that owns this Tab, if any. May > return null." Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2247: public Object getSnackbarManager() { On 2015/08/21 05:40:39, newt wrote: > return type should be SnackbarManager, not object. Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:284: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown when user auto signins to site"> On 2015/08/21 05:40:39, newt wrote: > "... when the user is automatically signed in to a site" Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller.h (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller.h:19: class AutoSigninSnackbarController { On 2015/08/21 05:40:39, newt wrote: > It appears you don't actually need a native AutoSigninSnackbarController object. > You could just have a single static method in C++: > > void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username) { > // call a static Java method to show the snackbar > } > > Then in Java, you'd have a method > > @CalledByNative > private void showAutoSigninSnackbar(Tab tab, String username) { > // create a controller and show the snackbar > } > > This would simplify things and avoid the tricky Java/C++ memory management. Done. https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc (right): https://codereview.chromium.org/1234063002/diff/320001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:94: void TestManagePasswordsUIController::UpdateAutoSigninSnackbarVisibility() { On 2015/08/21 05:40:39, newt wrote: > Which test(s) are actually testing this feature? The method is called when similar UI for desktop appears. But actually, I realized that creation of androis UI doesn't belong to ManagePasswordsUIController, I'll move it up to the call chain.
melandory@chromium.org changed reviewers: + engedy@chromium.org
engedy@chromium.org: Please review changes in chrome/browser/password_manager/chrome_password_manager_client.cc
chrome/browser/password_manager/chrome_password_manager_client.cc LGTM.
Patchset #8 (id:360001) has been deleted
lgtm % nits for chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc (right): https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc:12: void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username) { same nit abut the & here https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h (right): https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h:16: void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username); nit, move the & near to base::string16 Also can you document what exactly username is? email? a random string?
https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc (right): https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.cc:12: void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username) { On 2015/08/21 10:44:52, Miguel Garcia wrote: > same nit abut the & here Done. https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h (right): https://codereview.chromium.org/1234063002/diff/380001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h:16: void ShowAutoSigninSnackbar(TabAndroid *tab, const base::string16 &username); On 2015/08/21 10:44:52, Miguel Garcia wrote: > nit, move the & near to base::string16 > > Also can you document what exactly username is? email? a random string? Done.
Thanks. I like the structure of this now :) A few last comments. https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:65: if (mSnackbarManager != null) { If mSnackbarManager, then the AutoSigninSnackbarController won't be shown, and hence onDismissForEachType() will never be called, and the AutoSigninSnackbarController will stick around indefinitely as a TabObserver. I'd structure the class like this to avoid this problem: private static void showSnackbar(Tab tab, String username) { SnackbarManager snackbarManager = tab.getSnackbarManager(); if (snackbarManager == null) return; AutoSigninSnackbarController controller = new AutoSigninSnackbarController(snackbarManager, tab); Context context = mTab.getWindowAndroid().getApplicationContext(); String text = context.getString(R.string.passwords_auto_signin_message); snackbarManager.showSnackbar(Snackbar.make(username, controller).setTemplateText(text)); } https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:281: <message name="IDS_PASSWORDS_AUTO_SIGNIN_TITLE" desc="Title for checkbox to enable automatically signing the user in to a websites"> remove "a" https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:284: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown whenthe user is automatically signed in to a website"> s/whenthe/when the https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:246: ShowAutoSigninSnackbar(tab, local_forms[0]->username_value); Looks like you're no longer showing this snackbar for federated_forms. Is that correct behavior? (I don't know much about this code; I just noticed that this changed from earlier patchsets) https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h:5: #ifndef CHROME_BROWSER_UI_ANDROID_SNACKBARS_AUTO_SIGNIN_SNACKBAR_CONTROLLER_HELPER_H_ How about naming this auto_signin_snackbar.h or auto_signin_snackbar_controller.h? That seems sufficiently descriptive without being a mouthful. auto_signin_snackbar_controller.h is nice because it has the same name as the Java class, so it's easy for developers to figure out which cc files goes with the java file and vice versa.
Hi Newton, I've addressed last comments, PTAL. Thanks! https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/snackbar/smartlockautosignin/AutoSigninSnackbarController.java:65: if (mSnackbarManager != null) { On 2015/08/21 16:47:32, newt wrote: > If mSnackbarManager, then the AutoSigninSnackbarController won't be shown, and > hence onDismissForEachType() will never be called, and the > AutoSigninSnackbarController will stick around indefinitely as a TabObserver. > > I'd structure the class like this to avoid this problem: > > private static void showSnackbar(Tab tab, String username) { > SnackbarManager snackbarManager = tab.getSnackbarManager(); > if (snackbarManager == null) return; > > AutoSigninSnackbarController controller = new > AutoSigninSnackbarController(snackbarManager, tab); > Context context = mTab.getWindowAndroid().getApplicationContext(); > String text = context.getString(R.string.passwords_auto_signin_message); > snackbarManager.showSnackbar(Snackbar.make(username, > controller).setTemplateText(text)); > } Done. https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:281: <message name="IDS_PASSWORDS_AUTO_SIGNIN_TITLE" desc="Title for checkbox to enable automatically signing the user in to a websites"> On 2015/08/21 16:47:32, newt wrote: > remove "a" Done. https://codereview.chromium.org/1234063002/diff/420001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:284: <message name="IDS_PASSWORDS_AUTO_SIGNIN_MESSAGE" desc="Message shown whenthe user is automatically signed in to a website"> On 2015/08/21 16:47:32, newt wrote: > s/whenthe/when the Done. https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:246: ShowAutoSigninSnackbar(tab, local_forms[0]->username_value); On 2015/08/21 16:47:32, newt wrote: > Looks like you're no longer showing this snackbar for federated_forms. Is that > correct behavior? (I don't know much about this code; I just noticed that this > changed from earlier patchsets) Previously it was called it different place, where I had to check both normal and federated credentials. Here I just get a credentials which will be used for auto sign-in: https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h (right): https://codereview.chromium.org/1234063002/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/snackbars/auto_signin_snackbar_controller_helper.h:5: #ifndef CHROME_BROWSER_UI_ANDROID_SNACKBARS_AUTO_SIGNIN_SNACKBAR_CONTROLLER_HELPER_H_ On 2015/08/21 16:47:32, newt wrote: > How about naming this auto_signin_snackbar.h or > auto_signin_snackbar_controller.h? That seems sufficiently descriptive without > being a mouthful. > > auto_signin_snackbar_controller.h is nice because it has the same name as the > Java class, so it's easy for developers to figure out which cc files goes with > the java file and vice versa. Done.
lgtm after comment :) https://codereview.chromium.org/1234063002/diff/440001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1234063002/diff/440001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:198: RegisterAutoSigninSnackbarControllerHelper}, remove "Helper" from these
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org, engedy@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1234063002/#ps460001 (title: "git grep -l 'RegisterAutoSigninSnackbarControllerHelper' | xargs sed -i 's/RegisterAutoSigninSnackb…")
https://codereview.chromium.org/1234063002/diff/440001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1234063002/diff/440001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:198: RegisterAutoSigninSnackbarControllerHelper}, On 2015/08/21 19:21:46, newt wrote: > remove "Helper" from these Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234063002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1234063002/460001
Message was sent while issue was closed.
Committed patchset #12 (id:460001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/abfc5f025e187896f68b7183064ac3a5a662ee7e Cr-Commit-Position: refs/heads/master@{#344860} |
