Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466143002:
iOS: Mark HTTP pages with password fields with an omnibox icon. (Closed)
DescriptioniOS: Mark HTTP pages with password fields with an omnibox icon.
BUG=647822
================================
TEST=Use an iPhone, not an iPad. First, enable the proper flag:
--------------------------------
1. Open the Settings app
2. Scroll to Chrome Beta/Dev/Canary and press
3. Scroll down to Experimental Settings and press
4. Scroll to EXTRA FLAGS (ONE PER LINE)
5. Toggle "Append Extra Flags" to ON
6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes)
--------------------------------
Test 3 URLs:
1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL.
2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL.
3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL.
--------------------------------
4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL.
================================
Committed: https://crrev.com/5f03ba5ae3b6fa1059da663fa8c86f8846b16e31
Cr-Commit-Position: refs/heads/master@{#438721}
Patch Set 1 #Patch Set 2 : iOS HTTP Bad. #Patch Set 3 : iOS HTTP Bad. #Patch Set 4 : iOS HTTP Bad. #
Total comments: 25
Patch Set 5 : IsOriginSecure() #
Total comments: 4
Patch Set 6 : Address nits. #Patch Set 7 #
Total comments: 10
Patch Set 8 : Add comments #Patch Set 9 : iOS HTTP Bad. #Messages
Total messages: 45 (27 generated)
Description was changed from ========== Temp commit. BUG=647822 ========== to ========== iOS HTTP Bad BUG=647822 ==========
Description was changed from ========== iOS HTTP Bad BUG=647822 ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ==========
lgarron@chromium.org changed reviewers: + eugenebut@chromium.org
eugenebut@, could you review? https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:45: #include "ios/web/web_state/web_state_impl.h" This is giving me: You added one or more #includes that violate checkdeps rules. ios/chrome/browser/passwords/password_controller.mm Illegal include: "ios/web/web_state/web_state_impl.h" Should we update the deps, or do I need to work around this? https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:555: // TODO: Only do this for HTTP. Note: Haven't gotten to this, since it's the "easy" part. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:235: - (void)didShowSensitiveInputOnHttp; Is this the appropriate way to adapt OnSensitiveInputShownOnHttp to Cocoa? Or should I just name it OnSensitiveInputShownOnHttp directly? (Should this match the method on the web state?) https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4594: // TODO: Get the current item, not the visible item. The implementation of webControllerDidUpdateSSLStatusForCurrentNavigationItem: actually uses GetTransientItem(). Also, there is no "current item" method, on the controller, only: - GetVisibleItem() - GetLastCommittedItem() - GetPendingItem() - GetTransientItem() Which should I use? GetTransientItem() is consistent with didUpdateSSLStatusForCurrentNavigationItem, but sounds the second-least-accurate.
estark@chromium.org changed reviewers: + estark@chromium.org
Cool! I left some drive-by comments. Also would suggest writing some tests. https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:45: #include "ios/web/web_state/web_state_impl.h" On 2016/11/29 03:48:49, lgarron wrote: > This is giving me: > > You added one or more #includes that violate checkdeps rules. > ios/chrome/browser/passwords/password_controller.mm > Illegal include: "ios/web/web_state/web_state_impl.h" > > Should we update the deps, or do I need to work around this? I'm guessing ios/chrome is only allowed to depend on public ios/web interfaces, just as //chrome is only allowed to depend on //content/public. Can you instead move the OnSensitiveInputShownOnHttp method on to the public WebState interface in ios/web/public/web_state/web_state.h? https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:553: // account for the security nit: unfinished sentence? https://codereview.chromium.org/2466143002/diff/60001/ios/web/public/ssl_stat... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/public/ssl_stat... ios/web/public/ssl_status.h:29: DISPLAYED_PASSWORD_FIELD_ON_HTTP = 1 << 4, nit: add a comment explaining https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4597: item->GetSSL().security_style = web::SECURITY_STYLE_AUTHENTICATED; I don't think you should be setting the security_style or DISPLAYED_INSECURE_CONTENT here, unless I'm missing something. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4600: web::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP; Are you planning to later expand this method to handle credit cards too? Or do that as a separate method? If the latter, maybe this should be named didShowPasswrodInputOnHttp.
+1 for writing tests https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:45: #include "ios/web/web_state/web_state_impl.h" On 2016/11/29 21:28:48, estark wrote: > On 2016/11/29 03:48:49, lgarron wrote: > > This is giving me: > > > > You added one or more #includes that violate checkdeps rules. > > ios/chrome/browser/passwords/password_controller.mm > > Illegal include: "ios/web/web_state/web_state_impl.h" > > > > Should we update the deps, or do I need to work around this? > > I'm guessing ios/chrome is only allowed to depend on public ios/web interfaces, > just as //chrome is only allowed to depend on //content/public. > > Can you instead move the OnSensitiveInputShownOnHttp method on to the public > WebState interface in ios/web/public/web_state/web_state.h? Yep, ios/chrome can not depend on non-public ios/web. Same as for content. https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:60: ? true nit: do you need ternary here? |ssl.content_status & web::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP| perfectly converts to bool https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:235: - (void)didShowSensitiveInputOnHttp; On 2016/11/29 03:48:49, lgarron wrote: > Is this the appropriate way to adapt OnSensitiveInputShownOnHttp to Cocoa? > Or should I just name it OnSensitiveInputShownOnHttp directly? (Should this > match the method on the web state?) The naming pattern is correct. Please use all caps for HTTP though to follow Objective-C Style Guide. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:235: - (void)didShowSensitiveInputOnHttp; Please add comments. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4594: // TODO: Get the current item, not the visible item. On 2016/11/29 03:48:49, lgarron wrote: > The implementation of webControllerDidUpdateSSLStatusForCurrentNavigationItem: > actually uses GetTransientItem(). > > Also, there is no "current item" method, on the controller, only: > > - GetVisibleItem() > - GetLastCommittedItem() > - GetPendingItem() > - GetTransientItem() > > Which should I use? > GetTransientItem() is consistent with > didUpdateSSLStatusForCurrentNavigationItem, but sounds the > second-least-accurate. Tab uses transient item to exit from fullscreen if interstitial is shown. Which API do you use for other platforms? Is there anything suitable in CRWSessionController?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
eugenebut@, could you review? I believe I've addressed everything except the transient item inconsistency. https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... File ios/chrome/browser/passwords/password_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:45: #include "ios/web/web_state/web_state_impl.h" On 2016/11/30 at 17:58:41, Eugene But wrote: > On 2016/11/29 21:28:48, estark wrote: > > On 2016/11/29 03:48:49, lgarron wrote: > > > This is giving me: > > > > > > You added one or more #includes that violate checkdeps rules. > > > ios/chrome/browser/passwords/password_controller.mm > > > Illegal include: "ios/web/web_state/web_state_impl.h" > > > > > > Should we update the deps, or do I need to work around this? > > > > I'm guessing ios/chrome is only allowed to depend on public ios/web interfaces, > > just as //chrome is only allowed to depend on //content/public. > > > > Can you instead move the OnSensitiveInputShownOnHttp method on to the public > > WebState interface in ios/web/public/web_state/web_state.h? > Yep, ios/chrome can not depend on non-public ios/web. Same as for content. I've added it to the public interface (and removed the static_cast in below in this file.) https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:553: // account for the security On 2016/11/29 at 21:28:48, estark wrote: > nit: unfinished sentence? Fixed. https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/pass... ios/chrome/browser/passwords/password_controller.mm:555: // TODO: Only do this for HTTP. On 2016/11/29 at 03:48:49, lgarron wrote: > Note: Haven't gotten to this, since it's the "easy" part. Done. https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:60: ? true On 2016/11/30 at 17:58:41, Eugene But wrote: > nit: do you need ternary here? |ssl.content_status & web::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP| perfectly converts to bool I copied state->displayed_mixed_content for consistency. Should I be inconsistent (or convert state->displayed_mixed_content)? https://codereview.chromium.org/2466143002/diff/60001/ios/web/public/ssl_stat... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/public/ssl_stat... ios/web/public/ssl_status.h:29: DISPLAYED_PASSWORD_FIELD_ON_HTTP = 1 << 4, On 2016/11/29 at 21:28:48, estark wrote: > nit: add a comment explaining Done. (I based the wording off the desktop constant.) https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.h (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.h:235: - (void)didShowSensitiveInputOnHttp; On 2016/11/30 at 17:58:41, Eugene But wrote: > On 2016/11/29 03:48:49, lgarron wrote: > > Is this the appropriate way to adapt OnSensitiveInputShownOnHttp to Cocoa? > > Or should I just name it OnSensitiveInputShownOnHttp directly? (Should this > > match the method on the web state?) > The naming pattern is correct. Please use all caps for HTTP though to follow Objective-C Style Guide. Done. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4594: // TODO: Get the current item, not the visible item. On 2016/11/30 at 17:58:41, Eugene But wrote: > On 2016/11/29 03:48:49, lgarron wrote: > > The implementation of webControllerDidUpdateSSLStatusForCurrentNavigationItem: > > actually uses GetTransientItem(). > > > > Also, there is no "current item" method, on the controller, only: > > > > - GetVisibleItem() > > - GetLastCommittedItem() > > - GetPendingItem() > > - GetTransientItem() > > > > Which should I use? > > GetTransientItem() is consistent with > > didUpdateSSLStatusForCurrentNavigationItem, but sounds the > > second-least-accurate. > Tab uses transient item to exit from fullscreen if interstitial is shown. Which API do you use for other platforms? Is there anything suitable in CRWSessionController? Desktop uses LastCommittedEntry() [1]. Ive updated this callsite, and also used the last committed URL in the password manager. However, this function still calls webControllerDidUpdateSSLStatusForCurrentNavigationItem, which uses the transient entry. Is that safe, or should I mage a version of webControllerDidUpdateSSLStatusForCurrentNavigationItem that uses the last committed entry? [1] https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_manager.cc?dr=CS... https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4597: item->GetSSL().security_style = web::SECURITY_STYLE_AUTHENTICATED; On 2016/11/29 at 21:28:48, estark wrote: > I don't think you should be setting the security_style or DISPLAYED_INSECURE_CONTENT here, unless I'm missing something. Removed. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4600: web::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP; On 2016/11/29 at 21:28:48, estark wrote: > Are you planning to later expand this method to handle credit cards too? Or do that as a separate method? If the latter, maybe this should be named didShowPasswrodInputOnHttp. Possibly. I've renamed to didShowPasswordInputOnHTTP.
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=Test 3 URLs on an iPhone (not iPad): 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. ==========
Per Emily's comment, could you please add unit tests for WebController. Otherwise everything looks good. Also could you please add downstream integration test for this in interstitial_egtest.mm (the name is misleading and I will rename this test at some point). https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/chrome/browser/ssl/... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:60: ? true On 2016/12/02 01:21:52, lgarron wrote: > On 2016/11/30 at 17:58:41, Eugene But wrote: > > nit: do you need ternary here? |ssl.content_status & > web::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP| perfectly converts to bool > > I copied state->displayed_mixed_content for consistency. > Should I be inconsistent (or convert state->displayed_mixed_content)? Sorry, did not notice the previous line :) Feel free to leave as it is or remove ternary from both places. It's up to you. https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4594: // TODO: Get the current item, not the visible item. On 2016/12/02 01:21:52, lgarron wrote: > On 2016/11/30 at 17:58:41, Eugene But wrote: > > On 2016/11/29 03:48:49, lgarron wrote: > > > The implementation of > webControllerDidUpdateSSLStatusForCurrentNavigationItem: > > > actually uses GetTransientItem(). > > > > > > Also, there is no "current item" method, on the controller, only: > > > > > > - GetVisibleItem() > > > - GetLastCommittedItem() > > > - GetPendingItem() > > > - GetTransientItem() > > > > > > Which should I use? > > > GetTransientItem() is consistent with > > > didUpdateSSLStatusForCurrentNavigationItem, but sounds the > > > second-least-accurate. > > Tab uses transient item to exit from fullscreen if interstitial is shown. > Which API do you use for other platforms? Is there anything suitable in > CRWSessionController? > > Desktop uses LastCommittedEntry() [1]. > Ive updated this callsite, and also used the last committed URL in the password > manager. > > However, this function still calls > webControllerDidUpdateSSLStatusForCurrentNavigationItem, which uses the > transient entry. Is that safe, or should I mage a version of > webControllerDidUpdateSSLStatusForCurrentNavigationItem that uses the last > committed entry? > > [1] > https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_manager.cc?dr=CS... There is no ned to change Tab. It uses pending entry to check for interstitial presence, which is orthogonal to password field warning. https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:4596: item->GetSSL().content_status |= Do you want to DCHECK that origin is not secure? https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... ios/web/web_state/web_state_impl.mm:291: void WebStateImpl::OnPasswordInputShownOnHttp() { nit: Could you please try keeping the same order for this method as in the header.
Still trying to figure out how to use TestWebState to test this. Doing a CQ dry run to see if we broke anything. https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:4596: item->GetSSL().content_status |= On 2016/12/02 at 01:53:12, Eugene But wrote: > Do you want to DCHECK that origin is not secure? Good idea; done. https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... ios/web/web_state/web_state_impl.mm:291: void WebStateImpl::OnPasswordInputShownOnHttp() { On 2016/12/02 at 01:53:12, Eugene But wrote: > nit: Could you please try keeping the same order for this method as in the header. Whoops, fixed.
Still trying to figure out how to use TestWebState to test this. Doing a CQ dry run to see if we broke anything. https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:4596: item->GetSSL().content_status |= On 2016/12/02 at 01:53:12, Eugene But wrote: > Do you want to DCHECK that origin is not secure? Good idea; done. https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... ios/web/web_state/web_state_impl.mm:291: void WebStateImpl::OnPasswordInputShownOnHttp() { On 2016/12/02 at 01:53:12, Eugene But wrote: > nit: Could you please try keeping the same order for this method as in the header. Whoops, fixed.
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
On 2016/12/06 01:38:36, lgarron wrote: > Still trying to figure out how to use TestWebState to test this. Maybe TestWebState isn't the right way to test this since it doesn't have a NavigationManager... maybe unit tests for WebController and an interstitial_egtest.mm as Eugene suggested is the way to go instead? > > Doing a CQ dry run to see if we broke anything. > > https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller.mm:4596: item->GetSSL().content_status > |= > On 2016/12/02 at 01:53:12, Eugene But wrote: > > Do you want to DCHECK that origin is not secure? > > Good idea; done. > > https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... > File ios/web/web_state/web_state_impl.mm (right): > > https://codereview.chromium.org/2466143002/diff/120001/ios/web/web_state/web_... > ios/web/web_state/web_state_impl.mm:291: void > WebStateImpl::OnPasswordInputShownOnHttp() { > On 2016/12/02 at 01:53:12, Eugene But wrote: > > nit: Could you please try keeping the same order for this method as in the > header. > > Whoops, fixed.
Patchset #7 (id:160001) has been deleted
Now with 100% more unit tests! eugenebut@, could you review?
lgtm! Thank you for the tests. https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... File ios/chrome/browser/passwords/password_controller_unittest.mm (right): https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:32: #include "ios/web/public/navigation_item.h" s/include/import https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:33: #include "ios/web/public/navigation_manager.h" ditto https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:1303: TEST_F(PasswordControllerTest, HTTPNoPassword) { s/HTTPNoPassword/HttpNoPassword https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:1303: TEST_F(PasswordControllerTest, HTTPNoPassword) { Please add comments for test methods https://codereview.chromium.org/2466143002/diff/180001/ios/web/public/test/te... File ios/web/public/test/test_web_state.h (right): https://codereview.chromium.org/2466143002/diff/180001/ios/web/public/test/te... ios/web/public/test/test_web_state.h:60: void OnPasswordInputShownOnHttp() override{}; No space before |{| and drop |;|
Patchset #8 (id:200001) has been deleted
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % Eugene's comments. And per discussion IRL last night, I don't fully understand how this works (i.e. how the icon actually shows up) because the iOS code doesn't explicitly handle the HTTP_SHOW_WARNING security level anywhere. So it would be good to understand that (maybe add a comment explaining it?).
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=Test 3 URLs on an iPhone (not iPad): 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=First, enable the proper flag: Settings app > Chrome Beta/Dev/Canary > Experimental Settings > EXTRA FLAGS (ONE PER LINE) > toggle "Append Extra Flags" to ON, and set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) Test 3 URLs on an iPhone (not iPad): 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. ==========
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=First, enable the proper flag: Settings app > Chrome Beta/Dev/Canary > Experimental Settings > EXTRA FLAGS (ONE PER LINE) > toggle "Append Extra Flags" to ON, and set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) Test 3 URLs on an iPhone (not iPad): 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- ==========
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/password/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http.badssl.com/input/password/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ================================ TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- 4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL. ================================ ==========
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2466143002/#ps240001 (title: "iOS HTTP Bad.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
HTTP_SHOW_WARNING will be handled by [1] and [2]. [1] https://codereview.chromium.org/2574733003 [2] https://chromereviews.googleplex.com/556027013 Emily and I now also understand how flags work in local iOS builds, which do the correct thing. https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... File ios/chrome/browser/passwords/password_controller_unittest.mm (right): https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:32: #include "ios/web/public/navigation_item.h" On 2016/12/08 at 01:48:43, Eugene But wrote: > s/include/import I originally had import, but all these three imports have C++ classes, and at least one is accompanied by a .cc file. How do I tell which do use? (I can't find any documentation on it.) https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:1303: TEST_F(PasswordControllerTest, HTTPNoPassword) { On 2016/12/08 at 01:48:43, Eugene But wrote: > s/HTTPNoPassword/HttpNoPassword Comments added. This file also has a test called SaveOnNonHTMLLandingPage, so I followed that convention. If CamelCase is the correct choice, should I rename the other to SaveOnNonHtmlLandingPage? https://codereview.chromium.org/2466143002/diff/180001/ios/web/public/test/te... File ios/web/public/test/test_web_state.h (right): https://codereview.chromium.org/2466143002/diff/180001/ios/web/public/test/te... ios/web/public/test/test_web_state.h:60: void OnPasswordInputShownOnHttp() override{}; On 2016/12/08 at 01:48:43, Eugene But wrote: > No space before |{| and drop |;| Done.
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481770130098140,
"parent_rev": "625b90d8c042816055c49982417a00ce98120ea3", "commit_rev":
"7f930336f0570d90c07de1aa4402dc8e15fc0848"}
Message was sent while issue was closed.
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ================================ TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- 4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL. ================================ ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ================================ TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- 4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL. ================================ Review-Url: https://codereview.chromium.org/2466143002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ================================ TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- 4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL. ================================ Review-Url: https://codereview.chromium.org/2466143002 ========== to ========== iOS: Mark HTTP pages with password fields with an omnibox icon. BUG=647822 ================================ TEST=Use an iPhone, not an iPad. First, enable the proper flag: -------------------------------- 1. Open the Settings app 2. Scroll to Chrome Beta/Dev/Canary and press 3. Scroll down to Experimental Settings and press 4. Scroll to EXTRA FLAGS (ONE PER LINE) 5. Toggle "Append Extra Flags" to ON 6. Set Flag1 to "--mark-non-secure-as=show-non-secure-passwords-cc-ui" (without the quotes) -------------------------------- Test 3 URLs: 1) Visit https://badssl.com/input/login/ and verify that the omnibox security has a green lock security indicator to the left of the URL. 2) Visit http://http-login.badssl.com/ and verify that the omnibox security has a grey info icon ⓘ security indicator to the left of the URL. 3) Visit http://http.badssl.com/ and verify that the omnibox does *not* have a security indicator to the left of the URL. -------------------------------- 4) Set Flag1 (see above) to "--mark-non-secure-as=neutral" (without the quotes) and check that http://http-login.badssl.com/ does *not* have a security indicator to the left of the URL. ================================ Committed: https://crrev.com/5f03ba5ae3b6fa1059da663fa8c86f8846b16e31 Cr-Commit-Position: refs/heads/master@{#438721} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5f03ba5ae3b6fa1059da663fa8c86f8846b16e31 Cr-Commit-Position: refs/heads/master@{#438721}
Message was sent while issue was closed.
https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... File ios/chrome/browser/passwords/password_controller_unittest.mm (right): https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:32: #include "ios/web/public/navigation_item.h" On 2016/12/15 03:04:31, lgarron wrote: > On 2016/12/08 at 01:48:43, Eugene But wrote: > > s/include/import > > I originally had import, but all these three imports have C++ classes, and at > least one is accompanied by a .cc file. > How do I tell which do use? (I can't find any documentation on it.) Style Guide sais "#import Objective-C/Objective-C++ headers, and #include C/C++ headers": https://google.github.io/styleguide/objcguide.xml#_import_and__include So if you see #import or @ in the header, it is an Objective-C header for sure and it can not be compiled by C++ compiler. This rule will work for 99% of cases. https://codereview.chromium.org/2466143002/diff/180001/ios/chrome/browser/pas... ios/chrome/browser/passwords/password_controller_unittest.mm:1303: TEST_F(PasswordControllerTest, HTTPNoPassword) { On 2016/12/15 03:04:31, lgarron wrote: > On 2016/12/08 at 01:48:43, Eugene But wrote: > > s/HTTPNoPassword/HttpNoPassword > > Comments added. > > This file also has a test called SaveOnNonHTMLLandingPage, so I followed that > convention. > If CamelCase is the correct choice, should I rename the other to > SaveOnNonHtmlLandingPage? Camel case is correct choice per C++ Style Guide, but Chromium code is extremely inconsistent with it so if surrounding code uses all caps, then it's perfectly fine to keep all caps. |
