<webview>: Implement dialog API

<webview>: Implement dialog API

This CL implements a JavaScript Dialog API for <webview>. This API allows the embedder to intercept requests for alert/confirm/prompt dialogs, giving the embedder an opportunity to asynchronously display UI to respond to these requests.

Note: This API blocks the guest until one of three things happens:

1. The default is not prevented, and so the default action is equivalent to canceling the dialog.
2. The default is prevented and the event.dialog object becomes unreachable in JS and is garbage collected.
3. An action is taken: event.dialog.ok(...) or event.dialog.cancel();

BUG=260745
Test=WebViewTest.Dialog_*

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

[Fady Samuel, 2013-07-19 17:57:59]

Hi Istiaque,

There's still some unresolved issues for this feature but there's a lot of value
to expose this on dev channel currently. We're obviously not making any promises
that this API will stick. Please see the internal thread about the <webview>
Dialog API.

Thanks,

Fady

[lazyboy, 2013-07-19 21:43:16]

Sending some comments, haven't looked at the test yet.

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

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:230: if (!guest_)
How could this turn to null?

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:241: BrowserPluginGuest*
guest_;
A raw pointer is probably not ok here.

Also what happens if guest ask for dialog, then before embedder decides, guest
somehow gets killed/terminated?

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1532: callback.Run(true,
string16());
What does this do? Can you add a comment.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1536: WebContents*
web_contents,
nit: can fit in previous line.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1543: WebContents*
web_contents) {
nit: can fit in previous line.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:90: public
JavaScriptDialogManager,
sort?

https://codereview.chromium.org/19679002/diff/4001/content/common/browser_plu...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/19679002/diff/4001/content/common/browser_plu...
content/common/browser_plugin/browser_plugin_messages.h:253: // embedder also
has access.
Describe what |user_input| is for.

[Fady Samuel, 2013-07-23 15:41:10]

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

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:230: if (!guest_)
On 2013/07/19 21:43:16, lazyboy wrote:
> How could this turn to null?

Hmm, with the cleanup I recently added for pointer lock it's actually not
possible. Removed.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:230: if (!guest_)
On 2013/07/19 21:43:16, lazyboy wrote:
> How could this turn to null?

Hmm, it can't removed.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:241: BrowserPluginGuest*
guest_;
On 2013/07/19 21:43:16, lazyboy wrote:
> A raw pointer is probably not ok here.
> 
> Also what happens if guest ask for dialog, then before embedder decides, guest
> somehow gets killed/terminated? 

Removed pointer. This request goes away when BrowserPluginGuest goes away. I
made that change while refactoring pointerlock.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1536: WebContents*
web_contents,
On 2013/07/19 21:43:16, lazyboy wrote:
> nit: can fit in previous line.

The third line cannot.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:1543: WebContents*
web_contents) {
On 2013/07/19 21:43:16, lazyboy wrote:
> nit: can fit in previous line.

Done.

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:90: public
JavaScriptDialogManager,
On 2013/07/19 21:43:16, lazyboy wrote:
> sort?

Done.

https://codereview.chromium.org/19679002/diff/4001/content/common/browser_plu...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/19679002/diff/4001/content/common/browser_plu...
content/common/browser_plugin/browser_plugin_messages.h:253: // embedder also
has access.
On 2013/07/19 21:43:16, lazyboy wrote:
> Describe what |user_input| is for.

Done.

[lazyboy, 2013-07-23 18:45:00]

Some more comments, reviewed the test files.

https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_experimental.js:100: var
validateCall = function () {
nit: no space after function

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js
(right):

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:72:
function setupDialogTest(messageCallback, dialogHandler) {
nit: setUp...

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:92:
console.log('A communication channel has been established with webview.');
It seems all tests below calls
webview.contentWindow.postMessage([some-array],...) in this case, can we move
the common case here and pass [some-array] as param to setupDialogTest() to
remove code repeat below?

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:300:
chrome.test.sendMessage("Launched");
nit: single quotes

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js
(right):

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:8: if
(!embedder) {
Just curious, is this for correctness or do these functions get called before
channel is established (embedder == null).

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:22:
var msg = ['alert-dialog-requested'];
This line getting executed means dialog has been dismissed? So s/requested/done
or dismissed.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:49:
if (data[0] == 'connect') {
a switch (data[0]) might be more readable.

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:244: private:
Indent

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:300: case
JAVASCRIPT_MESSAGE_TYPE_ALERT:
Fix indent.

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:1569: // This is called
if the guest has a beforeunload event handler.
Can you also say what happens when callbark.Run is called?

[Fady Samuel, 2013-07-23 19:40:51]

PTAL

https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_experimental.js:100: var
validateCall = function () {
On 2013/07/23 18:45:01, lazyboy wrote:
> nit: no space after function

Done.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js
(right):

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:72:
function setupDialogTest(messageCallback, dialogHandler) {
On 2013/07/23 18:45:01, lazyboy wrote:
> nit: setUp...

Done.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:92:
console.log('A communication channel has been established with webview.');
On 2013/07/23 18:45:01, lazyboy wrote:
> It seems all tests below calls
> webview.contentWindow.postMessage([some-array],...) in this case, can we move
> the common case here and pass [some-array] as param to setupDialogTest() to
> remove code repeat below?

I like having the console message about the test starting. I'd prefer to leave
it as is.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:300:
chrome.test.sendMessage("Launched");
On 2013/07/23 18:45:01, lazyboy wrote:
> nit: single quotes

Done.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js
(right):

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:8: if
(!embedder) {
On 2013/07/23 18:45:01, lazyboy wrote:
> Just curious, is this for correctness or do these functions get called before
> channel is established (embedder == null).

It's 'correctness'. Since these are unnecessary I've removed them.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:22:
var msg = ['alert-dialog-requested'];
On 2013/07/23 18:45:01, lazyboy wrote:
> This line getting executed means dialog has been dismissed? So
s/requested/done
> or dismissed.

Done.

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js:49:
if (data[0] == 'connect') {
On 2013/07/23 18:45:01, lazyboy wrote:
> a switch (data[0]) might be more readable.

Done.

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:244: private:
On 2013/07/23 18:45:01, lazyboy wrote:
> Indent

Done.

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:300: case
JAVASCRIPT_MESSAGE_TYPE_ALERT:
On 2013/07/23 18:45:01, lazyboy wrote:
> Fix indent.

Done.

https://codereview.chromium.org/19679002/diff/16001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:1569: // This is called
if the guest has a beforeunload event handler.
On 2013/07/23 18:45:01, lazyboy wrote:
> Can you also say what happens when callbark.Run is called?

Done.

[lazyboy, 2013-07-23 20:13:23]

lgtm

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js
(right):

https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:92:
console.log('A communication channel has been established with webview.');
On 2013/07/23 19:40:52, Fady Samuel wrote:
> On 2013/07/23 18:45:01, lazyboy wrote:
> > It seems all tests below calls
> > webview.contentWindow.postMessage([some-array],...) in this case, can we
move
> > the common case here and pass [some-array] as param to setupDialogTest() to
> > remove code repeat below?
> 
> I like having the console message about the test starting. I'd prefer to leave
> it as is.
Sure.

[Fady Samuel, 2013-07-23 20:20:51]

+cdn@ for IPC change in browser_plugin_messages.h

[Cris Neckar, 2013-07-23 21:58:43]

IPC changes LGTM

[commit-bot: I haz the power, 2013-07-23 22:05:20]

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

[commit-bot: I haz the power, 2013-07-24 00:42:14]

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

[commit-bot: I haz the power, 2013-07-24 02:32:29]

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

[commit-bot: I haz the power, 2013-07-24 07:30:58]

Change committed as 213369