Move ContextMenuParams(WebContextMenuInfo) ctor to content/renderer/.

Move ContextMenuParams(WebContextMenuInfo) ctor to content/renderer/.

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

[darin (slow to review), 2013-05-14 00:14:40]

Not sure what you think of this approach using a *Builder subclass.  I've used
this approach in other cases (e.g.,
chromium/src/WebInputEventConversion.{h,cpp}).

[jamesr, 2013-05-14 03:58:24]

On 2013/05/14 00:14:40, darin wrote:
> Not sure what you think of this approach using a *Builder subclass.  I've used
> this approach in other cases (e.g.,
> chromium/src/WebInputEventConversion.{h,cpp}).

Huh!  I don't think I've ever seen this pattern before.  It's clever but I think
I'd prefer content_menu_params_builder.h having this instead:

  static ContentMenuParams FromWebContextMenuData(const
WebKit::WebContextMenuParams& data);

defined as

ContextMenuParams FromWeb...() {
  ContextMenuParams params;
  params.... = ...;
  ...
  return params;
}

NRVO will make it just as efficient and it's not quite as mind-bendy.

[jamesr, 2013-05-14 04:00:03]

lgtm. I'd prefer a function with a return, but up to you.

https://codereview.chromium.org/15049004/diff/3001/content/renderer/context_m...
File content/renderer/context_menu_params_builder.h (right):

https://codereview.chromium.org/15049004/diff/3001/content/renderer/context_m...
content/renderer/context_menu_params_builder.h:14:
ContextMenuParamsBuilder(const WebKit::WebContextMenuData& data);
explicit if you decide to stick with this pattern

[darin (slow to review), 2013-05-14 07:07:46]

On 2013/05/14 04:00:03, jamesr wrote:
> lgtm. I'd prefer a function with a return, but up to you.

Any thoughts on a naming pattern to use here?  I like having this code in a
separate file, and it is nice if the file name and contained function have the
same name.  I'm expecting we'll also need to repeat this pattern, whatever it
is.

"class FooBuilder : public Foo" is just nice because it saves on some typing and
file naming trivially follows ;-)


>
https://codereview.chromium.org/15049004/diff/3001/content/renderer/context_m...
> File content/renderer/context_menu_params_builder.h (right):
> 
>
https://codereview.chromium.org/15049004/diff/3001/content/renderer/context_m...
> content/renderer/context_menu_params_builder.h:14:
> ContextMenuParamsBuilder(const WebKit::WebContextMenuData& data);
> explicit if you decide to stick with this pattern

Thanks for catching that!

[jamesr, 2013-05-14 07:48:27]

Hmmm - can't think of a great answer for either of those.  Maybe the symbol is
called ContentMenuParamsBuilder::FromWebContextMenuData() and lives in
content_menu_params_builder.(h|cc) ? Clear, but awfully chatty.

[commit-bot: I haz the power, 2013-05-15 23:40:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/15049004/24001

[commit-bot: I haz the power, 2013-05-16 10:57:50]

Change committed as 200511