Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042033003:
Displaying human-readable Android credentials on Android OS. (Closed)
|
Created:
4 years, 6 months ago by dozsa Modified:
4 years, 6 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisplaying human-readable Android credentials on Android OS.
Note: link_url from GetShownOriginAndLinkUrl is not being used.
BUG=617094
Committed: https://crrev.com/127c33456ca196306f48b024a049f0a33699fe1e
Cr-Commit-Position: refs/heads/master@{#399453}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved SplitByDotAndReverse to password_ui_utils #
Total comments: 17
Patch Set 3 : Added unit test for SplitByDotAndReverse, addressed style issues. #
Total comments: 10
Patch Set 4 : Replaced hard-coded English string with translated string, fixed a few other minor issues. #
Total comments: 1
Patch Set 5 : Included <algorithm> in password_manager_presenter in order to solve trybot issue. #Patch Set 6 : Added #include <algorithm> to the files it was missing from. #
Total comments: 23
Patch Set 7 : Introduce ModifyOrigin in password_ui_utils #
Total comments: 10
Patch Set 8 : Address minor comments #
Messages
Total messages: 53 (20 generated)
vabr@chromium.org changed reviewers: + vabr@chromium.org
Thanks for the CL! I have two comments, please take a look. Addressing the second one will change some further places in the code, so let's continue the review once that's done. Thank you! Vaclav https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... chrome/browser/android/password_ui_view_android.cc:16: #include "chrome/browser/android/password_ui_view_android.h" Please keep this #include where it was (above all the other #includes, separated by a blank line). See https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes for the guidance on the #include order. https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... chrome/browser/android/password_ui_view_android.cc:34: const int kAndroidAppSchemeAndDelimiterLength = 10; // Length of 'android://'. Please move kAndroidAppSchemeAndDelimiterLength and SplitByDotAndReverse to components/password_manager/core/browser/password_ui_utils.*, and use them both here and in chrome/browser/ui/passwords/password_manager_presenter.cc. We should avoid code duplication as much as possible, to ensure the two implementations do not diverge (and also to reduce the binary size).
Thank you for the improvements! I have some more comments, 2 here and the rest in the code: (1) Currently link_url is not used. I think it is reasonable to keep it for a separate CL, but it should at least be mentioned in the CL description, and there should be a // TODO(crbug.com/617094) Also display link_url. in the code. (2) Once you address comments, feel free to mark them as "Done" (there is a link for that at each comment) or respond to them if not, and use the "Publish+Mail Comments" link in the left column of the page. That will send an e-mail with your comments, publish them in the code review tool, and mark the CL on the reviewer's dashboard as needing attention. I'm happy to answer any more questions you may have. Thanks for doing this work! Vaclav https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:9: #include "base/bind.h" None of the added #includes except for string_piece.h and password_ui_utils.h seem to be needed. Could you drop them? https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:32: using base::StringPiece; optional: Inlining "base::" in the code below is not changing how the lines are broken, so I'd recommend dropping the using directive in this case. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:33: using password_manager::PasswordStore; Appears unused, please remove. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:96: bool is_android_uri = false; The block here is identical to the one on lines 126-142. Please define a helper function in the anonymous namespace instead and call it here and below. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:17: using base::StringPiece; optional: My suggestion would be to drop the "using" here as well, as adding base:: on line 31 won't change how the lines are broken. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:31: std::string SplitByDotAndReverse(StringPiece host) { Please make sure to also use this function in chrome/browser/ui/passwords/password_manager_presenter.cc and remove its local copy in that file. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:31: std::string SplitByDotAndReverse(StringPiece host) { This function also needs tests. Please have a look at password_ui_utils_unittest.cc and add some. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.h:16: using base::StringPiece; nit: Please do not use the "using" directive in header files. Those may be included in many other files and cause naming conflicts. Please use the full type name with its namespace in the header, and only use "using" in the .cc file if the type name is used often enough and the namespace prefix is awkwardly long. (This is not necessarily the case here.) https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.h:26: // Reverses order of subdomains in hostname nit: Don't forget the full-stop (.) at the end of the sentence.
Description was changed from ========== Displaying human-readable Android credentials on Android OS. BUG=617094 ========== to ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used BUG=617094 ==========
Description was changed from ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used BUG=617094 ========== to ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ==========
https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... chrome/browser/android/password_ui_view_android.cc:16: #include "chrome/browser/android/password_ui_view_android.h" On 2016/06/08 07:51:33, vabr (Chromium) wrote: > Please keep this #include where it was (above all the other #includes, separated > by a blank line). See > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > for the guidance on the #include order. Done. https://codereview.chromium.org/2042033003/diff/1/chrome/browser/android/pass... chrome/browser/android/password_ui_view_android.cc:34: const int kAndroidAppSchemeAndDelimiterLength = 10; // Length of 'android://'. On 2016/06/08 07:51:34, vabr (Chromium) wrote: > Please move kAndroidAppSchemeAndDelimiterLength and SplitByDotAndReverse to > components/password_manager/core/browser/password_ui_utils.*, and use them both > here and in chrome/browser/ui/passwords/password_manager_presenter.cc. > > We should avoid code duplication as much as possible, to ensure the two > implementations do not diverge (and also to reduce the binary size). Done. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:9: #include "base/bind.h" On 2016/06/08 09:04:04, vabr (Chromium) wrote: > None of the added #includes except for string_piece.h and password_ui_utils.h > seem to be needed. Could you drop them? Done. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:32: using base::StringPiece; On 2016/06/08 09:04:04, vabr (Chromium) wrote: > optional: Inlining "base::" in the code below is not changing how the lines are > broken, so I'd recommend dropping the using directive in this case. Done. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:33: using password_manager::PasswordStore; On 2016/06/08 09:04:05, vabr (Chromium) wrote: > Appears unused, please remove. Done. https://codereview.chromium.org/2042033003/diff/20001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:96: bool is_android_uri = false; On 2016/06/08 09:04:04, vabr (Chromium) wrote: > The block here is identical to the one on lines 126-142. > > Please define a helper function in the anonymous namespace instead and call it > here and below. Done. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:17: using base::StringPiece; On 2016/06/08 09:04:05, vabr (Chromium) wrote: > optional: My suggestion would be to drop the "using" here as well, as adding > base:: on line 31 won't change how the lines are broken. Done. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:31: std::string SplitByDotAndReverse(StringPiece host) { On 2016/06/08 09:04:05, vabr (Chromium) wrote: > Please make sure to also use this function in > chrome/browser/ui/passwords/password_manager_presenter.cc and remove its local > copy in that file. Done. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:31: std::string SplitByDotAndReverse(StringPiece host) { On 2016/06/08 09:04:05, vabr (Chromium) wrote: > This function also needs tests. Please have a look at > password_ui_utils_unittest.cc and add some. Done. https://codereview.chromium.org/2042033003/diff/20001/components/password_man... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/20001/components/password_man... components/password_manager/core/browser/password_ui_utils.h:16: using base::StringPiece; On 2016/06/08 09:04:05, vabr (Chromium) wrote: > nit: Please do not use the "using" directive in header files. Those may be > included in many other files and cause naming conflicts. Please use the full > type name with its namespace in the header, and only use "using" in the .cc file > if the type name is used often enough and the namespace prefix is awkwardly > long. (This is not necessarily the case here.) Done.
Looks even better! Just some minor comments below. Also, please keep a blank line in the CL description after the first line. The CL description becomes the git commit message, and the second line being blank is what git expects. Thanks! Vaclav https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { Please do not pass the PasswordForm by value. It is a big structure and this function does not need to own or modify it. Please use a const reference isntead: const autofill::PasswordForm& form https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { There is already a function of this name in affiliation_utils.h. To avoid clashes, please rename it. Possible suggestion is: GetDisplayOriginForSettings. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { nit: Please document what the function does. Something like: // Returns the origin string to be displayed in Chrome settings for |form|. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:37: human_readable_origin = password_manager::SplitByDotAndReverse( optional: Insert DCHECK(is_android_uri); between lines 36 and 37, to document that is_clickable actually implies is_android_uri. This assumption is used when removing the android:// prefix from the origin. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:44: human_readable_origin = human_readable_origin + " (Android)"; Please use the translated string, identified by IDS_PASSWORDS_ANDROID_URI_SUFFIX, instead of hard-coding the English version. You will need l10n_util::GetStringUTF8 to transform IDS_PASSWORDS_ANDROID_URI_SUFFIX into a string.
Description was changed from ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ========== to ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ==========
https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { On 2016/06/08 11:46:21, vabr (Chromium) wrote: > Please do not pass the PasswordForm by value. It is a big structure and this > function does not need to own or modify it. Please use a const reference > isntead: > const autofill::PasswordForm& form Done. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { On 2016/06/08 11:46:21, vabr (Chromium) wrote: > nit: Please document what the function does. Something like: > // Returns the origin string to be displayed in Chrome settings for |form|. Done. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:28: std::string GetHumanReadableOrigin(autofill::PasswordForm form) { On 2016/06/08 11:46:21, vabr (Chromium) wrote: > There is already a function of this name in affiliation_utils.h. To avoid > clashes, please rename it. Possible suggestion is: GetDisplayOriginForSettings. Done. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:37: human_readable_origin = password_manager::SplitByDotAndReverse( On 2016/06/08 11:46:21, vabr (Chromium) wrote: > optional: Insert > DCHECK(is_android_uri); > between lines 36 and 37, to document that is_clickable actually implies > is_android_uri. This assumption is used when removing the android:// prefix from > the origin. Done. https://codereview.chromium.org/2042033003/diff/40001/chrome/browser/android/... chrome/browser/android/password_ui_view_android.cc:44: human_readable_origin = human_readable_origin + " (Android)"; On 2016/06/08 11:46:21, vabr (Chromium) wrote: > Please use the translated string, identified by > IDS_PASSWORDS_ANDROID_URI_SUFFIX, instead of hard-coding the English version. > You will need l10n_util::GetStringUTF8 to transform > IDS_PASSWORDS_ANDROID_URI_SUFFIX into a string. Done.
LGTM, thanks for the patch! I'm happy to help you select the reviewer for chrome/browser/android/password_ui_view_android.cc if you want to. Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dozsa@google.com changed reviewers: + miguelg@chromium.org
https://codereview.chromium.org/2042033003/diff/60001/components/password_man... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/60001/components/password_man... components/password_manager/core/browser/password_ui_utils.cc:32: std::reverse(parts.begin(), parts.end()); Looking at the trybots, we need #include <algorithm> for std::reverse
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/80001
miguelg@ -- please review chrome/browser/android/password_ui_view_android.cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dozsa@google.com changed reviewers: + bauerb@chromium.org - miguelg@chromium.org
@bauerb - could you please review this CL?
On 2016/06/13 08:29:53, dozsa wrote: > @bauerb - could you please review this CL? Actually, I think you only need Bernhard's approval for chrome/browser/android/password_ui_view_android.cc, Paula. Cheers, Vaclav
Thank you for the correction, Vaclav! Yes, I only need approval for that file. Best, Paula On Mon, Jun 13, 2016 at 10:34 AM, <vabr@chromium.org> wrote: > On 2016/06/13 08:29:53, dozsa wrote: > > @bauerb - could you please review this CL? > > Actually, I think you only need Bernhard's approval for > chrome/browser/android/password_ui_view_android.cc, Paula. > > Cheers, > Vaclav > > https://codereview.chromium.org/2042033003/ > -- Paula Valentina Dozsa SWE Intern Google Munich NYU Abu Dhabi Class of '18 -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description nit: The name of the OS is just "Android", not "Android OS". Also, try to use the imperative form for commit messages; see http://chris.beams.io/posts/git-commit/ for example. https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... chrome/browser/android/password_ui_view_android.cc:46: &human_readable_origin[ This alignment is kind of weird, but I suspect it would change anyway if we don't access kAndroidAppSchemeAndDelimiterLength from a different namespace (see my comment in password_manager_presenter.cc). https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... chrome/browser/android/password_ui_view_android.cc:52: l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); Manually composing user-visible localized strings like that seems... off. I think it would be better to make the localized resource a format string and pass the origin in as a parameter. https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:84: &origin[password_manager::kAndroidAppSchemeAndDelimiterLength], Instead of exposing this constant, I think it would be better to expose a function that does this, and keep the constant an implementation detail of that function. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:27: const int kAndroidAppSchemeAndDelimiterLength = 10; // Length of 'android://'. Actually, could you define this as sizeof("android://")? :) The compiler should be able to figure out the length of the string without putting the actual bytes into the object file. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:24: // Reverses order of subdomains in hostname. Nit: the individual pieces aren't subdomains (successive suffixes of the hostname would be); instead, they're called labels. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { FWIW, I think in this case you'd be better served by unrolling the loop: EXPECT_EQ("android.example.com", SplitByDotAndReverse("com.example.android")); ... For example, that will tell you the expected and actual value in case of a failure.
https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... chrome/browser/android/password_ui_view_android.cc:52: l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); On 2016/06/13 09:34:59, Bernhard Bauer wrote: > Manually composing user-visible localized strings like that seems... off. I > think it would be better to make the localized resource a format string and pass > the origin in as a parameter. While I agree with this observation, I would suggest to do it in a separate CL. The current usage is already present in the desktop settings, so those would have to be changed here as well. Let's not bloat the CL and let it serve 2 purposes. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:29: // Reverse order of subdomains in hostname. nit: No need to copy the comment in the .cc, when it already is in the header. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { On 2016/06/13 09:34:59, Bernhard Bauer wrote: > FWIW, I think in this case you'd be better served by unrolling the loop: > > EXPECT_EQ("android.example.com", SplitByDotAndReverse("com.example.android")); > ... > > For example, that will tell you the expected and actual value in case of a > failure. Bernhard, this seems to be the case already. EXPECT_EQ will print out both arguments and the values they evaluate to. My personal preference here is for the current code, because if we need to add more pairs, reading the test cases array definition is easier than the lines cluttered with EXPECT_EQ and SplitByDotAndReverse.
https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:98: const base::StringPiece input; Just FYI: In unit tests we don't care so much, but in production code you should use const char* here, because StringPiece and std::string have constructors, and we don't want static initializers in production code. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { On 2016/06/13 09:45:21, vabr (Chromium) wrote: > On 2016/06/13 09:34:59, Bernhard Bauer wrote: > > FWIW, I think in this case you'd be better served by unrolling the loop: > > > > EXPECT_EQ("android.example.com", > SplitByDotAndReverse("com.example.android")); > > ... > > > > For example, that will tell you the expected and actual value in case of a > > failure. > > Bernhard, this seems to be the case already. EXPECT_EQ will print out both > arguments and the values they evaluate to. Yes, but it will print the unevaluated arguments literally as what is passed to the EXPECT_EQ macro, i.e. "SplitByDotAndReverse(test_case.input)". > My personal preference here is for the current code, because if we need to add > more pairs, reading the test cases array definition is easier than the lines > cluttered with EXPECT_EQ and SplitByDotAndReverse.
https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:98: const base::StringPiece input; On 2016/06/13 09:55:07, Bernhard Bauer wrote: > Just FYI: In unit tests we don't care so much, but in production code you should > use const char* here, because StringPiece and std::string have constructors, and > we don't want static initializers in production code. But here there is no static variable, kTestCases is a local one, its life span is limited to the current function. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { On 2016/06/13 09:55:07, Bernhard Bauer wrote: > On 2016/06/13 09:45:21, vabr (Chromium) wrote: > > On 2016/06/13 09:34:59, Bernhard Bauer wrote: > > > FWIW, I think in this case you'd be better served by unrolling the loop: > > > > > > EXPECT_EQ("android.example.com", > > SplitByDotAndReverse("com.example.android")); > > > ... > > > > > > For example, that will tell you the expected and actual value in case of a > > > failure. > > > > Bernhard, this seems to be the case already. EXPECT_EQ will print out both > > arguments and the values they evaluate to. > > Yes, but it will print the unevaluated arguments literally as what is passed to > the EXPECT_EQ macro, i.e. "SplitByDotAndReverse(test_case.input)". But there is no advantage in that, as long as SplitByDotAndReverse is injective on the domain of strings in the test cases. > > > My personal preference here is for the current code, because if we need to add > > more pairs, reading the test cases array definition is easier than the lines > > cluttered with EXPECT_EQ and SplitByDotAndReverse. >
https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:98: const base::StringPiece input; On 2016/06/13 10:01:03, vabr (Chromium) wrote: > On 2016/06/13 09:55:07, Bernhard Bauer wrote: > > Just FYI: In unit tests we don't care so much, but in production code you > should > > use const char* here, because StringPiece and std::string have constructors, > and > > we don't want static initializers in production code. > > But here there is no static variable, kTestCases is a local one, its life span > is limited to the current function. (Which is also a thing I would avoid in production code ☺) https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { On 2016/06/13 10:01:03, vabr (Chromium) wrote: > On 2016/06/13 09:55:07, Bernhard Bauer wrote: > > On 2016/06/13 09:45:21, vabr (Chromium) wrote: > > > On 2016/06/13 09:34:59, Bernhard Bauer wrote: > > > > FWIW, I think in this case you'd be better served by unrolling the loop: > > > > > > > > EXPECT_EQ("android.example.com", > > > SplitByDotAndReverse("com.example.android")); > > > > ... > > > > > > > > For example, that will tell you the expected and actual value in case of a > > > > failure. > > > > > > Bernhard, this seems to be the case already. EXPECT_EQ will print out both > > > arguments and the values they evaluate to. > > > > Yes, but it will print the unevaluated arguments literally as what is passed > to > > the EXPECT_EQ macro, i.e. "SplitByDotAndReverse(test_case.input)". > > But there is no advantage in that, as long as SplitByDotAndReverse is injective > on the domain of strings in the test cases. I personally would find it more readable than having to infer the input from the expected output, but I don't feel strongly enough about it to object. > > > > > My personal preference here is for the current code, because if we need to > add > > > more pairs, reading the test cases array definition is easier than the lines > > > cluttered with EXPECT_EQ and SplitByDotAndReverse. > > >
https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... File chrome/browser/android/password_ui_view_android.cc (right): https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... chrome/browser/android/password_ui_view_android.cc:46: &human_readable_origin[ On 2016/06/13 09:34:59, Bernhard Bauer wrote: > This alignment is kind of weird, but I suspect it would change anyway if we > don't access kAndroidAppSchemeAndDelimiterLength from a different namespace (see > my comment in password_manager_presenter.cc). Done. https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/android... chrome/browser/android/password_ui_view_android.cc:52: l10n_util::GetStringUTF8(IDS_PASSWORDS_ANDROID_URI_SUFFIX); On 2016/06/13 09:45:20, vabr (Chromium) wrote: > On 2016/06/13 09:34:59, Bernhard Bauer wrote: > > Manually composing user-visible localized strings like that seems... off. I > > think it would be better to make the localized resource a format string and > pass > > the origin in as a parameter. > > While I agree with this observation, I would suggest to do it in a separate CL. > The current usage is already present in the desktop settings, so those would > have to be changed here as well. Let's not bloat the CL and let it serve 2 > purposes. Acknowledged. https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/2042033003/diff/100001/chrome/browser/ui/pass... chrome/browser/ui/passwords/password_manager_presenter.cc:84: &origin[password_manager::kAndroidAppSchemeAndDelimiterLength], On 2016/06/13 09:34:59, Bernhard Bauer wrote: > Instead of exposing this constant, I think it would be better to expose a > function that does this, and keep the constant an implementation detail of that > function. Done. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:27: const int kAndroidAppSchemeAndDelimiterLength = 10; // Length of 'android://'. On 2016/06/13 09:34:59, Bernhard Bauer wrote: > Actually, could you define this as sizeof("android://")? :) The compiler should > be able to figure out the length of the string without putting the actual bytes > into the object file. Done. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:29: // Reverse order of subdomains in hostname. On 2016/06/13 09:45:21, vabr (Chromium) wrote: > nit: No need to copy the comment in the .cc, when it already is in the header. Done. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:24: // Reverses order of subdomains in hostname. On 2016/06/13 09:34:59, Bernhard Bauer wrote: > Nit: the individual pieces aren't subdomains (successive suffixes of the > hostname would be); instead, they're called labels. Done. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:98: const base::StringPiece input; On 2016/06/13 09:55:07, Bernhard Bauer wrote: > Just FYI: In unit tests we don't care so much, but in production code you should > use const char* here, because StringPiece and std::string have constructors, and > we don't want static initializers in production code. Done. https://codereview.chromium.org/2042033003/diff/100001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:103: for (const auto& test_case : kTestCases) { On 2016/06/13 10:34:01, Bernhard Bauer wrote: > On 2016/06/13 10:01:03, vabr (Chromium) wrote: > > On 2016/06/13 09:55:07, Bernhard Bauer wrote: > > > On 2016/06/13 09:45:21, vabr (Chromium) wrote: > > > > On 2016/06/13 09:34:59, Bernhard Bauer wrote: > > > > > FWIW, I think in this case you'd be better served by unrolling the loop: > > > > > > > > > > EXPECT_EQ("android.example.com", > > > > SplitByDotAndReverse("com.example.android")); > > > > > ... > > > > > > > > > > For example, that will tell you the expected and actual value in case of > a > > > > > failure. > > > > > > > > Bernhard, this seems to be the case already. EXPECT_EQ will print out both > > > > arguments and the values they evaluate to. > > > > > > Yes, but it will print the unevaluated arguments literally as what is passed > > to > > > the EXPECT_EQ macro, i.e. "SplitByDotAndReverse(test_case.input)". > > > > But there is no advantage in that, as long as SplitByDotAndReverse is > injective > > on the domain of strings in the test cases. > > I personally would find it more readable than having to infer the input from the > expected output, but I don't feel strongly enough about it to object. > > > > > > > > My personal preference here is for the current code, because if we need to > > add > > > > more pairs, reading the test cases array definition is easier than the > lines > > > > cluttered with EXPECT_EQ and SplitByDotAndReverse. > > > > > > I used the unit test for GetShownOrigin as a model for this test. Conform to vabr@'s comments, I've left the test as is.
The CQ bit was checked by dozsa@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/120001
Still LGTM, with some comments. Thanks! Vaclav https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:27: std::string SplitByDotAndReverse(base::StringPiece host) { This could be in the anonymous namespace now. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:34: std::string ModifyOrigin(std::string origin) { For performance purposes, I would suggest to take a const reference: const std::string& origin and then return the result of SplitByDotAndReverse directly. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:22: extern const int kAndroidAppSchemeAndDelimiterLength; // Length of 'android://' Now you no longer need to expose the constant. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:25: std::string SplitByDotAndReverse(base::StringPiece host); And this function should also be OK to move into the anonymous namespace in the .cc file only. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:28: std::string ModifyOrigin(std::string origin); nit: StripAndroidAndReverse might be a slightly more descriptive name.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:27: std::string SplitByDotAndReverse(base::StringPiece host) { On 2016/06/13 13:03:36, vabr (Chromium) wrote: > This could be in the anonymous namespace now. SplitByDotAndReverse is also used in password_manager_presenter.cc. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:34: std::string ModifyOrigin(std::string origin) { On 2016/06/13 13:03:36, vabr (Chromium) wrote: > For performance purposes, I would suggest to take a const reference: > const std::string& origin > and then return the result of SplitByDotAndReverse directly. Done. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:22: extern const int kAndroidAppSchemeAndDelimiterLength; // Length of 'android://' On 2016/06/13 13:03:36, vabr (Chromium) wrote: > Now you no longer need to expose the constant. Done. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:25: std::string SplitByDotAndReverse(base::StringPiece host); On 2016/06/13 13:03:36, vabr (Chromium) wrote: > And this function should also be OK to move into the anonymous namespace in the > .cc file only. Done. https://codereview.chromium.org/2042033003/diff/120001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:28: std::string ModifyOrigin(std::string origin); On 2016/06/13 13:03:36, vabr (Chromium) wrote: > nit: StripAndroidAndReverse might be a slightly more descriptive name. Done.
LGTM!
The CQ bit was checked by dozsa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2042033003/#ps140001 (title: "Address minor comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/140001
The CQ bit was unchecked by dozsa@google.com
The CQ bit was checked by dozsa@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042033003/140001
Message was sent while issue was closed.
Description was changed from ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ========== to ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 ========== to ========== Displaying human-readable Android credentials on Android OS. Note: link_url from GetShownOriginAndLinkUrl is not being used. BUG=617094 Committed: https://crrev.com/127c33456ca196306f48b024a049f0a33699fe1e Cr-Commit-Position: refs/heads/master@{#399453} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/127c33456ca196306f48b024a049f0a33699fe1e Cr-Commit-Position: refs/heads/master@{#399453} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
