Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2542593003:
HTTP Bad: Add a seperator between http warning message and other entries (Closed)
|
Created:
4 years ago by lshang Modified:
4 years ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHTTP Bad: Add a seperator between http warning message and other entries
According to the mocks(go/fns-ui-spec), there should be a seperator between http
warning message and other autofill entries.
BUG=662298, 662297
Committed: https://crrev.com/a8edeaa9c328d890df581d29012f294f3ad38a29
Cr-Commit-Position: refs/heads/master@{#436205}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix test #Patch Set 3 : fix win trybots #
Messages
Total messages: 28 (20 generated)
Description was changed from ========== HTTP Bad: Add a seperator between http warning message and other entries BUG= ========== to ========== HTTP Bad: Add a seperator between http warning message and other entries According to the mocks(go/fns-ui-spec), there should be a seperator between http warning message and other autofill entries. BUG=662298, 662297 ==========
lshang@chromium.org changed reviewers: + mathp@chromium.org, vabr@chromium.org
PTAL thanks!
The CQ bit was checked by lshang@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM. The mocks are desktop-only and the new code specifically excludes Android (except for the tests, which are therefore failing). Just out of curiosity, is the plan for Android not to have the separator at all? Cheers, Vaclav https://codereview.chromium.org/2542593003/diff/1/components/password_manager... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2542593003/diff/1/components/password_manager... components/password_manager/core/browser/password_autofill_manager_unittest.cc:622: // there are 3 suggestions in total, and the message comes first among The comment is now out of date, there are 3 suggestions + 1 separator.
The CQ bit was checked by lshang@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...
On 2016/12/01 12:26:53, vabr (Chromium) wrote: > LGTM. The mocks are desktop-only and the new code specifically excludes Android > (except for the tests, which are therefore failing). Just out of curiosity, is > the plan for Android not to have the separator at all? Thanks! The mocks are for both mobile and desktop[1], but on Android, by default there is a separator for every dropdown item[2], so we don't need to explicitly add it in the suggestions vector, this is why you could see in a couple of places where Android is excluded when adding separators. [1]:https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%20Secure#%2FSpec.png%3Fpos=top-left&z=width&bg=black [2]: https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... https://codereview.chromium.org/2542593003/diff/1/components/password_manager... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2542593003/diff/1/components/password_manager... components/password_manager/core/browser/password_autofill_manager_unittest.cc:622: // there are 3 suggestions in total, and the message comes first among On 2016/12/01 12:26:53, vabr (Chromium) wrote: > The comment is now out of date, there are 3 suggestions + 1 separator. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/12/02 02:41:44, lshang wrote: > On 2016/12/01 12:26:53, vabr (Chromium) wrote: > > LGTM. The mocks are desktop-only and the new code specifically excludes > Android > > (except for the tests, which are therefore failing). Just out of curiosity, is > > the plan for Android not to have the separator at all? > > Thanks! The mocks are for both mobile and desktop[1], but on Android, by default > there is a separator for every dropdown item[2], so we don't need to explicitly > add it in the suggestions vector, this is why you could see in a couple of > places where Android is excluded when adding separators. > > [1]:https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%20Secure#%2FSpec.png%3Fpos=top-left&z=width&bg=black > [2]: > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop... > > https://codereview.chromium.org/2542593003/diff/1/components/password_manager... > File > components/password_manager/core/browser/password_autofill_manager_unittest.cc > (right): > > https://codereview.chromium.org/2542593003/diff/1/components/password_manager... > components/password_manager/core/browser/password_autofill_manager_unittest.cc:622: > // there are 3 suggestions in total, and the message comes first among > On 2016/12/01 12:26:53, vabr (Chromium) wrote: > > The comment is now out of date, there are 3 suggestions + 1 separator. > > Done. lgtm, thanks!
On 2016/12/02 02:41:44, lshang wrote: > On 2016/12/01 12:26:53, vabr (Chromium) wrote: > > LGTM. The mocks are desktop-only and the new code specifically excludes > Android > > (except for the tests, which are therefore failing). Just out of curiosity, is > > the plan for Android not to have the separator at all? > > Thanks! The mocks are for both mobile and desktop[1], but on Android, by default > there is a separator for every dropdown item[2], so we don't need to explicitly > add it in the suggestions vector, this is why you could see in a couple of > places where Android is excluded when adding separators. Thanks for the explanation, still LGTM! Cheers, Vaclav
The CQ bit was checked by lshang@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2542593003/#ps60001 (title: "fix win trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480894172020290,
"parent_rev": "663ca07c2eb417c1bed014a16b165d2161b55d76", "commit_rev":
"fdc7657cea22f48aa040e7650e85f4a9c4ddb645"}
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: Add a seperator between http warning message and other entries According to the mocks(go/fns-ui-spec), there should be a seperator between http warning message and other autofill entries. BUG=662298, 662297 ========== to ========== HTTP Bad: Add a seperator between http warning message and other entries According to the mocks(go/fns-ui-spec), there should be a seperator between http warning message and other autofill entries. BUG=662298, 662297 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: Add a seperator between http warning message and other entries According to the mocks(go/fns-ui-spec), there should be a seperator between http warning message and other autofill entries. BUG=662298, 662297 ========== to ========== HTTP Bad: Add a seperator between http warning message and other entries According to the mocks(go/fns-ui-spec), there should be a seperator between http warning message and other autofill entries. BUG=662298, 662297 Committed: https://crrev.com/a8edeaa9c328d890df581d29012f294f3ad38a29 Cr-Commit-Position: refs/heads/master@{#436205} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a8edeaa9c328d890df581d29012f294f3ad38a29 Cr-Commit-Position: refs/heads/master@{#436205} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
