Add a "Save password" item to the context menu.

Add a "Save password" item to the context menu.

The item is only shown if
 - force-saving is enabled by flag
 - the context menu is invoked on a password field

There is currently no functionality.

BUG=484891
Committed: https://crrev.com/1cceccd889984e70980ccaddb7a40eb1e7e95b96
Cr-Commit-Position: refs/heads/master@{#331346}

[msramek, 2015-05-19 09:13:57]

Patchset #1 (id:1) has been deleted

[msramek, 2015-05-19 09:42:07]

msramek@chromium.org changed reviewers:
+ avi@chromium.org, grt@chromium.org, isherman@chromium.org, mkwst@chromium.org,
sdefresne@chromium.org, vabr@chromium.org

[msramek, 2015-05-19 09:42:08]

Hi guys,

I'm adding a new button to the context menu (currently hidden under a flag and
with no action yet).

Could you please have a look?

+grt@ for chrome/app/
+sdefresne@ for components/ (that's a bit broad, but there isn't a more
particular OWNERS file)
+mkwst@ for frame_messages.h
+avi@ for renderer_context_menu and context_menu_params
+isherman@ for histograms
+vabr@ FYI

Thanks,
Martin

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
content/renderer/context_menu_params_builder.cc:87: // Find out if this node is
a password field.
Is it OK to add a field to ContextMenuParams that is not directly represented in
blink::WebContextMenuData? Or should I commit to Webkit first?

[sdefresne, 2015-05-19 09:51:15]

components/renderer_context_menu LGTM

BTW, I think you should add OWNERS to components/renderer_context_menu and I
would suggest just copying chrome/browser/renderer_context_menu/OWNERS.

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1151: +            Save password
The description says it should be in Title Case, so it should be "Save
Password".

[vabr (Chromium), 2015-05-19 09:58:13]

LGTM with nits.

Thanks!
Vaclav

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1150: +          <message
name="IDS_CONTENT_CONTEXT_FORCESAVEPASSWORD" desc="In Title Case: The name of
the Save Password command in the content area context menu">
Should the name be different from the other one?

https://codereview.chromium.org/1142923002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1142923002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1049: if
(password_manager::ForceSavingExperimentEnabled()) {
nit: To spare indenting, we usually do
if (!experiment enabled)
  return

// The rest of the stuff, at the base level indentation.

[Mike West, 2015-05-19 10:11:22]

https://codereview.chromium.org/1142923002/diff/20001/content/public/common/c...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/1142923002/diff/20001/content/public/common/c...
content/public/common/context_menu_params.h:170: bool is_password_field;
I would suggest turning this into an enum of possible field types, as I can
imagine that this feature will end up needing to do strange things for username
fields, and it would be a shame to need to tack additional fields onto an
already-huge struct to do so.

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
content/renderer/context_menu_params_builder.cc:87: // Find out if this node is
a password field.
On 2015/05/19 at 09:42:08, msramek wrote:
> Is it OK to add a field to ContextMenuParams that is not directly represented
in blink::WebContextMenuData? Or should I commit to Webkit first?

I'd suggest adding the logic to Blink first (you'll want to look at something
around 'ContextMenuClientImpl::showContextMenu') rather than reaching down from
the embedder. If nothing else, that will allow you to use safe casting methods
that RELEASE_ASSERT if you screw things up. :)

[msramek, 2015-05-19 12:39:48]

https://codereview.chromium.org/1142923002/diff/20001/content/public/common/c...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/1142923002/diff/20001/content/public/common/c...
content/public/common/context_menu_params.h:170: bool is_password_field;
On 2015/05/19 10:11:22, Mike West (holiday in DE) wrote:
> I would suggest turning this into an enum of possible field types, as I can
> imagine that this feature will end up needing to do strange things for
username
> fields, and it would be a shame to need to tack additional fields onto an
> already-huge struct to do so.

Good idea. The current proposal expects right-click interaction with password
fields only, but that can easily change.

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/1142923002/diff/20001/content/renderer/contex...
content/renderer/context_menu_params_builder.cc:87: // Find out if this node is
a password field.
On 2015/05/19 10:11:22, Mike West (holiday in DE) wrote:
> On 2015/05/19 at 09:42:08, msramek wrote:
> > Is it OK to add a field to ContextMenuParams that is not directly
represented
> in blink::WebContextMenuData? Or should I commit to Webkit first?
> 
> I'd suggest adding the logic to Blink first (you'll want to look at something
> around 'ContextMenuClientImpl::showContextMenu') rather than reaching down
from
> the embedder. If nothing else, that will allow you to use safe casting methods
> that RELEASE_ASSERT if you screw things up. :)

Done in https://codereview.chromium.org/1137733004 .

[msramek, 2015-05-19 12:53:11]

Sylvain, I agree that it makes sense to copy that OWNERS file, but in a separate
CL.

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1150: +          <message
name="IDS_CONTENT_CONTEXT_FORCESAVEPASSWORD" desc="In Title Case: The name of
the Save Password command in the content area context menu">
On 2015/05/19 09:58:13, vabr (Chromium) wrote:
> Should the name be different from the other one?

It should be Title Case :) We have to define it twice for two states of the
"use_title_case" flag. I admit that I have no idea where and why is that flag
used.

https://codereview.chromium.org/1142923002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1151: +            Save password
On 2015/05/19 09:51:15, sdefresne wrote:
> The description says it should be in Title Case, so it should be "Save
> Password".

Oh, that was a really sloppy copy-paste.

https://codereview.chromium.org/1142923002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1142923002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1049: if
(password_manager::ForceSavingExperimentEnabled()) {
On 2015/05/19 09:58:13, vabr (Chromium) wrote:
> nit: To spare indenting, we usually do
> if (!experiment enabled)
>   return
> 
> // The rest of the stuff, at the base level indentation.

I thought that this is semantically better, as AppendPasswordItems() might
contain other items in the future, based on other experiments. But I can change
it for now :)

[Avi (use Gerrit), 2015-05-19 14:48:39]

I'm confused.

According to the bug, you haven't run this by UI. It seems like you want to
implement it, and _then_ do it? I'm not sure what that accomplishes; I don't see
there being a technical question of feasibility that needs answering.

[Avi (use Gerrit), 2015-05-19 14:49:20]

On 2015/05/19 14:48:39, Avi wrote:
> I'm confused.
> 
> According to the bug, you haven't run this by UI. It seems like you want to
> implement it, and _then_ do it? I'm not sure what that accomplishes; I don't
see
> there being a technical question of feasibility that needs answering.

Also, this isn't a "button" in your CL description. It's an item in the context
menu.

[msramek, 2015-05-19 15:10:26]

On 2015/05/19 14:49:20, Avi wrote:
> On 2015/05/19 14:48:39, Avi wrote:
> > I'm confused.
> > 
> > According to the bug, you haven't run this by UI. It seems like you want to
> > implement it, and _then_ do it? I'm not sure what that accomplishes; I don't
> see
> > there being a technical question of feasibility that needs answering.
> 
> Also, this isn't a "button" in your CL description. It's an item in the
context
> menu.

We did have a UI discussion about this, but it was inconclusive so far. We
decided to implement a prototype behind a flag and then iterate with them to see
what works the best.

Fixed the description.

[Avi (use Gerrit), 2015-05-19 15:14:17]

On 2015/05/19 15:10:26, msramek wrote:
> We did have a UI discussion about this, but it was inconclusive so far. We
> decided to implement a prototype behind a flag and then iterate with them to
see
> what works the best.

OK, that makes sense. Thank you for the context.

LGTM

[grt (UTC plus 2), 2015-05-19 17:30:00]

chrome/app/ rubberstamp lgtm

[Ilya Sherman, 2015-05-19 22:53:12]

histograms.xml lgtm

[msramek, 2015-05-21 13:59:37]

Hi Mike, do you have any more comments on this?

[msramek, 2015-05-26 08:41:23]

Mike, friendly ping.

[Mike West, 2015-05-26 09:07:11]

LGTM, sorry for the delay!

[msramek, 2015-05-26 09:13:13]

Thanks :)

[msramek, 2015-05-26 09:13:21]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-05-26 09:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1142923002/#ps60001
(title: "Fixed string, unindented.")

[commit-bot: I haz the power, 2015-05-26 09:13:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142923002/60001

[commit-bot: I haz the power, 2015-05-26 09:17:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-26 09:18:01]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2015-05-26 09:33:44]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-05-26 09:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
mkwst@chromium.org, sdefresne@chromium.org, vabr@chromium.org,
isherman@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1142923002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-05-26 09:34:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142923002/80001

[commit-bot: I haz the power, 2015-05-26 11:27:59]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-05-26 11:28:52]

Patchset 4 (id:??) landed as
https://crrev.com/1cceccd889984e70980ccaddb7a40eb1e7e95b96
Cr-Commit-Position: refs/heads/master@{#331346}