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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by leonhsl
Modified:
4 months, 1 week 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 25 (13 generated)
leonhsl
Hi, vabr@, kolos@, I've confirmed locally that this CL does fix bug 676814, PTAL, Thanks.
4 months, 2 weeks 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 ...
4 months, 1 week ago (2017-02-09 13:51:01 UTC) #8
kolos1
LGTM Great thanks for fixing that!
4 months, 1 week ago (2017-02-09 13:57:37 UTC) #9
leonhsl
On 2017/02/09 13:51:01, kolos1 wrote: > The password manager works in custom tabs even after ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-02-12 18:48:13 UTC) #11
leonhsl
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 ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week ago (2017-02-13 00:20:32 UTC) #17
leonhsl
+Tom for OWNER review of components/autofill/content/common/autofill_driver.mojom, Thanks.
4 months, 1 week ago (2017-02-13 01:53:55 UTC) #19
Tom Sepez
LGTM on removing method.
4 months, 1 week 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
4 months, 1 week ago (2017-02-14 00:16:01 UTC) #22
commit-bot: I haz the power
4 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318