Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
967193002:
Credential saving clank infobar for Smart Lock. (Closed)
|
Created:
5 years, 9 months ago by melandory Modified:
5 years, 9 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reusable_more_button Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCredential saving clank infobar for Smart Lock.
In case infobar is triggered by credential management API
infobar should have different appearance.
BUG=454815
Committed: https://crrev.com/e508b5d19b3bb82afc4d11885c8ab1c18192ec3b
Cr-Commit-Position: refs/heads/master@{#319892}
Patch Set 1 #Patch Set 2 : #
Total comments: 42
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #Messages
Total messages: 23 (7 generated)
Hi all, review this CL, please. mkwst@: chrome/browser/password_manager/chrome_password_manager_client.cc chrome/browser/password_manager/save_password_infobar_delegate.h chrome/browser/password_manager/save_password_infobar_delegate.cc bauerb@: chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java dfalcantara@: chrome/browser/ui/android/infobars/save_password_infobar.h chrome/browser/ui/android/infobars/save_password_infobar.cc sky@: chrome/app/generated_resources.grd chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi
melandory@chromium.org changed reviewers: + bauerb@chromium.org, dfalcantara@chromium.org, mkwst@chromium.org, sky@chromium.org
Hi all, review this CL, please. mkwst@: chrome/browser/password_manager/chrome_password_manager_client.cc chrome/browser/password_manager/save_password_infobar_delegate.h chrome/browser/password_manager/save_password_infobar_delegate.cc bauerb@: chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java dfalcantara@: chrome/browser/ui/android/infobars/save_password_infobar.h chrome/browser/ui/android/infobars/save_password_infobar.cc sky@: chrome/app/generated_resources.grd chrome/chrome_browser.gypi chrome/chrome_browser_ui.gypi
https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java (right): https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:11: * Save password infobar offers a user an ability to save password to the site. Nit: "*The* Save Password infobars offers *the* user *the* ability to save *a* password to the site" (or maybe "a site password"). https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:17: boolean mAddMoreButton; Member variables should be private. Also, you could make it final. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:26: SavePasswordInfoBar(long nativeInfoBar, int iconDrawbleId, String message, Is this constructor meant to be called from somewhere else? If so, make it public. If not, make it private. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:36: if (mAddMoreButton) Android style is to either put everything on one line if it fits, or use braces. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:20: return source_type == This seems to be indented a bit weirdly... Could you run it through clang-format? https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:37: // For android in case of smart lock we need different appearance of Nit: Capitalize Android (you're not talking about a robot :) ). https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:40: scoped_ptr<SavePasswordInfoBarDelegate>(new SavePasswordInfoBarDelegate( Extract the delegate and the infobar into local variables, then you only have to #ifdef the infobar constructor. [...] https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:43: // For desktop we'll keep using confirm ifobar. "[...] the infobar" (with an "n" and an article). https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:45: SavePasswordInfoBarDelegate( Does this constructor need to be public now? https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:50: // If infobar was tiggered by Credential management API, then on Android Nit: "triggered" (Tigger is something else [http://en.wikipedia.org/wiki/Tigger] :) ). Also: Articles ("If *the* infobar was triggered by *the* Credential management API, then [...] it should display *the* 'More' button"). https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.cc:39: return static_cast<SavePasswordInfoBarDelegate*>(delegate()); You can probably inline this method; it's private and used exactly once. Alternatively, you get the delegate at construction time anyway. You could save a pointer to the delegate, or save a flag with the value of delegate->ShouldShowMoreButton(). https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.h:28: bool RegisterSavePasswordInfoBar(JNIEnv* env); You could move this into the class as a static method, to avoid polluting the global namespace. Also, is there ever called somewhere?
chrome/browser/password_manager LGTM % bauerb's excellent comments. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:20: return source_type == On 2015/03/02 at 10:28:09, Bernhard Bauer wrote: > This seems to be indented a bit weirdly... Could you run it through clang-format? It's weird enough that I have to assume clang-format is responsible. :) https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:81: // Determines source from where infobar was triggered. Nit: This doesn't "determine", it "records" or something similar.
https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/android/ja... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java (right): https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:11: * Save password infobar offers a user an ability to save password to the site. On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Nit: "*The* Save Password infobars offers *the* user *the* ability to save *a* > password to the site" (or maybe "a site password"). offer, for the site? https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:17: boolean mAddMoreButton; On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Member variables should be private. Also, you could make it final. Can you rename this boolean? It makes it sound like the field is for an "Add More" button (because Button is an android class). Maybe mIsMoreButtonNeeded? https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/android/ja... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:26: SavePasswordInfoBar(long nativeInfoBar, int iconDrawbleId, String message, On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Is this constructor meant to be called from somewhere else? If so, make it > public. If not, make it private. Should be private; you don't want people to be able to create one of these without a native counterpart. https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/pa... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/pa... chrome/browser/password_manager/chrome_password_manager_client.cc:192: SavePasswordInfoBarDelegate::Create(web_contents(), form_to_save.Pass(), nit: if the arguments don't fit, c++ style puts them on their own lines IIRC https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/pa... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/pa... chrome/browser/password_manager/save_password_infobar_delegate.cc:43: // For desktop we'll keep using confirm ifobar. On 2015/03/02 10:28:09, Bernhard Bauer wrote: > "[...] the infobar" (with an "n" and an article). nit: may as well say ConfirmInfobar since it's a specific class type. https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/ui... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/ui... chrome/browser/ui/android/infobars/save_password_infobar.h:11: class SavePasswordInfoBar : public ConfirmInfoBar { add a comment describing the class https://chromiumcodereview.appspot.com/967193002/diff/20001/chrome/browser/ui... chrome/browser/ui/android/infobars/save_password_infobar.h:28: bool RegisterSavePasswordInfoBar(JNIEnv* env); On 2015/03/02 10:28:09, Bernhard Bauer wrote: > You could move this into the class as a static method, to avoid polluting the > global namespace. > > Also, is there ever called somewhere? Supposed to be called from chrome_jni_registrar. It'll crash when the infobar appears, otherwise, since the methods aren't registered.
LGTM
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java (right): https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:11: * Save password infobar offers a user an ability to save password to the site. On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Nit: "*The* Save Password infobars offers *the* user *the* ability to save *a* > password to the site" (or maybe "a site password"). Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:11: * Save password infobar offers a user an ability to save password to the site. On 2015/03/02 17:39:38, dfalcantara wrote: > On 2015/03/02 10:28:09, Bernhard Bauer wrote: > > Nit: "*The* Save Password infobars offers *the* user *the* ability to save *a* > > password to the site" (or maybe "a site password"). > > offer, for the site? Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:17: boolean mAddMoreButton; On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Member variables should be private. Also, you could make it final. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:17: boolean mAddMoreButton; On 2015/03/02 17:39:38, dfalcantara wrote: > On 2015/03/02 10:28:09, Bernhard Bauer wrote: > > Member variables should be private. Also, you could make it final. > > Can you rename this boolean? It makes it sound like the field is for an "Add > More" button (because Button is an android class). Maybe mIsMoreButtonNeeded? Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:26: SavePasswordInfoBar(long nativeInfoBar, int iconDrawbleId, String message, On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Is this constructor meant to be called from somewhere else? If so, make it > public. If not, make it private. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:26: SavePasswordInfoBar(long nativeInfoBar, int iconDrawbleId, String message, On 2015/03/02 17:39:38, dfalcantara wrote: > On 2015/03/02 10:28:09, Bernhard Bauer wrote: > > Is this constructor meant to be called from somewhere else? If so, make it > > public. If not, make it private. > > Should be private; you don't want people to be able to create one of these > without a native counterpart. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/infobar/SavePasswordInfoBar.java:36: if (mAddMoreButton) On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Android style is to either put everything on one line if it fits, or use braces. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:192: SavePasswordInfoBarDelegate::Create(web_contents(), form_to_save.Pass(), On 2015/03/02 17:39:38, dfalcantara wrote: > nit: if the arguments don't fit, c++ style puts them on their own lines IIRC I was also surprised when I saw how clang format formatted this line, but apparently it's allowed: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl... see second example. I'll put all arguments on separate line. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:20: return source_type == On 2015/03/02 11:54:27, Mike West wrote: > On 2015/03/02 at 10:28:09, Bernhard Bauer wrote: > > This seems to be indented a bit weirdly... Could you run it through > clang-format? > > It's weird enough that I have to assume clang-format is responsible. :) Yeah, Mike is right :) https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:20: return source_type == On 2015/03/02 10:28:09, Bernhard Bauer wrote: > This seems to be indented a bit weirdly... Could you run it through > clang-format? Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:37: // For android in case of smart lock we need different appearance of On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Nit: Capitalize Android (you're not talking about a robot :) ). Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:40: scoped_ptr<SavePasswordInfoBarDelegate>(new SavePasswordInfoBarDelegate( On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Extract the delegate and the infobar into local variables, then you only have to > #ifdef the infobar constructor. > > [...] Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:43: // For desktop we'll keep using confirm ifobar. On 2015/03/02 10:28:09, Bernhard Bauer wrote: > "[...] the infobar" (with an "n" and an article). Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.cc:43: // For desktop we'll keep using confirm ifobar. On 2015/03/02 17:39:38, dfalcantara wrote: > On 2015/03/02 10:28:09, Bernhard Bauer wrote: > > "[...] the infobar" (with an "n" and an article). > > nit: may as well say ConfirmInfobar since it's a specific class type. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:45: SavePasswordInfoBarDelegate( On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Does this constructor need to be public now? Nope. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:50: // If infobar was tiggered by Credential management API, then on Android On 2015/03/02 10:28:09, Bernhard Bauer wrote: > Nit: "triggered" (Tigger is something else [http://en.wikipedia.org/wiki/Tigger] > :) ). > > Also: Articles ("If *the* infobar was triggered by *the* Credential management > API, then [...] it should display *the* 'More' button"). Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:81: // Determines source from where infobar was triggered. On 2015/03/02 11:54:27, Mike West wrote: > Nit: This doesn't "determine", it "records" or something similar. Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.cc:39: return static_cast<SavePasswordInfoBarDelegate*>(delegate()); On 2015/03/02 10:28:09, Bernhard Bauer wrote: > You can probably inline this method; it's private and used exactly once. > > Alternatively, you get the delegate at construction time anyway. You could save > a pointer to the delegate, or save a flag with the value of > delegate->ShouldShowMoreButton(). Actually twice, also for GetMessage. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.h:11: class SavePasswordInfoBar : public ConfirmInfoBar { On 2015/03/02 17:39:38, dfalcantara wrote: > add a comment describing the class Done. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.h:28: bool RegisterSavePasswordInfoBar(JNIEnv* env); On 2015/03/02 17:39:38, dfalcantara wrote: > On 2015/03/02 10:28:09, Bernhard Bauer wrote: > > You could move this into the class as a static method, to avoid polluting the > > global namespace. > > > > Also, is there ever called somewhere? > > Supposed to be called from chrome_jni_registrar. It'll crash when the infobar > appears, otherwise, since the methods aren't registered. Done.
Please run trybots on your CLs, and also do a manual check that nothing blows up; I see at least two things in here that would cause a crash. https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.cc:192: SavePasswordInfoBarDelegate::Create(web_contents(), form_to_save.Pass(), On 2015/03/03 11:55:46, melandory wrote: > On 2015/03/02 17:39:38, dfalcantara wrote: > > nit: if the arguments don't fit, c++ style puts them on their own lines IIRC > > I was also surprised when I saw how clang format formatted this line, but > apparently it's allowed: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl... > see second example. > > I'll put all arguments on separate line. > Yeah, the "either all arguments on one line, or each on a separate line" rule only applies to method headers. https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:47: // it should display the "More" button. Merge this with the previous line. https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.cc (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.cc:14: save_password_delegate_(delegate.Pass()) { You can't pass a scoped_ptr twice: on the technical level, calling .Pass() will null it out; on the conceptual level, only one object can own the delegate, so you can't pass ownership to two objects. unique_ptr makes this a bit more explicit in its name, because there can only be one instance of the same unique_ptr. https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.h:21: bool RegisterSavePasswordInfoBar(JNIEnv* env); I think this should be static? And it's still called nowhere. Did you test that this doesn't crash?
On 2015/03/03 16:54:21, Bernhard Bauer wrote: > Please run trybots on your CLs, and also do a manual check that nothing blows > up; I see at least two things in here that would cause a crash. For now I can't do it, because this CL depends on this https://codereview.chromium.org/965603002/ So trybots will fail. > https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... > File chrome/browser/password_manager/chrome_password_manager_client.cc (right): > > https://codereview.chromium.org/967193002/diff/20001/chrome/browser/password_... > chrome/browser/password_manager/chrome_password_manager_client.cc:192: > SavePasswordInfoBarDelegate::Create(web_contents(), form_to_save.Pass(), > On 2015/03/03 11:55:46, melandory wrote: > > On 2015/03/02 17:39:38, dfalcantara wrote: > > > nit: if the arguments don't fit, c++ style puts them on their own lines IIRC > > > > I was also surprised when I saw how clang format formatted this line, but > > apparently it's allowed: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl... > > see second example. > > > > I'll put all arguments on separate line. > > > > Yeah, the "either all arguments on one line, or each on a separate line" rule > only applies to method headers. > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... > File chrome/browser/password_manager/save_password_infobar_delegate.h (right): > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... > chrome/browser/password_manager/save_password_infobar_delegate.h:47: // it > should display the "More" button. > Merge this with the previous line. > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... > File chrome/browser/ui/android/infobars/save_password_infobar.cc (right): > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... > chrome/browser/ui/android/infobars/save_password_infobar.cc:14: > save_password_delegate_(delegate.Pass()) { > You can't pass a scoped_ptr twice: on the technical level, calling .Pass() will > null it out; on the conceptual level, only one object can own the delegate, so > you can't pass ownership to two objects. unique_ptr makes this a bit more > explicit in its name, because there can only be one instance of the same > unique_ptr. > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... > File chrome/browser/ui/android/infobars/save_password_infobar.h (right): > > https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... > chrome/browser/ui/android/infobars/save_password_infobar.h:21: bool > RegisterSavePasswordInfoBar(JNIEnv* env); > I think this should be static? And it's still called nowhere. Did you test that > this doesn't crash?
On 2015/03/04 11:59:05, melandory wrote: > On 2015/03/03 16:54:21, Bernhard Bauer wrote: > > Please run trybots on your CLs, and also do a manual check that nothing blows > > up; I see at least two things in here that would cause a crash. > For now I can't do it, because this CL depends on this > https://codereview.chromium.org/965603002/ > > So trybots will fail. You could try `git try origin/master` (note the missing `cl`), which will directly send the diff to origin/master to the tryserver, which means it will try both CLs together.
https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/save_password_infobar_delegate.h (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/save_password_infobar_delegate.h:47: // it should display the "More" button. On 2015/03/03 16:54:20, Bernhard (slow until Mar 9) wrote: > Merge this with the previous line. Done. https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.cc (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.cc:14: save_password_delegate_(delegate.Pass()) { On 2015/03/03 16:54:20, Bernhard (slow until Mar 9) wrote: > You can't pass a scoped_ptr twice: on the technical level, calling .Pass() will > null it out; on the conceptual level, only one object can own the delegate, so > you can't pass ownership to two objects. unique_ptr makes this a bit more > explicit in its name, because there can only be one instance of the same > unique_ptr. Done. https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://codereview.chromium.org/967193002/diff/60001/chrome/browser/ui/androi... chrome/browser/ui/android/infobars/save_password_infobar.h:21: bool RegisterSavePasswordInfoBar(JNIEnv* env); On 2015/03/03 16:54:20, Bernhard Bauer wrote: > I think this should be static? And it's still called nowhere. Did you test that > this doesn't crash? It should be static. It doesn't crash and I think it's because method isn't called anywhere.
lgtm % bauerb's approval https://chromiumcodereview.appspot.com/967193002/diff/60001/chrome/browser/ui... File chrome/browser/ui/android/infobars/save_password_infobar.h (right): https://chromiumcodereview.appspot.com/967193002/diff/60001/chrome/browser/ui... chrome/browser/ui/android/infobars/save_password_infobar.h:21: bool RegisterSavePasswordInfoBar(JNIEnv* env); On 2015/03/05 17:15:13, melandory wrote: > On 2015/03/03 16:54:20, Bernhard Bauer wrote: > > I think this should be static? And it's still called nowhere. Did you test > that > > this doesn't crash? > > It should be static. It doesn't crash and I think it's because method isn't > called anywhere. The reason why it would crash it because it _wasn't_ called anywhere. This method (and the other ones like it in the other C++ <-> Java linked classes) links the C++ SavePasswordInfoBar with the Java SavePasswordInfoBar methods, creating a mapping that allows C++ methods to be called from Java and vice versa. If you fail to register the methods in chrome_jni_registrar.cc, then trying to call any methods across the language boundary will immediately result in Chrome crashing because it can't find the methods. That said: please remove the comment above it. Not sure how useful it is.
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
LGTM
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, sky@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/967193002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967193002/140001
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e508b5d19b3bb82afc4d11885c8ab1c18192ec3b Cr-Commit-Position: refs/heads/master@{#319892} |
