Make <a ping> work for context menus.

Make <a ping> work for context menus.

This adds a link_followed parameter to CustomContextMenuContext. This object is sent back to the renderer process when a context menu is closed, allowing us to provide sufficient information to send pings if appropriate.

BUG=257281
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285011

[Nate Chapin, 2014-07-14 19:01:47]

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

https://codereview.chromium.org/384243004/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1941:
params_.custom_context.link_followed = url;
The meat of the change. We set link_followed in certain cases so that the
HTMLAnchorElement in the renderer process gets a chance to send pings. If
link_followed is left invalid, no pings will be sent.

The rule here is:
* A protocol handler might translate the url, or this function could be called
for an image/video context menu (which set a different parameter than link_url).
So, don't give the renderer a chance to ping if the URL wasn't exactly the
link_url that the renderer process provided.
* If this link is being opened off the record, we suppress referrers, so
suppress pings as well. This seems to match the intent of the user in severing
any connections between the referring page and the destination page.

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
File content/renderer/context_menu_params_builder.h (right):

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.h:20: static blink::WebNode
extractParentAnchorNode(const blink::WebNode& node);
WebContextMenuData provides the deepest WebNode that was clicked for the context
menu. Since that can be a text node for a clicked link, we need a way to walk up
the DOM, looking for a parent that is actually an <a>. This logic is extracted
from Build().

[darin (slow to review), 2014-07-22 17:34:40]

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

https://codereview.chromium.org/384243004/diff/20001/content/public/common/co...
content/public/common/context_menu_params.h:40: GURL link_followed;
Please add some documentation here to explain the purpose of this field.

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

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.cc:83: blink::WebNode selectedNode
= node;
nit: selectedNode -> selected_node

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
File content/renderer/context_menu_params_builder.h (right):

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.h:20: static blink::WebNode
extractParentAnchorNode(const blink::WebNode& node);
nit: extract -> Extract

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.h:20: static blink::WebNode
extractParentAnchorNode(const blink::WebNode& node);
On 2014/07/14 19:01:46, Nate Chapin wrote:
> WebContextMenuData provides the deepest WebNode that was clicked for the
context
> menu. Since that can be a text node for a clicked link, we need a way to walk
up
> the DOM, looking for a parent that is actually an <a>. This logic is extracted
> from Build().

You should probably add some comments on this function. However, I'm not sure
this function really has that much to do with ContextMenuParamsBuilder. Is this
the best place for this function to live?

https://codereview.chromium.org/384243004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/384243004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1017:
ContextMenuParamsBuilder::extractParentAnchorNode(
maybe we should just have a dom_utils.{h,cc} in content/renderer/, and put this
method in there? it seems like it doesn't strictly speaking have anything to do
with context menus.

[Nate Chapin, 2014-07-22 18:19:56]

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

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.cc:83: blink::WebNode selectedNode
= node;
On 2014/07/22 17:34:39, darin wrote:
> nit: selectedNode -> selected_node

Done.

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
File content/renderer/context_menu_params_builder.h (right):

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.h:20: static blink::WebNode
extractParentAnchorNode(const blink::WebNode& node);
On 2014/07/22 17:34:40, darin wrote:
> nit: extract -> Extract

Done.

https://codereview.chromium.org/384243004/diff/20001/content/renderer/context...
content/renderer/context_menu_params_builder.h:20: static blink::WebNode
extractParentAnchorNode(const blink::WebNode& node);
On 2014/07/22 17:34:39, darin wrote:
> On 2014/07/14 19:01:46, Nate Chapin wrote:
> > WebContextMenuData provides the deepest WebNode that was clicked for the
> context
> > menu. Since that can be a text node for a clicked link, we need a way to
walk
> up
> > the DOM, looking for a parent that is actually an <a>. This logic is
extracted
> > from Build().
> 
> You should probably add some comments on this function. However, I'm not sure
> this function really has that much to do with ContextMenuParamsBuilder. Is
this
> the best place for this function to live?

Done.

https://codereview.chromium.org/384243004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/384243004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1017:
ContextMenuParamsBuilder::extractParentAnchorNode(
On 2014/07/22 17:34:40, darin wrote:
> maybe we should just have a dom_utils.{h,cc} in content/renderer/, and put
this
> method in there? it seems like it doesn't strictly speaking have anything to
do
> with context menus.

Agree. dom_utils.{h,cc} seems like a reasonable name, will move there.

[darin (slow to review), 2014-07-22 18:25:52]

LGTM

[jschuh, 2014-07-22 20:09:01]

ipc security lgtm (notes: browser -> renderer url)

[Nate Chapin, 2014-07-23 14:34:17]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-07-23 14:35:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/japhet@chromium.org/384243004/40001

[commit-bot: I haz the power, 2014-07-23 19:21:44]

Change committed as 285011