Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(266)

Issue 19679002: <webview>: Implement dialog API (Closed)

Created:
7 years, 5 months ago by Fady Samuel
Modified:
7 years, 5 months ago
Reviewers:
lazyboy, Cris Neckar
CC:
chromium-reviews, joi+watch-content_chromium.org, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

<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

Patch Set 1 #

Patch Set 2 : Tests! Yay@ #

Patch Set 3 : Added more tests and fixed a bug #

Total comments: 14

Patch Set 4 : Addressed comments #

Patch Set 5 : Describes |user_input| #

Total comments: 21

Patch Set 6 : Addressed lazyboy's comments #

Patch Set 7 : Fixed test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+690 lines, -58 lines) Patch
M chrome/browser/extensions/web_view_browsertest.cc View 1 2 1 chunk +42 lines, -0 lines 0 comments Download
M chrome/renderer/resources/extensions/web_view.js View 1 2 3 7 chunks +8 lines, -8 lines 0 comments Download
M chrome/renderer/resources/extensions/web_view_experimental.js View 1 2 3 4 5 2 chunks +73 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.html View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js View 1 2 3 4 5 1 chunk +302 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/platform_apps/web_view/dialog/inject_dialog.js View 1 2 3 4 5 1 chunk +55 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/dialog/manifest.json View 1 1 chunk +2 lines, -2 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/dialog/test.js View 1 0 chunks +-1 lines, --1 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.h View 1 2 3 7 chunks +27 lines, -3 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.cc View 1 2 3 4 5 12 chunks +130 lines, -28 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_constants.h View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_constants.cc View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_message_enums.h View 1 chunk +6 lines, -0 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_messages.h View 1 2 3 4 1 chunk +5 lines, -3 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin.h View 1 2 3 2 chunks +8 lines, -3 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin.cc View 1 2 3 4 5 6 3 chunks +20 lines, -11 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin_bindings.cc View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
Fady Samuel
Hi Istiaque, There's still some unresolved issues for this feature but there's a lot of ...
7 years, 5 months ago (2013-07-19 17:57:59 UTC) #1
lazyboy
Sending some comments, haven't looked at the test yet. https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode230 content/browser/browser_plugin/browser_plugin_guest.cc:230: ...
7 years, 5 months ago (2013-07-19 21:43:16 UTC) #2
Fady Samuel
https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/19679002/diff/4001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode230 content/browser/browser_plugin/browser_plugin_guest.cc:230: if (!guest_) On 2013/07/19 21:43:16, lazyboy wrote: > How ...
7 years, 5 months ago (2013-07-23 15:41:10 UTC) #3
lazyboy
Some more comments, reviewed the test files. https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources/extensions/web_view_experimental.js File chrome/renderer/resources/extensions/web_view_experimental.js (right): https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources/extensions/web_view_experimental.js#newcode100 chrome/renderer/resources/extensions/web_view_experimental.js:100: var validateCall ...
7 years, 5 months ago (2013-07-23 18:45:00 UTC) #4
Fady Samuel
PTAL https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources/extensions/web_view_experimental.js File chrome/renderer/resources/extensions/web_view_experimental.js (right): https://codereview.chromium.org/19679002/diff/16001/chrome/renderer/resources/extensions/web_view_experimental.js#newcode100 chrome/renderer/resources/extensions/web_view_experimental.js:100: var validateCall = function () { On 2013/07/23 ...
7 years, 5 months ago (2013-07-23 19:40:51 UTC) #5
lazyboy
lgtm https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js File chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js (right): https://codereview.chromium.org/19679002/diff/16001/chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js#newcode92 chrome/test/data/extensions/platform_apps/web_view/dialog/embedder.js:92: console.log('A communication channel has been established with webview.'); ...
7 years, 5 months ago (2013-07-23 20:13:23 UTC) #6
Fady Samuel
+cdn@ for IPC change in browser_plugin_messages.h
7 years, 5 months ago (2013-07-23 20:20:51 UTC) #7
Cris Neckar
IPC changes LGTM
7 years, 5 months ago (2013-07-23 21:58:43 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/19679002/26001
7 years, 5 months ago (2013-07-23 22:05:20 UTC) #9
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=62703
7 years, 5 months ago (2013-07-24 00:42:14 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/19679002/54001
7 years, 5 months ago (2013-07-24 02:32:29 UTC) #11
commit-bot: I haz the power
7 years, 5 months ago (2013-07-24 07:30:58 UTC) #12
Message was sent while issue was closed.
Change committed as 213369

Powered by Google App Engine
This is Rietveld 408576698