<webview>: Partially migrate loadcommit event from content to chrome

<webview>: Partially migrate loadcommit event from content to chrome

All the pieces are now in place for upstreaming the big content->chrome
<webview> refactor. This first patch puts in the first set of pieces in
place for migrating loadcommit to chrome.

BUG=166165
Test=WebViewTest.*

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

[Fady Samuel, 2013-06-19 18:13:48]

Hi Istiaque,

Now that all the pieces towards the API migration are upstream, this is the
first step towards migrating API. This patch partially migrates loadcommit to
the chrome layer. It isn't exposed as an internal event to content anymore.

Fady

[Fady Samuel, 2013-06-19 18:14:29]

Sorry, that should've said content->chrome not chrome->content.

[lazyboy, 2013-06-19 19:22:16]

Looks good from browser plugin/web_view.js perspective, some comments. Feel free
to add someone to review the listener bit in extensions after addressing them,
thanks.

https://codereview.chromium.org/17165004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://codereview.chromium.org/17165004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api_constants.h:5: // Consts used
for the WebView API.
nit: Constants

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:48: var eventOpts = {
supportsListeners: true, supportsFilters: true };
nit: here and below: no space after { and before }

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:267:
loadCommitEvent.addListener(function(event) {
How is it ensured that loadcommit event doesn't reach here (and therefore be
ignored) before attach event?

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:268: var evt = new
Event('loadcommit', { bubbles: true });
e event evt are kind of hard to read here.
s/evt/webviewEvent

https://codereview.chromium.org/17165004/diff/4001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (left):

https://codereview.chromium.org/17165004/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:1131:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, LoadCommit) {
Equivalent test in chrome/?

[Fady Samuel, 2013-06-19 19:42:11]

+mpcomplete@: Could you please look at web_view.js. In particular the way I
create an eventBinding.Event. It seems that we can only have one instance of
that event per event name. That makes sense, but I'm wondering if I'm doing
anything silly here? Thanks.

https://codereview.chromium.org/17165004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://codereview.chromium.org/17165004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api_constants.h:5: // Consts used
for the WebView API.
On 2013/06/19 19:22:16, lazyboy wrote:
> nit: Constants

Done.

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:48: var eventOpts = {
supportsListeners: true, supportsFilters: true };
On 2013/06/19 19:22:16, lazyboy wrote:
> nit: here and below: no space after { and before }

Done.

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:267:
loadCommitEvent.addListener(function(event) {
On 2013/06/19 19:22:16, lazyboy wrote:
> How is it ensured that loadcommit event doesn't reach here (and therefore be
> ignored) before attach event?

No content is loaded prior to attachment. We fire the -internal-attach prior to
sending the IPC to the browser process to attach. The IPC to add an event
listener gets sent prior to the IPC to attach as well.

https://codereview.chromium.org/17165004/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:268: var evt = new
Event('loadcommit', { bubbles: true });
On 2013/06/19 19:22:16, lazyboy wrote:
> e event evt are kind of hard to read here.
> s/evt/webviewEvent

Done.

https://codereview.chromium.org/17165004/diff/4001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (left):

https://codereview.chromium.org/17165004/diff/4001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:1131:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, LoadCommit) {
On 2013/06/19 19:22:16, lazyboy wrote:
> Equivalent test in chrome/?

It already exists. WebViewTest.Shim already tests loadcommit pretty extensively.

[Matt Perry, 2013-06-19 22:47:50]

Right, you can only have 1 chrome.Event with a given name.

https://codereview.chromium.org/17165004/diff/1011/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api_constants.cc (right):

https://codereview.chromium.org/17165004/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api_constants.cc:10: const char
kEventLoadCommit[] = "loadcommit";
nit: capitalize the event name for consistency.

Also, please make this const the whole event string, "webview.onLoadCommit".

[Fady Samuel, 2013-06-20 00:26:55]

PTAL. Istiaque, are you happy with this now?

https://codereview.chromium.org/17165004/diff/1011/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api_constants.cc (right):

https://codereview.chromium.org/17165004/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api_constants.cc:10: const char
kEventLoadCommit[] = "loadcommit";
On 2013/06/19 22:47:50, Matt Perry wrote:
> nit: capitalize the event name for consistency.
> 
> Also, please make this const the whole event string, "webview.onLoadCommit".

Done.

[Matt Perry, 2013-06-20 00:31:34]

LGTM for */extensions

https://codereview.chromium.org/17165004/diff/2002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17165004/diff/2002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:52: var loadCommitEvent =
createEvent('LoadCommit');
I would put the full name in here as well... easier to grep for.

[Fady Samuel, 2013-06-20 00:55:14]

https://codereview.chromium.org/17165004/diff/2002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17165004/diff/2002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:52: var loadCommitEvent =
createEvent('LoadCommit');
On 2013/06/20 00:31:34, Matt Perry wrote:
> I would put the full name in here as well... easier to grep for.

Done.

[lazyboy, 2013-06-20 02:37:05]

lgtm

lgtm with couple comments.

https://chromiumcodereview.appspot.com/17165004/diff/16001/chrome/browser/ext...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://chromiumcodereview.appspot.com/17165004/diff/16001/chrome/browser/ext...
chrome/browser/extensions/api/webview/webview_api_constants.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
nit: here and in other files, remove the (c) part. Although at this point I'm
not sure we are being consistent about this anymore.

https://chromiumcodereview.appspot.com/17165004/diff/16001/chrome/browser/web...
File chrome/browser/webview/webview_guest.cc (right):

https://chromiumcodereview.appspot.com/17165004/diff/16001/chrome/browser/web...
chrome/browser/webview/webview_guest.cc:8: #include
"base/strings/stringprintf.h"
Not required anymore?

[Fady Samuel, 2013-06-20 03:58:28]

PTAL Istiaque. Thanks.

https://codereview.chromium.org/17165004/diff/16001/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://codereview.chromium.org/17165004/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/webview_api_constants.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/06/20 02:37:05, lazyboy wrote:
> nit: here and in other files, remove the (c) part. Although at this point I'm
> not sure we are being consistent about this anymore.

Done.

https://codereview.chromium.org/17165004/diff/16001/chrome/browser/webview/we...
File chrome/browser/webview/webview_guest.cc (right):

https://codereview.chromium.org/17165004/diff/16001/chrome/browser/webview/we...
chrome/browser/webview/webview_guest.cc:8: #include
"base/strings/stringprintf.h"
On 2013/06/20 02:37:05, lazyboy wrote:
> Not required anymore?

Done.

[lazyboy, 2013-06-20 04:01:20]

lgtm

[commit-bot: I haz the power, 2013-06-20 04:04:38]

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

[commit-bot: I haz the power, 2013-06-20 04:35:13]

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-06-20 04:49:12]

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

[commit-bot: I haz the power, 2013-06-20 05:05:36]

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.

[Fady Samuel, 2013-06-20 05:07:26]

Doh, it looks like this CL will break <adview>. Adding rpaquay@ as reviewer.
We'll need to sync up tomorrow to make this do the right thing.

[Fady Samuel, 2013-06-21 14:19:03]

+kalman@ to continue a discussion we had yesterday.

It turns out that ad_view.json was required to make the FeatureProvider work.
What's going on? :-(

On the positive side, <adview> works again after this refactor! :D

[not at google - send to devlin, 2013-06-21 16:09:53]

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/common/exte...
File chrome/common/extensions/api/adview.json (right):

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/common/exte...
chrome/common/extensions/api/adview.json:8: "nodoc": true
not needed, since if you don't add an html file it won't have any docs.

also you mgiht want to upload with that git cl flag which says not to detect
copies.

[not at google - send to devlin, 2013-06-21 16:10:24]

> +kalman@ to continue a discussion we had yesterday.
> 

Yep. I'm happy to land the empty adview.json, with a reference to the bug. I'll
take a look at it in a couple of weeks when I'm back from vacation.

[lazyboy, 2013-06-21 17:56:23]

Some more comments for newly added files. Still lgtm.

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/adv...
File chrome/browser/adview/adview_guest.h (right):

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:15: // A AdViewGuest is a
WebContentsObserver on the guest WebContents of a
nit: An maybe

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:18: // created on attachment. That is, when
a guest WebContents is associated with
s/That is, when a a guest ../When a guest WC is associated with a particular
embedder WC, we call this "attachment".

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:22: class AdViewGuest : public
content::WebContentsObserver {
There might be opportunity to refactor/reuse code betwn AdViewGuest adn
WebViewGuest? Add a TODO if so.

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:70: scoped_ptr<base::DictionaryValue>
args_;
Remove this one too.

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/17165004/diff/48002/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:728: new
WebViewGuest(guest_web_contents,
else if "webview" ... else DCHECK

[commit-bot: I haz the power, 2013-06-25 14:44:12]

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

[commit-bot: I haz the power, 2013-06-25 14:56:00]

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

[Fady Samuel, 2013-06-25 14:59:01]

+sky@ for chrome OWNER review. It seems I need someone to sign off on the
content of the new directory chrome/browser/adview.

[sky, 2013-06-25 16:16:51]

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
File chrome/browser/adview/adview_constants.h (right):

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_constants.h:10: namespace adview_constants {
namespace is wrong here. Should be adview. See
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...
.

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
File chrome/browser/adview/adview_guest.cc (right):

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_guest.cc:26: static
base::LazyInstance<AdViewGuestMap> adview_guest_map =
don't need static here.

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_guest.cc:44: std::string api_name;
You may get a warning with this code on a release build.

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_guest.cc:53: int guest_instance_id) {
nit: indenting is off.

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
File chrome/browser/adview/adview_guest.h (right):

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:13: namespace chrome {
No need for a namespace here. But if you do want one, use adview like the other
file.

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/adv...
chrome/browser/adview/adview_guest.h:40: int instance_id() const { return
adview_instance_id_; }
style guide says accessor should method field name, so this should be
adview_instance_id().

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/17165004/diff/61001/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:723: new
AdViewGuest(guest_web_contents,
Document AdViewGuest/WebViewGuest delete themselves when done.

[Fady Samuel, 2013-06-25 16:54:40]

PTAL

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
File chrome/browser/adview/adview_constants.h (right):

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_constants.h:10: namespace adview_constants {
On 2013/06/25 16:16:52, sky wrote:
> namespace is wrong here. Should be adview. See
>
https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/name...
> .

Done.

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
File chrome/browser/adview/adview_guest.cc (right):

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_guest.cc:26: static
base::LazyInstance<AdViewGuestMap> adview_guest_map =
On 2013/06/25 16:16:52, sky wrote:
> don't need static here.

Done.

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_guest.cc:44: std::string api_name;
On 2013/06/25 16:16:52, sky wrote:
> You may get a warning with this code on a release build.

This check is redundant anyway. Removing.

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_guest.cc:53: int guest_instance_id) {
On 2013/06/25 16:16:52, sky wrote:
> nit: indenting is off.

Done.

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
File chrome/browser/adview/adview_guest.h (right):

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_guest.h:13: namespace chrome {
On 2013/06/25 16:16:52, sky wrote:
> No need for a namespace here. But if you do want one, use adview like the
other
> file.

Done.

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/adview/adv...
chrome/browser/adview/adview_guest.h:40: int instance_id() const { return
adview_instance_id_; }
On 2013/06/25 16:16:52, sky wrote:
> style guide says accessor should method field name, so this should be
> adview_instance_id().

Renamed field and accessor to view_instance_id().

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/17165004/diff/61001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:723: new
AdViewGuest(guest_web_contents,
On 2013/06/25 16:16:52, sky wrote:
> Document AdViewGuest/WebViewGuest delete themselves when done.

Done.

[sky, 2013-06-25 17:51:40]

LGTM

[commit-bot: I haz the power, 2013-06-25 17:53:09]

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

[commit-bot: I haz the power, 2013-06-25 17:53:19]

Failed to apply patch for chrome/browser/chrome_content_browser_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chrome_content_browser_client.cc
  Hunk #3 FAILED at 715.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chrome_content_browser_client.cc.rej

Patch:       chrome/browser/chrome_content_browser_client.cc
Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc
b/chrome/browser/chrome_content_browser_client.cc
index
ea6bb61cf0b51e8a01be9e11c8462e773cdfc5c0..2ded298bd206d40f576410f32133fd08a91902db
100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -17,6 +17,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/app/breakpad_mac.h"
+#include "chrome/browser/adview/adview_guest.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/browser_about_handler.h"
 #include "chrome/browser/browser_process.h"
@@ -84,6 +85,7 @@
 #include "chrome/browser/user_style_sheet_watcher.h"
 #include "chrome/browser/user_style_sheet_watcher_factory.h"
 #include "chrome/browser/validation_message_message_filter.h"
+#include "chrome/browser/webview/webview_constants.h"
 #include "chrome/browser/webview/webview_guest.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_constants.h"
@@ -713,11 +715,28 @@ void ChromeContentBrowserClient::GuestWebContentsAttached(
   std::vector<ExtensionMsg_Loaded_Params> extensions;
   extensions.push_back(ExtensionMsg_Loaded_Params(extension));
   guest_web_contents->Send(new ExtensionMsg_Loaded(extensions));
-  new WebViewGuest(guest_web_contents,
-                   embedder_web_contents,
-                   extension->id(),
-                   browser_plugin_instance_id,
-                   extra_params);
+
+  std::string api_type;
+  extra_params.GetString(webview::kAttributeApi, &api_type);
+
+  // WebViewGuest and AdViewGuest's lifetimes iare tied to their associated
+  // guest WebContents' lifetime. When the guest WebContents is destroyed, so
is
+  // the attached WebViewGuest or AdViewGuest.
+  if (api_type == "adview") {
+    new AdViewGuest(guest_web_contents,
+                    embedder_web_contents,
+                    extension->id(),
+                    browser_plugin_instance_id,
+                    extra_params);
+  } else if (api_type == "webview") {
+    new WebViewGuest(guest_web_contents,
+                     embedder_web_contents,
+                     extension->id(),
+                     browser_plugin_instance_id,
+                     extra_params);
+  } else {
+    NOTREACHED();
+  }
 }
 
 void ChromeContentBrowserClient::RenderProcessHostCreated(

[commit-bot: I haz the power, 2013-06-25 18:15:16]

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

[commit-bot: I haz the power, 2013-06-25 19:06:52]

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

[commit-bot: I haz the power, 2013-06-25 19:08:29]

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

[commit-bot: I haz the power, 2013-06-25 20:42:58]

Change committed as 208542