Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1318523011:
[Password Manager] Copiable username and origin. Linkable origin elided from the left. (Closed)
|
Created:
5 years, 3 months ago by kolos1 Modified:
5 years, 1 month ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, vabr+watchlist_chromium.org, arv+watch_chromium.org, gcasto+watchlist_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. |
Description1. Copiable username and origin.
2. Linkable origin elided from the left.
UI mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/copiable%20username%20and%20origin%20link
BUG=483915
Committed: https://crrev.com/00512644944fa28dff9272d31cb3292c17151992
Cr-Commit-Position: refs/heads/master@{#356866}
Patch Set 1 : Copiable origin and username (javascript) #Patch Set 2 : Copiable username and origin in css. Linkable origin elided from the left. #
Total comments: 14
Patch Set 3 : Replace const std::vector<std::string> with const std::string[] #Patch Set 4 : Added comments to new css classes. Removed left align for rtl locales. #
Total comments: 16
Patch Set 5 : Changes addressed to reviewer comments #
Total comments: 8
Patch Set 6 : Icons appearance fixed #
Total comments: 13
Patch Set 7 : GetShownOrigin was moved to a separate file (password_ui_utils.h) #Patch Set 8 : New icon appearance removed #Patch Set 9 : Tests compilation error fixed #
Total comments: 7
Patch Set 10 : #
Total comments: 20
Patch Set 11 : Small changes in password_ui_utils* #
Total comments: 2
Patch Set 12 : Changes with StringPiece #Dependent Patchsets: Messages
Total messages: 50 (15 generated)
Patchset #1 (id:1) has been deleted
kolos@chromium.org changed reviewers: + estade@chromium.org
Hello Evan! Please, review my changes. Thanks. Best regards, Maxim Kolosovskiy
On 2015/09/08 16:11:42, kolos1 wrote: > Hello Evan! > > Please, review my changes. Thanks. > > Best regards, > Maxim Kolosovskiy can you do this in css?
Patchset #2 (id:40001) has been deleted
kolos@chromium.org changed reviewers: + vabr@chromium.org
Patchset #2 (id:60001) has been deleted
Hi! Copiable username and origin is done in CSS. I also made some other changes of UI on chrome://settings/passwords (the description and the title are updated). Please review them. Regards, Maxim Kolosovskiy
https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:71: #saved-passwords-list .left-elided-url, can you just do .left-elided-url{...}? https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:75: direction: rtl; I must be totally misunderstanding this class because I have no idea why you're setting the direction to rtl regardless of locale https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:77: padding-left: 26px; why do you have padding-left as well as -webkit-padding-start? (same goes for margin) https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: text-align: left; why do you want to left align for RTL? the omnibox displays URLs right-aligned in RTL. If this class mysteriously makes sense it needs a lot more docs. https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.js:52: urlDiv.classList.add('favicon-cell'); urlDiv.className = 'favicon-cell left-elided-url url'; https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.js:79: usernameInput.type = 'text'; one other good way of doing this is to use html templates: <div hidden> <input id="usernameInputTemplate" type="text" class="inactive-item" ...> <input id="passwordInputTemplate" ...> </div> then in JS you just do: var node = $('usernameInputTemplate').cloneNode(true); node.id = ''; // or node.removeAttribute('id');
https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:71: #saved-passwords-list .left-elided-url, On 2015/09/29 18:06:52, Evan Stade wrote: > can you just do .left-elided-url{...}? Done. https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:75: direction: rtl; This class elide urls (without scheme and path) from the left. So, if URL doesn't fit, we show only high-level domains and hide low-level domains (e.g. ...ubdomain.example.com). And urls should be aligned to the left. See UI mocks for clarification. My solution is "direction: rtl; unicode-bidi: bidi-override; text-align: left;" I need to override values -webkit-margin-start=7px -webkit-padding-start=26px of the class ".favicon", because I change direction of the text, but I don't need so big margin/padding on the left side. I set them to 0 and 3px. But I need margin and padding on the left side. Otherwise, url will overlap an icon. I set them to 7px and 26px (values of -webkit-margin-start and -webkit-padding-start) https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.js:52: urlDiv.classList.add('favicon-cell'); On 2015/09/29 18:06:53, Evan Stade wrote: > urlDiv.className = 'favicon-cell left-elided-url url'; Done.
https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:75: direction: rtl; On 2015/09/29 18:29:01, kolos1 wrote: > This class elide urls (without scheme and path) from the left. So, if URL > doesn't fit, we show only high-level domains and hide low-level domains (e.g. > ...ubdomain.example.com). And urls should be aligned to the left. See UI mocks > for clarification. > > My solution is "direction: rtl; unicode-bidi: bidi-override; text-align: left;" I see. So this is a big hack and needs a lot of docs, because it actually has nothing to do with bidi or RTL. (Did you test what it looks like in an RTL locale?) > > I need to override values -webkit-margin-start=7px -webkit-padding-start=26px of > the class ".favicon", because I change direction of the text, but I don't need > so big margin/padding on the left side. I set them to 0 and 3px. > > But I need margin and padding on the left side. Otherwise, url will overlap an > icon. I set them to 7px and 26px (values of -webkit-margin-start and > -webkit-padding-start) can you make them -webkit-...-end: then? Because it's pretty subtle that you're switching nomenclature halfway down this list of attributes https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: text-align: left; On 2015/09/29 18:06:52, Evan Stade wrote: > why do you want to left align for RTL? the omnibox displays URLs right-aligned > in RTL. If this class mysteriously makes sense it needs a lot more docs. I still suspect this is the wrong text-align for RTL
Hi, I made some changes addressed to reviewers' comments. Please review them. CL has been tested on both RTL and LTR locales. Regards, Maxim https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:75: direction: rtl; Yes, I tested with RTL and LTR locales. Now it works. Make -webkit-...-end. https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:77: padding-left: 26px; For LTR: since the direction is changed, the icon is at the end (i.e. left). So, start and end margin/padding have to be swapped. For RTL: there will be not padding/margin changes. See the next patch. https://codereview.chromium.org/1318523011/diff/80001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: text-align: left; Right. It should be only for LTR. Fixed.
can you post a screenshot of a URL being elided in RTL? https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:219: if (model.item(i)[options.passwordManager.ORIGINAL_DATA_INDEX] == The item should be changed to a dict (js object) and then you can reference by name (hardcoded string is fine in this case). https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:238: base::ListValue* entry = new base::ListValue(); it makes more sense for this to be a dictionary, so the js can look up by name, which is much more self documenting than some semi-arbitrary index. https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:248: entries.Append(entry); this version of ListValue::Append is deprecated https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.cc:361: break; // remove only one prefix (e.g. www.mobile.de) nit: comments should be formatted like sentences (capitalization, final punctuation) https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.cc:367: } else { nit: ternary operator https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.h:209: // Remove prefixes "m.", "mobile." or "www." from the result of nit: this kind of comment is likely to get out of sync with code and not be updated. I think it should be a bit more generic: // Returns a string suitable for display to the user ... less of a nit: I don't understand why this is different from GetHumanReadableOrigin. How is "human readable" distinct from "shown"? Hopefully we wouldn't show something that's human unreadable :) https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:383: EXPECT_EQ(GetShownOrigin(GURL("https://m.subdomain.example.net"), ""), test upper case handling? non-ascii URL handling? https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:391: EXPECT_EQ(GetShownOrigin(GURL("https://m.www.de"), ""), "www.de"); nit: more readable if you do struct { std::string input; std::string output; } test_cases[] = { ... } for (int i = 0; i < arraysize(test_cases); ...) ...
Hi, Evan! Screenshots for left elided URL are here. Please review new changes. Regards, Maxim https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager.js (right): https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/password_manager.js:219: if (model.item(i)[options.passwordManager.ORIGINAL_DATA_INDEX] == On 2015/10/05 18:59:41, Evan Stade wrote: > The item should be changed to a dict (js object) and then you can reference by > name (hardcoded string is fine in this case). Done. https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:238: base::ListValue* entry = new base::ListValue(); On 2015/10/05 18:59:41, Evan Stade wrote: > it makes more sense for this to be a dictionary, so the js can look up by name, > which is much more self documenting than some semi-arbitrary index. Done. https://codereview.chromium.org/1318523011/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:248: entries.Append(entry); On 2015/10/05 18:59:41, Evan Stade wrote: > this version of ListValue::Append is deprecated Done. https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils.cc (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.cc:361: break; // remove only one prefix (e.g. www.mobile.de) On 2015/10/05 18:59:41, Evan Stade wrote: > nit: comments should be formatted like sentences (capitalization, final > punctuation) Done. https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.cc:367: } else { On 2015/10/05 18:59:41, Evan Stade wrote: > nit: ternary operator Done. https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils.h:209: // Remove prefixes "m.", "mobile." or "www." from the result of Yes, I agree, but there are many ways to get "human readable" origin. For example, GetHumanReadableOrigin always shows the scheme. GetShownOrigin always omit scheme. As I understand, we should describe the difference. I was going to say that GetShownOrigin copies the behavior of FormatUrlForSecurityDisplayOmitScheme and also removes the prefixes. The comment to FormatUrlForSecurityDisplayOmitScheme looks similar. Changed. Please review. https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... File components/password_manager/core/browser/affiliation_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:383: EXPECT_EQ(GetShownOrigin(GURL("https://m.subdomain.example.net"), ""), Tests with upper case letters has been added. Thanks. Processing various charsets, remove scheme, remove default port, remove path and others are the responsibility of FormatUrlForSecurityDisplayOmitScheme and GURL. Our work is removing prefixes ("m", "www", "mobile"). So, I check whether GetShownOrigin removes these prefixes. The fist test is a general test to check whether default port, path and parameters are removed too. https://codereview.chromium.org/1318523011/diff/120001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:391: EXPECT_EQ(GetShownOrigin(GURL("https://m.www.de"), ""), "www.de"); On 2015/10/05 18:59:42, Evan Stade wrote: > nit: more readable if you do > > struct { > std::string input; > std::string output; > } test_cases[] = { ... } > > for (int i = 0; i < arraysize(test_cases); ...) > ... Done.
Sorry, forgot to copy the link to screenshots https://code.google.com/p/chromium/issues/detail?id=483915#c16
kolos@chromium.org changed reviewers: - vabr@chromium.org
Hi Evan! Friendly ping, could you review my changes, please? Best regards, Maxim Kolosovskiy
https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:13: /** @const */ var ORIGIN_FIELD = 'origin'; I don't think these are worth much. What is the purpose? I'd just inline the strings. https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:44: const char kOriginField[] = "origin"; ditto, not much use for these if you refactor/share more code below. https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:251: void PasswordManagerHandler::SetPasswordExceptionList( it seems like there's a lot of overlap here and in SetPasswordList https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:254: for (size_t i = 0; i < password_exception_list.size(); ++i) { for (const autofill::PasswordForm& exception : password_exception_list) { }
On 2015/10/13 16:42:53, kolos1 wrote: > Hi Evan! > > Friendly ping, could you review my changes, please? > > Best regards, > Maxim Kolosovskiy also, sorry for slowness, feel free to ping me more rapidly next time
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
kolos@chromium.org changed reviewers: + pkotwicz@chromium.org, vabr@chromium.org - estade@chromium.org
Hi, vabr@chromium.org: Please review changes in affiliation_utils* pkotwicz@chromium.org: Please review changes in large_icon_service*, large_icon_url_parser*, fallback_icon_url_parser.h Thanks. Best regards, Maxim Kolosovskiy https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:13: /** @const */ var ORIGIN_FIELD = 'origin'; It collects all information about the fields of PasswordListItem. If somebody want to change PasswordListItem, there is no need to check all code of password_manager_list.js, password_manager.js or some other file. I believe this code much less error-prone. I didn't some mistakes because there were inlined numbers in password_manager_list.js and password_manager.js. I would prefer to have this. https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:44: const char kOriginField[] = "origin"; I would like to have it. It gives explicit description what we pass from C++ part to Javascript part. https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:251: void PasswordManagerHandler::SetPasswordExceptionList( On 2015/10/13 17:15:01, Evan Stade wrote: > it seems like there's a lot of overlap here and in SetPasswordList Done. Thanks. https://codereview.chromium.org/1318523011/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/password_manager_handler.cc:254: for (size_t i = 0; i < password_exception_list.size(); ++i) { On 2015/10/13 17:15:01, Evan Stade wrote: > for (const autofill::PasswordForm& exception : password_exception_list) { > > } Done. Thanks
kolos@chromium.org changed reviewers: + estade@chromium.org - pkotwicz@chromium.org, vabr@chromium.org
Hi Evan, Thanks. I made some changes addressed to your comments (password_manager_list.js, password_manager_handler.cc). Please review them. I also had to add some changes in other files since I got an update from UI team on icons appearance. All entries in the list must have an icon. I didn't show an icon if the origin is insecure. Please also review large_icon_source* and utils.js Best regards, Maxim Kolosovskiy
kolos@chromium.org changed reviewers: + pkotwicz@chromium.org, vabr@chromium.org
components/password_manager/core/browser/* LGTM with some comments. Thanks! Vaclav https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... File components/password_manager/core/browser/affiliation_utils.h (right): https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... components/password_manager/core/browser/affiliation_utils.h:212: std::string GetShownOrigin(const GURL& url, const std::string& languages); I don't think this has anything to do with affiliation. Feel free to start a new file, e.g., passwords_ui_util.h/.cc or similar. https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... File components/password_manager/core/browser/affiliation_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:396: EXPECT_EQ(GetShownOrigin(GURL(test_case.input), ""), test_case.output); nit: The expected value goes first, so that EXPECT_EQ's error messaging makes sense. EXPECT_EQ(a, b), if a!= b, says something like: "actual = b, but expected a". So EXPECT_EQ(test_case.output, GetShownOrigin(GURL(test_case.input), "")); https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:396: EXPECT_EQ(GetShownOrigin(GURL(test_case.input), ""), test_case.output); Please also add the input to the error logs: EXPECT_EQ(...) << "for input " << test_case.input;
https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... ui/webui/resources/js/util.js:357: getFaviconImageSet('', 16); This code confuses me. Here is my understanding of the behavior you are introducing Favicon present for |url|: - Favicon used for both HTTP and HTTPS Favicon not present for url: - "Blank page" used for HTTP - "Solid gray tile with first letter" of path used for HTTPS On Android, we are going to replacing the usage of the blank page favicon with the solid color favicon soon. So I doubt that a user will attribute any meaning as to which type of fallback icon ("Blank page" or "Solid gray tile") is used. If the type of fallback icon is important, it would be preferable if you had a custom image asset to use as the fallback in the HTTP case. The favicon work seems to be unrelated to crbug.com/483915. Can you pull it out into a separate CL?
https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... ui/webui/resources/js/util.js:357: getFaviconImageSet('', 16); If the url is insecure, we show 'blank' icon. If the url is secure: if there is an icon, show the icon. otherwise show solid gray icon with first letter. UI team offered to distinguish secure and insecure origins with icons. It is not so hard for a user: if an icon is blank, the origins isn't secure. If there is some 'picture', it is secure.
https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... ui/webui/resources/js/util.js:357: getFaviconImageSet('', 16); Sorry for my misunderstanding. I think that we should not be hijacking the default favicon to have a special meaning. I am in favor of having a dedicated asset for insecure URLs. You should be able to access it via chrome://theme/IDR_FAVICON_INSECURE (Where IDR_FAVICON_INSECURE is a new image asset) We are not yet ready to start using gray tiles with a letter as the default icon in desktop Chrome UI. Making the switch is easiest if everything is switched over simultaneously. I have an OKR to switch over the Android UI this quarter.
https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:306: getIconBasedOnUrlSecurity(this.url, this.isUrlSecure); indent https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:43: // chrome://large-icon/16/fallback/,777,FFF,0.625,0.4/http://example.com I don't think you should be passing this fallback stuff into C++, can't you do this all in css? see how multiple background urls can be used as a fallback in case one fails to load: https://jsfiddle.net/82oqoepe/1/
https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:306: getIconBasedOnUrlSecurity(this.url, this.isUrlSecure); On 2015/10/15 23:46:32, Evan Stade wrote: > indent Done. https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/large_icon_source.h (right): https://codereview.chromium.org/1318523011/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/large_icon_source.h:43: // chrome://large-icon/16/fallback/,777,FFF,0.625,0.4/http://example.com Thanks. That is very interesting idea, but what if the regular icon is partially transparent. Is it possible to disable transparency of a background image? I didn't find so far. https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... File components/password_manager/core/browser/affiliation_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:396: EXPECT_EQ(GetShownOrigin(GURL(test_case.input), ""), test_case.output); On 2015/10/15 11:58:26, vabr (Chromium) wrote: > nit: The expected value goes first, so that EXPECT_EQ's error messaging makes > sense. EXPECT_EQ(a, b), if a!= b, says something like: "actual = b, but expected > a". So > > EXPECT_EQ(test_case.output, GetShownOrigin(GURL(test_case.input), "")); Done. https://codereview.chromium.org/1318523011/diff/200001/components/password_ma... components/password_manager/core/browser/affiliation_utils_unittest.cc:396: EXPECT_EQ(GetShownOrigin(GURL(test_case.input), ""), test_case.output); On 2015/10/15 11:58:26, vabr (Chromium) wrote: > Please also add the input to the error logs: > > EXPECT_EQ(...) << "for input " << test_case.input; Done. https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/1318523011/diff/200001/ui/webui/resources/js/... ui/webui/resources/js/util.js:357: getFaviconImageSet('', 16); Thanks for your advices. Could you share your opinion with UI team, please? I will add you as cc to email thread. Could you explain why we cannot use gray tiles with a letter?
Description was changed from ========== 1. Copiable username and origin. 2. Linkable origin elided from the left. 3. For insecure origins show default icon. If the secure origin has an icon, show it. Otherwise, show a fallback icon, i.e. an icon with the first letter of domain name. UI mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/copiable%20... BUG=483915 ========== to ========== 1. Copiable username and origin. 2. Linkable origin elided from the left. UI mocks: https://folio.googleplex.com/chrome-ux/mocks/321-password-manager/copiable%20... BUG=483915 ==========
Patchset #8 (id:240001) has been deleted
(is this patch still waiting for review? It seems like it's stuck on pkotwicz's concerns?
It was decided to postpone using the fallback icons. So, I rolled back some changes. Please review. Screenshots: https://code.google.com/p/chromium/issues/detail?id=483915#c19 https://code.google.com/p/chromium/issues/detail?id=483915#c20 vabr@chromium.org: GetShownOrigin was moved to a separate file. estade@chromium.org: Thanks for you email. It is not stuck. I waited for further updates from UI team and didn't send the CL to review. There is no issues so far. So, please review. pkotwicz@chromium.org: Thanks for your feedback. At this point, there is no files for you to review.
lgtm https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:26: input[type='text'].inactive-item { do you need the input part of this rule or is the class name enough? https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:73: */ i think this */ belongs on the previous line https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:13: /** @const */ var ORIGIN_FIELD = 'origin'; I'm not entirely convinced this is useful (as opposed to inlining strings) but I guess I don't care too much
https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:26: input[type='text'].inactive-item { You are right. The class name is enough, but we should override this rule https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... So, we should provide more specified rule ('input.inactive-item' or '#saved-passwords-list .inactive-item'). I chose 'input.inactive-item', because in the future we will also need '#password-exceptions-list.inactive-item'. 'input.inactive-item' will cover both cases. https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.css:73: */ On 2015/10/26 18:41:13, Evan Stade wrote: > i think this */ belongs on the previous line Done. https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:13: /** @const */ var ORIGIN_FIELD = 'origin'; Let me repeat my previous comment. It collects all information about the fields of PasswordListItem. If somebody want to change PasswordListItem, there is no need to check all code of password_manager_list.js, password_manager.js or some other files. I believe this code much less error-prone. I did some mistakes, because there were inlined numbers in password_manager_list.js and password_manager.js. I would prefer to have this.
https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/1318523011/diff/280001/chrome/browser/resourc... chrome/browser/resources/options/password_manager_list.js:13: /** @const */ var ORIGIN_FIELD = 'origin'; On 2015/10/27 08:24:36, kolos1 wrote: > Let me repeat my previous comment. > > It collects all information about the fields of PasswordListItem. If somebody > want to change PasswordListItem, there is no need to check all code of > password_manager_list.js, password_manager.js or some other files. well there probably is a need to check all code of those other files. Say I want to change 'username' somehow. Maybe we're tracking email instead. So I change 'username' to 'email' and USERNAME_FIELD to EMAIL_FIELD, and then I have to go update all the places that use USERNAME_FIELD. Although if I don't, Chrome will still compile just fine; I'm not even getting compile time checking. That said, I don't feel strongly, lg stands either way.
Thanks, Maxim. I left some comments at the 3 new files, but still LGTM. Cheers, Vaclav https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:7: #include <iosfwd> Seems not needed. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:9: #include "base/strings/string_util.h" Is this one needed? https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:18: const std::string kRemovedPrefixes[] = {"m.", "mobile.", "www."}; Having a global std::string is forbidden by the style, because of non-trivial constructors and destructors [1]. Please use const char* const kRemovedPrefixes[] = ... instead. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:26: for (std::string prefix : kRemovedPrefixes) { Please use base::StringPiece to avoid copying the prefixes into a new string. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:10: #include <iosfwd> I don't think you need <iosfwd>, but you need <string>. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2015 https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:8: #include "url/gurl.h" You can drop this #include, it is in the header file already. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:16: } kTestCases[] = { kTestCases suggest this is a constant, so please declare it as const struct.
Some small changes in password_ui_utils* https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:7: #include <iosfwd> On 2015/10/28 14:10:19, vabr (Chromium) wrote: > Seems not needed. Done. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:9: #include "base/strings/string_util.h" Yes, for base::CompareCase::INSENSITIVE_ASCII and base::StartsWith. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:18: const std::string kRemovedPrefixes[] = {"m.", "mobile.", "www."}; On 2015/10/28 14:10:18, vabr (Chromium) wrote: > Having a global std::string is forbidden by the style, because of non-trivial > constructors and destructors [1]. Please use > const char* const kRemovedPrefixes[] = ... > instead. > > [1] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... Done. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:26: for (std::string prefix : kRemovedPrefixes) { Sorry, don't understand. We don't copy |prefix|. We remove prefixes from the original string. Did you mean to declare |result| as StringPiece? I suppose that at most one prefix removing doesn't worth two type conversion (from std::string to StringPiece and back). https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.h (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.h:10: #include <iosfwd> On 2015/10/28 14:10:19, vabr (Chromium) wrote: > I don't think you need <iosfwd>, but you need <string>. Done. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils_unittest.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/10/28 14:10:19, vabr (Chromium) wrote: > 2014 -> 2015 Done. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:8: #include "url/gurl.h" On 2015/10/28 14:10:19, vabr (Chromium) wrote: > You can drop this #include, it is in the header file already. Done. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils_unittest.cc:16: } kTestCases[] = { On 2015/10/28 14:10:19, vabr (Chromium) wrote: > kTestCases suggest this is a constant, so please declare it as const struct. Done.
Hi Maxim, LGTM, but please do address the comment about StringPiece before landing. I'm happy to talk to you about that if you have any questions. Cheers, Vaclav https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:9: #include "base/strings/string_util.h" On 2015/10/29 13:55:25, kolos1 wrote: > Yes, for base::CompareCase::INSENSITIVE_ASCII and base::StartsWith. Acknowledged. https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:26: for (std::string prefix : kRemovedPrefixes) { On 2015/10/29 13:55:25, kolos1 wrote: > Sorry, don't understand. We don't copy |prefix|. We remove prefixes from the > original string. We do copy prefix, in the constructor of std::string which takes the element of kRemovedPrefixes. std::string always owns the characters, so it has to copy them. StringPiece, to the contrary, does not own the characters. Its constructor will just remember the address and length of the element in kRemovedPrefixes. > > Did you mean to declare |result| as StringPiece? That's actually also a good idea! In addition to using StringPiece for |prefix|, please use it also for |result|. Instead of std::string::substr, you can then use base::StringPiece::remove_prefix. > I suppose that at most one > prefix removing doesn't worth two type conversion (from std::string to > StringPiece and back). If you do what I suggested above, there will be no copying of characters until line 34, where |result| will be converted to a std::string. Currently, characters are copied on lines 25, 26, and 29. Note that converting to StringPiece is rather cheap, just creating or assigning a std::string is costly. https://codereview.chromium.org/1318523011/diff/320001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/320001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:18: const char* kRemovedPrefixes[] = {"m.", "mobile.", "www."}; const char* const instead of just const char* (the name style indicates kRemovedPrefixes is a constant, but const char* is a mutable pointer to const characters).
https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:26: for (std::string prefix : kRemovedPrefixes) { Ah, I see. Thanks.That's interesting point. I hope I fixed it in the intended way. https://codereview.chromium.org/1318523011/diff/320001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/320001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:18: const char* kRemovedPrefixes[] = {"m.", "mobile.", "www."}; Ah, you mentioned it in the previous comment. I missed it. Sorry. Done.
LGTM, thank you Maxim! Vaclav https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... File components/password_manager/core/browser/password_ui_utils.cc (right): https://codereview.chromium.org/1318523011/diff/300001/components/password_ma... components/password_manager/core/browser/password_ui_utils.cc:26: for (std::string prefix : kRemovedPrefixes) { On 2015/10/29 14:50:57, kolos1 wrote: > Ah, I see. Thanks.That's interesting point. I hope I fixed it in the intended > way. You did. Thanks!
The CQ bit was checked by kolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/1318523011/#ps340001 (title: "Changes with StringPiece")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1318523011/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1318523011/340001
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/00512644944fa28dff9272d31cb3292c17151992 Cr-Commit-Position: refs/heads/master@{#356866} |
