<webview>: Plumb edit commands

<webview>: Plumb edit commands

This patch plumbs edit commands that do not trigger menu blink on Mac.

This is the corresponding Blink patch: https://codereview.chromium.org/15071004/

BUG=230148
Test=WebViewInteractiveTest.EditCommandsNoMenu

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

[dcheng, 2013-05-21 19:32:37]

I'm assuming there's a corresponding patch in blink?

https://codereview.chromium.org/15149006/diff/8001/chrome/test/data/extension...
File
chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
(right):

https://codereview.chromium.org/15149006/diff/8001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js:78:
var webview = embedder.setUpGuest_();
Nit: indent.

https://codereview.chromium.org/15149006/diff/8001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/15149006/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1331: }
Nit: indent.

https://codereview.chromium.org/15149006/diff/8001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/15149006/diff/8001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2200: GetFocusedNode()))
Is there a corresponding patch to blink? I don't think WebFrame::executeCommand
has an overload for this.

[Fady Samuel, 2013-05-21 19:41:35]

PTAL

https://codereview.chromium.org/15149006/diff/8001/chrome/test/data/extension...
File
chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
(right):

https://codereview.chromium.org/15149006/diff/8001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js:78:
var webview = embedder.setUpGuest_();
On 2013/05/21 19:32:37, dcheng wrote:
> Nit: indent.

Done.

https://codereview.chromium.org/15149006/diff/8001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/15149006/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1331: }
On 2013/05/21 19:32:37, dcheng wrote:
> Nit: indent.

Done.

https://codereview.chromium.org/15149006/diff/8001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/15149006/diff/8001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:2200: GetFocusedNode()))
On 2013/05/21 19:32:37, dcheng wrote:
> Is there a corresponding patch to blink? I don't think
WebFrame::executeCommand
> has an overload for this.

Yes, I just added a link in the description.

[dcheng, 2013-05-22 21:43:43]

LGTM with nits.

https://codereview.chromium.org/15149006/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/15149006/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2200: GetFocusedNode()))
I think it would be worth mentioning somewhere why we need to call
GetFocusedNode() now (perhaps CL description for the Blink change would be a
good place?)

[Fady Samuel, 2013-05-24 13:49:46]

+creis content/renderer/render_view_impl.cc OWNER review.

[Charlie Reis, 2013-05-28 16:49:24]

One sanity check question.

https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
content/common/browser_plugin/browser_plugin_messages.h:159:
std::vector<content::EditCommand> /* edit_commands */)
Sanity check: What damage could a compromised renderer do if it sent corrupted
EditCommands?  I see that we currently send them from the browser to the
renderer in input_messages.h, but the browser is trusted.  We wouldn't want a
bad renderer to take over the browser or another renderer by sending it a bad
EditCommand.

(If there's some validation logic or a reason this is safe, then I'm fine with
it.)

[Fady Samuel, 2013-05-28 21:24:17]

https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
content/common/browser_plugin/browser_plugin_messages.h:159:
std::vector<content::EditCommand> /* edit_commands */)
On 2013/05/28 16:49:25, creis wrote:
> Sanity check: What damage could a compromised renderer do if it sent corrupted
> EditCommands?  I see that we currently send them from the browser to the
> renderer in input_messages.h, but the browser is trusted.  We wouldn't want a
> bad renderer to take over the browser or another renderer by sending it a bad
> EditCommand.
> 
> (If there's some validation logic or a reason this is safe, then I'm fine with
> it.)

Edit commands are derived from key combinations. We already forward (and trust)
mouse events and touch events from the embedder to the guest. Why would edit
commands be any different?

Mind you, for keyboard events we actually grab those off the embedder's queue. I
did that for implementation simplicity and not necessarily for security:
NativeWebKeyboardEvents are required to process system keys if I recall
correctly. Thus, rather than creating a new WebKeyboardEvent for the guest from
the embedder, we grab the embedder's from its key queue.

Maybe we need to rethink the security of input events in general?

[Charlie Reis, 2013-05-28 21:37:25]

On 2013/05/28 21:24:17, Fady Samuel wrote:
>
https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
> File content/common/browser_plugin/browser_plugin_messages.h (right):
> 
>
https://codereview.chromium.org/15149006/diff/12001/content/common/browser_pl...
> content/common/browser_plugin/browser_plugin_messages.h:159:
> std::vector<content::EditCommand> /* edit_commands */)
> On 2013/05/28 16:49:25, creis wrote:
> > Sanity check: What damage could a compromised renderer do if it sent
corrupted
> > EditCommands?  I see that we currently send them from the browser to the
> > renderer in input_messages.h, but the browser is trusted.  We wouldn't want
a
> > bad renderer to take over the browser or another renderer by sending it a
bad
> > EditCommand.
> > 
> > (If there's some validation logic or a reason this is safe, then I'm fine
with
> > it.)
> 
> Edit commands are derived from key combinations. We already forward (and
trust)
> mouse events and touch events from the embedder to the guest. Why would edit
> commands be any different?

This isn't what I meant.  Yes, a renderer could lie about which input events and
edit commands are being sent to its subframes.

I was wondering what damage the EditCommand object could do if it was
deserialized in another renderer (e.g., make us dereference a bad pointer). 
Looks like it's just a struct with two strings, so I'm not concerned.

content/renderer/render_view_impl.cc LGTM.

> Mind you, for keyboard events we actually grab those off the embedder's queue.
I
> did that for implementation simplicity and not necessarily for security:
> NativeWebKeyboardEvents are required to process system keys if I recall
> correctly. Thus, rather than creating a new WebKeyboardEvent for the guest
from
> the embedder, we grab the embedder's from its key queue.
> 
> Maybe we need to rethink the security of input events in general?

[commit-bot: I haz the power, 2013-05-30 20:09:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/12001

[Fady Samuel, 2013-05-30 20:09:59]

+cdn@ for IPC audit

[Cris Neckar, 2013-05-30 21:09:40]

On 2013/05/30 20:09:59, Fady Samuel wrote:
> +cdn@ for IPC audit

LGTM given offline conversation with Fady. The browser is just proxying this to
another renderer and it doesn't seem to present much danger of renderer to
renderer escalation.

[commit-bot: I haz the power, 2013-05-30 21:10:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/12001

[commit-bot: I haz the power, 2013-05-30 21:32:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-30 22:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/12001

[commit-bot: I haz the power, 2013-05-30 22:33:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-31 08:53:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/12001

[commit-bot: I haz the power, 2013-05-31 10:06:13]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-31 14:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/62001

[commit-bot: I haz the power, 2013-05-31 15:45:31]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-31 18:10:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/69003

[commit-bot: I haz the power, 2013-05-31 19:33:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-31 19:48:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/15149006/86002

[commit-bot: I haz the power, 2013-06-01 01:03:47]

Change committed as 203542