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

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

Created:
10 months ago by leonhsl(Using Gerrit)
Modified:
10 months 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@ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -41 lines) Patch
M chrome/renderer/autofill/fake_content_password_manager_driver.h View 3 chunks +0 lines, -6 lines 0 comments Download
M chrome/renderer/autofill/fake_content_password_manager_driver.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/renderer/autofill/password_autofill_agent_browsertest.cc View 1 chunk +0 lines, -7 lines 0 comments Download
M components/autofill/content/common/autofill_driver.mojom View 1 chunk +0 lines, -5 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.h View 1 chunk +0 lines, -1 line 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.cc View 1 2 chunks +13 lines, -4 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver_unittest.cc View 2 chunks +11 lines, -11 lines 0 comments Download

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.
10 months 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 ...
10 months ago (2017-02-09 13:51:01 UTC) #8
kolos1
LGTM Great thanks for fixing that!
10 months 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 ...
10 months 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 ...
10 months 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 ...
10 months 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
10 months 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)
10 months 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.
10 months ago (2017-02-13 01:53:55 UTC) #19
Tom Sepez
LGTM on removing method.
10 months 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
10 months ago (2017-02-14 00:16:01 UTC) #22
commit-bot: I haz the power
10 months 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 0eb02b776