Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(460)

Issue 2680163006: Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver (Closed)

Created:
1 year ago by leonhsl(Using Gerrit)
Modified:
1 year ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, Aaron Boodman, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, viettrungluu+watch_chromium.org, browser-components-watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, darin (slow to review), qsr+mojo_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent to get logging state from browser side when PasswordAutofillAgent is created. It's useful before due to lack of pending mechanism for legacy IPCs, but now in mojo world we can achieve this goal without it. This CL lets browser side just send logging state as soon as it's ready, even at that timing point renderer process has not started or has not created PasswordAutofillAgent, the mojo call autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached inside mojo message pipe and be executed immediately once PasswordAutofillAgent instantiates and binds itself with the pending mojo connection. This keeps exactly the same behavior with before: autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first mojo call arriving at autofill::PasswordAutofillAgent from browser side. BUG=676814 TEST=components_unittests Review-Url: https://codereview.chromium.org/2680163006 Cr-Commit-Position: refs/heads/master@{#450171} Committed: https://chromium.googlesource.com/chromium/src/+/2364c1b8adb850f0d1b1e29f16ecfd75679280d5

Patch Set 1 #

Total comments: 2

Patch Set 2 : Address comments from vabr@ #

Messages

Total messages: 25 (13 generated)
leonhsl(Using Gerrit)
Hi, vabr@, kolos@, I've confirmed locally that this CL does fix bug 676814, PTAL, Thanks.
1 year ago (2017-02-09 09:45:18 UTC) #4
kolos1
The password manager works in custom tabs even after "warmup". Could you please explain what ...
1 year ago (2017-02-09 13:51:01 UTC) #8
kolos1
LGTM Great thanks for fixing that!
1 year ago (2017-02-09 13:57:37 UTC) #9
leonhsl(Using Gerrit)
On 2017/02/09 13:51:01, kolos1 wrote: > The password manager works in custom tabs even after ...
1 year ago (2017-02-10 06:24:26 UTC) #10
vabr (Chromium)
Thanks for the fix and for the helpful explanations both in the CL description and ...
1 year ago (2017-02-12 18:48:13 UTC) #11
leonhsl(Using Gerrit)
Thanks all! Sending to CQ now. https://codereview.chromium.org/2680163006/diff/1/components/password_manager/content/browser/content_password_manager_driver.cc File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2680163006/diff/1/components/password_manager/content/browser/content_password_manager_driver.cc#newcode54 components/password_manager/content/browser/content_password_manager_driver.cc:54: GetPasswordAutofillAgent()->SetLoggingState( On 2017/02/12 ...
1 year ago (2017-02-13 00:14:32 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2680163006/20001
1 year ago (2017-02-13 00:15:02 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/362476)
1 year ago (2017-02-13 00:20:32 UTC) #17
leonhsl(Using Gerrit)
+Tom for OWNER review of components/autofill/content/common/autofill_driver.mojom, Thanks.
1 year ago (2017-02-13 01:53:55 UTC) #19
Tom Sepez
LGTM on removing method.
1 year ago (2017-02-13 17:55:56 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2680163006/20001
1 year ago (2017-02-14 00:16:01 UTC) #22
commit-bot: I haz the power
1 year ago (2017-02-14 00:31:24 UTC) #25
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2364c1b8adb850f0d1b1e29f16ec...

Powered by Google App Engine
This is Rietveld 408576698