Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(528)

Issue 1159143002: Add a basic functionality to the force-saving menu item. (Closed)

Created:
5 years, 6 months ago by msramek
Modified:
5 years, 6 months ago
CC:
browser-components-watch_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mkwst+watchlist-passwords_chromium.org, mlamouri+watch-content_chromium.org, rouslan+autofillwatch_chromium.org, vabr+watchlist_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a basic functionality to the force-saving menu item. Currently, clicking on this item in the context menu will invoke the password save bubble. The credentials can be saved, but password manager will not work correctly until the page is reloaded; this is WIP. BUG=484891 Committed: https://crrev.com/6bb816c5c0d8caed240931e87a0be2a9316f0112 Cr-Commit-Position: refs/heads/master@{#332028}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Changed message semantics. #

Total comments: 4

Patch Set 3 : Error handling in the autofill agent. #

Total comments: 2

Patch Set 4 : Added stub methods. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -1 line) Patch
M chrome/browser/password_manager/chrome_password_manager_client.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/password_manager/chrome_password_manager_client.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu.cc View 2 chunks +3 lines, -1 line 0 comments Download
M components/autofill/content/common/autofill_messages.h View 1 2 3 2 chunks +11 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 2 3 2 chunks +21 lines, -0 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.h View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.cc View 1 2 3 3 chunks +12 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.h View 1 chunk +5 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.cc View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager_client.h View 1 chunk +4 lines, -0 lines 1 comment Download
M components/password_manager/core/browser/password_manager_driver.h View 1 2 3 1 chunk +4 lines, -0 lines 1 comment Download
M components/password_manager/core/browser/stub_password_manager_client.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/core/browser/stub_password_manager_client.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/stub_password_manager_driver.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/core/browser/stub_password_manager_driver.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (3 generated)
msramek
Hi Mike, Václav and Avi, please take a look. Mike: autofill_messages Avi: render_view_context_menu Václav: password_manager ...
5 years, 6 months ago (2015-05-28 19:10:59 UTC) #2
Avi (use Gerrit)
On 2015/05/28 19:10:59, msramek wrote: > Avi: render_view_context_menu LGTM
5 years, 6 months ago (2015-05-28 19:15:08 UTC) #3
vabr (Chromium)
LGTM with a couple of nits and a question. Cheers, Vaclav https://codereview.chromium.org/1159143002/diff/1/components/autofill/content/common/autofill_messages.h File components/autofill/content/common/autofill_messages.h (right): ...
5 years, 6 months ago (2015-05-28 19:41:24 UTC) #4
msramek
https://codereview.chromium.org/1159143002/diff/1/components/autofill/content/common/autofill_messages.h File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/1159143002/diff/1/components/autofill/content/common/autofill_messages.h#newcode191 components/autofill/content/common/autofill_messages.h:191: // can be saved. On 2015/05/28 19:41:23, vabr (Chromium) ...
5 years, 6 months ago (2015-05-28 20:28:28 UTC) #5
vabr (Chromium)
LGTM, thanks! Vaclav https://codereview.chromium.org/1159143002/diff/1/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1159143002/diff/1/components/autofill/content/renderer/password_autofill_agent.cc#newcode1254 components/autofill/content/renderer/password_autofill_agent.cc:1254: ProvisionallySavePassword(input.form(), RESTRICTION_NONE); On 2015/05/28 20:28:28, msramek ...
5 years, 6 months ago (2015-05-29 06:48:50 UTC) #6
Mike West
https://codereview.chromium.org/1159143002/diff/20001/components/autofill/content/common/autofill_messages.h File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/1159143002/diff/20001/components/autofill/content/common/autofill_messages.h#newcode347 components/autofill/content/common/autofill_messages.h:347: // Inform the browser which password form is currently ...
5 years, 6 months ago (2015-05-29 07:24:49 UTC) #7
msramek
https://codereview.chromium.org/1159143002/diff/20001/components/autofill/content/common/autofill_messages.h File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/1159143002/diff/20001/components/autofill/content/common/autofill_messages.h#newcode347 components/autofill/content/common/autofill_messages.h:347: // Inform the browser which password form is currently ...
5 years, 6 months ago (2015-05-29 11:57:21 UTC) #8
Mike West
Thanks for taking another pass. LGTM % optional nit. https://codereview.chromium.org/1159143002/diff/40001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1159143002/diff/40001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1259 components/autofill/content/renderer/password_autofill_agent.cc:1259: ...
5 years, 6 months ago (2015-05-29 12:26:53 UTC) #9
msramek
https://codereview.chromium.org/1159143002/diff/40001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1159143002/diff/40001/components/autofill/content/renderer/password_autofill_agent.cc#newcode1259 components/autofill/content/renderer/password_autofill_agent.cc:1259: password_form.reset(new PasswordForm()); On 2015/05/29 12:26:53, Mike West wrote: > ...
5 years, 6 months ago (2015-05-29 12:38:55 UTC) #10
msramek
I added empty methods to the password manager client and driver. This is just a ...
5 years, 6 months ago (2015-05-29 18:45:38 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159143002/60001
5 years, 6 months ago (2015-05-29 18:47:14 UTC) #14
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 6 months ago (2015-05-29 19:37:07 UTC) #15
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/6bb816c5c0d8caed240931e87a0be2a9316f0112 Cr-Commit-Position: refs/heads/master@{#332028}
5 years, 6 months ago (2015-05-29 19:37:51 UTC) #16
vabr (Chromium)
Thanks for the explanation in #11. However, to minimize churn for iOS, non-essential methods in ...
5 years, 6 months ago (2015-06-01 07:17:21 UTC) #17
msramek
I see. My understanding is that while the used UI is experimental, the functionality itself ...
5 years, 6 months ago (2015-06-01 08:05:11 UTC) #18
vabr (Chromium)
5 years, 6 months ago (2015-06-01 08:47:27 UTC) #19
Message was sent while issue was closed.
On 2015/06/01 08:05:11, msramek wrote:
> I see. My understanding is that while the used UI is experimental, the
> functionality itself is not considered non-essential, and thus I declared it
as
> pure virtual.
You are right in that once it is no longer experimental, it will be essential.
At that moment we will have an iOS implementation, and will be able to make the
interface method pure virtual again.

> 
> However, you're right that we don't want to worry about al OS right now. Sorry
> for that; I'll send a follow-up CL shortly.

Thanks!

Vaclav

Powered by Google App Engine
This is Rietveld 408576698