Add loadCommit and loadStop Event

Add loadCommit and loadStop Event.

loadCommit notifies that a load has committed with event properties "url" and "isTopLevel".

loadStop notifies that all loads in the tag (including all subframes) have completed, such that any progress indicator can complete, with no event properties.

BUG=154125, 153537
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162877

[Charlie Reis, 2012-10-08 19:03:39]

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

https://codereview.chromium.org/11035067/diff/21002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:383:
RecordAction(UserMetricsAction("BrowserPlugin.Guest.DidCommit"));
Let's stick with DidNavigate here, since we already have data collected under
that name.

https://codereview.chromium.org/11035067/diff/21002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:386: void
BrowserPluginGuest::DidFinishLoad(int64 frame_id,
We should be listening to DidStopLoading rather than DidFinishLoad.

https://codereview.chromium.org/11035067/diff/21002/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11035067/diff/21002/content/common/browser_pl...
content/common/browser_plugin_messages.h:163: // When all the guest pages have
completed, the browser process informs
These comments are backwards.

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:51: const char
kLoadCommitEventName[] = "loadCommit";
Please keep these alphabetized.

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:52: const char
kNavigationEventName[] = "navigation";
We can remove this one now, right?

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:373: v8::Context::Scope
context_scope(v8::Context::New());
Note: We've learned that this approach doesn't work.  I'll try to give you a
better pattern to use after our meeting with Adam this afternoon.

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:9: document.title = "loadStop";
Won't this interfere with other tests?  Every test will have a loadStop event.

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:14: document.title = evt.url;
Same as above.  We'll need some way to avoid affecting the other tests.  One way
is to inject a "shouldUpdateTitleOnCommit" variable, similar to
"shouldEchoTitle" in https://codereview.chromium.org/11027065/.  (It defaults to
false, and I set it to true in the test that cares about it.)

[Charlie Reis, 2012-10-10 18:40:08]

Here's an improved approach to creating the event listeners:
https://codereview.chromium.org/11086025/

[irobert1, 2012-10-10 20:56:56]

Got it.

On Wed, Oct 10, 2012 at 11:40 AM, <creis@chromium.org> wrote:

> Here's an improved approach to creating the event listeners:
>
https://codereview.chromium.**org/11086025/<https://codereview.chromium.org/1...
>
>
>
https://codereview.chromium.**org/11035067/<https://codereview.chromium.org/1...
>

[Fady Samuel, 2012-10-10 22:34:39]

Please mark the bugs as Started.

[irobert, 2012-10-10 23:55:16]

With improved approach to creating the event listeners

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

https://codereview.chromium.org/11035067/diff/21002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:383:
RecordAction(UserMetricsAction("BrowserPlugin.Guest.DidCommit"));
On 2012/10/08 19:03:39, creis wrote:
> Let's stick with DidNavigate here, since we already have data collected under
> that name.

Done.

https://codereview.chromium.org/11035067/diff/21002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:386: void
BrowserPluginGuest::DidFinishLoad(int64 frame_id,
On 2012/10/08 19:03:39, creis wrote:
> We should be listening to DidStopLoading rather than DidFinishLoad.

Done.

https://codereview.chromium.org/11035067/diff/21002/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11035067/diff/21002/content/common/browser_pl...
content/common/browser_plugin_messages.h:163: // When all the guest pages have
completed, the browser process informs
On 2012/10/08 19:03:39, creis wrote:
> These comments are backwards.

Done.

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:51: const char
kLoadCommitEventName[] = "loadCommit";
On 2012/10/08 19:03:39, creis wrote:
> Please keep these alphabetized.

Done.

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:52: const char
kNavigationEventName[] = "navigation";
On 2012/10/08 19:03:39, creis wrote:
> We can remove this one now, right?

Done.

https://codereview.chromium.org/11035067/diff/21002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:373: v8::Context::Scope
context_scope(v8::Context::New());
On 2012/10/08 19:03:39, creis wrote:
> Note: We've learned that this approach doesn't work.  I'll try to give you a
> better pattern to use after our meeting with Adam this afternoon.

Done.

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:9: document.title = "loadStop";
On 2012/10/08 19:03:39, creis wrote:
> Won't this interfere with other tests?  Every test will have a loadStop event.

I don't think it will effect other test based on the result I ran on my local
machine.
Done.

https://codereview.chromium.org/11035067/diff/21002/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:14: document.title = evt.url;
On 2012/10/08 19:03:39, creis wrote:
> Same as above.  We'll need some way to avoid affecting the other tests.  One
way
> is to inject a "shouldUpdateTitleOnCommit" variable, similar to
> "shouldEchoTitle" in https://codereview.chromium.org/11027065/.  (It defaults
to
> false, and I set it to true in the test that cares about it.)

Done.

[Fady Samuel, 2012-10-11 15:25:36]

http://codereview.chromium.org/11035067/diff/15004/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/15004/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:745:
string16(), ASCIIToUTF16("isTopLevel"));
The spacing is wrong here. It should be 4 spaces from the left.

http://codereview.chromium.org/11035067/diff/15004/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

http://codereview.chromium.org/11035067/diff/15004/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:59: void LoadCommit(const GURL&
url, int process_id, bool is_top_level);
FYI, this conflicts with my patch http://codereview.chromium.org/11092023/.
Depending on which one of us lands first, one of us will have to do a little bit
of rebasing. :-)

[irobert, 2012-10-11 16:50:44]

Fix Nits and Comments

http://codereview.chromium.org/11035067/diff/15004/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/15004/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:745:
string16(), ASCIIToUTF16("isTopLevel"));
On 2012/10/11 15:25:36, Fady Samuel wrote:
> The spacing is wrong here. It should be 4 spaces from the left.

Done.

http://codereview.chromium.org/11035067/diff/15004/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

http://codereview.chromium.org/11035067/diff/15004/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:59: void LoadCommit(const GURL&
url, int process_id, bool is_top_level);
Sure. Since we are planning to remove this DidNavigate API, and I have already
replace the code which call this API with the LoadCommit API. I think you can
land first and I can rebase based on your commit. 

I don't know whether it is ok or not. 

On 2012/10/11 15:25:36, Fady Samuel wrote:
> FYI, this conflicts with my patch http://codereview.chromium.org/11092023/.
> Depending on which one of us lands first, one of us will have to do a little
bit
> of rebasing. :-)

[Charlie Reis, 2012-10-11 18:55:28]

Great, I think we're almost there.  And yes, I'll try to get Fady's CL approved
first, and we can resolve the merge conflicts before landing this one.  (Thanks
for the heads up.)

http://codereview.chromium.org/11035067/diff/15006/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/11035067/diff/15006/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:396: // Inform its
embedder of loadStop.
nit: This is clear from the code, so we don't need this comment.

http://codereview.chromium.org/11035067/diff/15006/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/15006/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:735:
expected_title);
This will be flaky.  See my comments in the html file for a suggestion on how to
fix.

http://codereview.chromium.org/11035067/diff/15006/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/11035067/diff/15006/content/common/browser_plu...
content/common/browser_plugin_messages.h:174: bool /* is_main_frame */)
nit: is_top_level

http://codereview.chromium.org/11035067/diff/15006/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (right):

http://codereview.chromium.org/11035067/diff/15006/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:57: void LoadStop();
nit: We probably should re-order these closer to how we expect them to occur:
LoadStart, LoadCommit, LoadStop.  (The rest don't matter.)  Please also update
the order in the .cc file.

http://codereview.chromium.org/11035067/diff/15006/content/test/data/browser_...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/11035067/diff/15006/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:12: var isTopLevel;
nit: commitIsTopLevel.
(There are isTopLevel properties on other events as well.)

http://codereview.chromium.org/11035067/diff/15006/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:14: document.title = evt.url;
We can't distinguish between this and loadStart.  Both set the title to the URL.

That means your test might be flaky: if the title watcher hears loadStart and
looks for isTopLevel before loadCommit happens, then it will fail.  (You may
still see it pass in some cases if the title watcher doesn't get a chance to
react before both loadStart and loadCommit have fired.)

Since title watchers wait for a particular title to show up, it might be
possible to have each event update the title with a unique string.  If you set
it to "loadCommit" instead, we could wait for that reliably.

[irobert, 2012-10-11 20:45:48]

Fix Test and Nits

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

https://codereview.chromium.org/11035067/diff/15006/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:396: // Inform its
embedder of loadStop.
On 2012/10/11 18:55:28, creis wrote:
> nit: This is clear from the code, so we don't need this comment.

Done.

https://codereview.chromium.org/11035067/diff/15006/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/15006/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:735:
expected_title);
On 2012/10/11 18:55:28, creis wrote:
> This will be flaky.  See my comments in the html file for a suggestion on how
to
> fix.

Done.

https://codereview.chromium.org/11035067/diff/15006/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11035067/diff/15006/content/common/browser_pl...
content/common/browser_plugin_messages.h:174: bool /* is_main_frame */)
On 2012/10/11 18:55:28, creis wrote:
> nit: is_top_level

Done.

https://codereview.chromium.org/11035067/diff/15006/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11035067/diff/15006/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:57: void LoadStop();
On 2012/10/11 18:55:28, creis wrote:
> nit: We probably should re-order these closer to how we expect them to occur:
> LoadStart, LoadCommit, LoadStop.  (The rest don't matter.)  Please also update
> the order in the .cc file.

Done.

https://codereview.chromium.org/11035067/diff/15006/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/15006/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:12: var isTopLevel;
On 2012/10/11 18:55:28, creis wrote:
> nit: commitIsTopLevel.
> (There are isTopLevel properties on other events as well.)

Done.

https://codereview.chromium.org/11035067/diff/15006/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:14: document.title = evt.url;
I use "loadCommit:" as the prefix and plus the url as the title changed by
loadCommit event handler.

On 2012/10/11 18:55:28, creis wrote:
> We can't distinguish between this and loadStart.  Both set the title to the
URL.
> 
> That means your test might be flaky: if the title watcher hears loadStart and
> looks for isTopLevel before loadCommit happens, then it will fail.  (You may
> still see it pass in some cases if the title watcher doesn't get a chance to
> react before both loadStart and loadCommit have fired.)
> 
> Since title watchers wait for a particular title to show up, it might be
> possible to have each event update the title with a unique string.  If you set
> it to "loadCommit" instead, we could wait for that reliably.

[Charlie Reis, 2012-10-11 21:56:33]

Thanks.  Fady's CL has landed now (http://crrev.com/161414).  Can you rebase and
fix the merge conflicts?

https://codereview.chromium.org/11035067/diff/29001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/29001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:734: const
string16 expected_title = expected_prefix+ASCIIToUTF16(kHTMLForGuest);
I would suggest StringPrintf on "loadCommit:%s" instead.

[irobert, 2012-10-15 19:51:40]

Fix Comment and Resolve Merge Conflict

https://codereview.chromium.org/11035067/diff/29001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/29001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:734: const
string16 expected_title = expected_prefix+ASCIIToUTF16(kHTMLForGuest);
On 2012/10/11 21:56:33, creis wrote:
> I would suggest StringPrintf on "loadCommit:%s" instead.

Done.

[Fady Samuel, 2012-10-15 19:59:20]

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:803:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
Please use ExecuteSyncJSFunction.

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:822:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
Please use ExecuteSyncJSFunction.

https://codereview.chromium.org/11035067/diff/52001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (left):

https://codereview.chromium.org/11035067/diff/52001/content/common/browser_pl...
content/common/browser_plugin_messages.h:177:
IPC_STRUCT_BEGIN(BrowserPluginMsg_DidNavigate_Params)
We still need these params for CanGoBack/CanGoForward.

Please rename this to BrowserPluginMsg_LoadCommit_Params but keep it.

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:359: const
BrowserPluginMsg_DidNavigate_Params& params) {
We still want these parameters for CanGoBack/CanGoForward. Please keep these.
Just rename them.

Please make sure all BrowserPluginHostTest.* and BrowserPluginTest.* pass.

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:95:
plugin->LoadCommit(url, process_id, is_top_level);
Spacing is wrong here.

[Charlie Reis, 2012-10-15 22:59:56]

Agreed with Fady that we should keep that params object.

http://codereview.chromium.org/11035067/diff/52001/content/test/data/browser_...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/11035067/diff/52001/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:14: document.title =
"loadCommit:"+evt.url;
nit: Put spaces around the +.

[irobert, 2012-10-16 01:26:39]

Fix DidNavigate_Params and Test

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:803:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
On 2012/10/15 19:59:20, Fady Samuel wrote:
> Please use ExecuteSyncJSFunction.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:822:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16(
On 2012/10/15 19:59:20, Fady Samuel wrote:
> Please use ExecuteSyncJSFunction.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (left):

https://codereview.chromium.org/11035067/diff/52001/content/common/browser_pl...
content/common/browser_plugin_messages.h:177:
IPC_STRUCT_BEGIN(BrowserPluginMsg_DidNavigate_Params)
On 2012/10/15 19:59:20, Fady Samuel wrote:
> We still need these params for CanGoBack/CanGoForward.
> 
> Please rename this to BrowserPluginMsg_LoadCommit_Params but keep it.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/common/browser_pl...
content/common/browser_plugin_messages.h:177:
IPC_STRUCT_BEGIN(BrowserPluginMsg_DidNavigate_Params)
On 2012/10/15 19:59:20, Fady Samuel wrote:
> We still need these params for CanGoBack/CanGoForward.
> 
> Please rename this to BrowserPluginMsg_LoadCommit_Params but keep it.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:359: const
BrowserPluginMsg_DidNavigate_Params& params) {
On 2012/10/15 19:59:20, Fady Samuel wrote:
> We still want these parameters for CanGoBack/CanGoForward. Please keep these.
> Just rename them.
> 
> Please make sure all BrowserPluginHostTest.* and BrowserPluginTest.* pass.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/11035067/diff/52001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:95:
plugin->LoadCommit(url, process_id, is_top_level);
On 2012/10/15 19:59:20, Fady Samuel wrote:
> Spacing is wrong here.

Done.

https://codereview.chromium.org/11035067/diff/52001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/52001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:14: document.title =
"loadCommit:"+evt.url;
On 2012/10/15 22:59:56, creis wrote:
> nit: Put spaces around the +.

Done.

[Fady Samuel, 2012-10-16 01:40:00]

Another drive-by review :-)

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:122: RenderViewHost*
render_view_host) OVERRIDE;
Does this need to be on a new line? Will it go over 80 characters if it's all on
one line?

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:827:
base::Value* isTopLevel = rvh->ExecuteJavascriptAndGetValue(
Please make sure to hold this value in a scoped_ptr so it's freed when it goes
out of scope.

http://codereview.chromium.org/11035067/diff/44014/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/11035067/diff/44014/content/common/browser_plu...
content/common/browser_plugin_messages.h:191: // When all the guest pages have
completed, the browser process informs
I don't think this comment is accurate. 

"When the guest page has completed loading, ..." is probably better. Charlie,
what do you think?

[irobert, 2012-10-16 20:10:09]

Fix Nits and Comments

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:122: RenderViewHost*
render_view_host) OVERRIDE;
On 2012/10/16 01:40:00, Fady Samuel wrote:
> Does this need to be on a new line? Will it go over 80 characters if it's all
on
> one line?

Done.

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/44014/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:827:
base::Value* isTopLevel = rvh->ExecuteJavascriptAndGetValue(
On 2012/10/16 01:40:00, Fady Samuel wrote:
> Please make sure to hold this value in a scoped_ptr so it's freed when it goes
> out of scope.

Done.

http://codereview.chromium.org/11035067/diff/44014/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/11035067/diff/44014/content/common/browser_plu...
content/common/browser_plugin_messages.h:191: // When all the guest pages have
completed, the browser process informs
On 2012/10/16 01:40:00, Fady Samuel wrote:
> I don't think this comment is accurate. 
> 
> "When the guest page has completed loading, ..." is probably better. Charlie,
> what do you think?

Done.

[Charlie Reis, 2012-10-16 21:17:55]

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:432:
RecordAction(UserMetricsAction("BrowserPlugin.Guest.DidCommit"));
As before, please leave this as BrowserPlugin.Guest.DidNavigate.  It's used for
histograms, and we have existing data under that name.

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:437: new
BrowserPluginMsg_LoadStop(instance_id()));
This will all fit on one line.

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:870:
scoped_ptr<base::Value> isTopLevel(rvh->ExecuteJavascriptAndGetValue(
nit: is_top_level

http://codereview.chromium.org/11035067/diff/59001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:872: bool t;
bool top_level_bool = false;

http://codereview.chromium.org/11035067/diff/59001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/11035067/diff/59001/content/common/browser_plu...
content/common/browser_plugin_messages.h:191: // When the guest page has
completed loading, the browser process informs
This isn't correct, since these comments are still backwards for LoadCommit vs
LoadStop.  Please update as follows:

"When the guest page has completed loading (including subframes)" refers to
LoadStop.

"When the guest commits a navigation" refers to LoadCommit.

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (left):

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:352: guest_crashed_ = false;
Please keep these three lines at the start of LoadCommit.

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:433: if (frame) {
nit: No braces needed, since both the conditional and body fit on one line.

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.h (left):

http://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.h:74: 
Please leave this line, to avoid code churn.

[irobert, 2012-10-17 20:12:37]

Fix Comments and Merge
 (Please Ignore the Patch 14 since I got a Server error when uploading it.)

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

https://codereview.chromium.org/11035067/diff/59001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:432:
RecordAction(UserMetricsAction("BrowserPlugin.Guest.DidCommit"));
On 2012/10/16 21:17:55, creis wrote:
> As before, please leave this as BrowserPlugin.Guest.DidNavigate.  It's used
for
> histograms, and we have existing data under that name.

Done.

https://codereview.chromium.org/11035067/diff/59001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:437: new
BrowserPluginMsg_LoadStop(instance_id()));
On 2012/10/16 21:17:55, creis wrote:
> This will all fit on one line.

Done.

https://codereview.chromium.org/11035067/diff/59001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/59001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:870:
scoped_ptr<base::Value> isTopLevel(rvh->ExecuteJavascriptAndGetValue(
On 2012/10/16 21:17:55, creis wrote:
> nit: is_top_level

Done.

https://codereview.chromium.org/11035067/diff/59001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:872: bool t;
On 2012/10/16 21:17:55, creis wrote:
> bool top_level_bool = false;

Done.

https://codereview.chromium.org/11035067/diff/59001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11035067/diff/59001/content/common/browser_pl...
content/common/browser_plugin_messages.h:191: // When the guest page has
completed loading, the browser process informs
On 2012/10/16 21:17:55, creis wrote:
> This isn't correct, since these comments are still backwards for LoadCommit vs
> LoadStop.  Please update as follows:
> 
> "When the guest page has completed loading (including subframes)" refers to
> LoadStop.
> 
> "When the guest commits a navigation" refers to LoadCommit.

Done.

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (left):

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:352: guest_crashed_ = false;
On 2012/10/16 21:17:55, creis wrote:
> Please keep these three lines at the start of LoadCommit.

Done.

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:433: if (frame) {
On 2012/10/16 21:17:55, creis wrote:
> nit: No braces needed, since both the conditional and body fit on one line.

Done.

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (left):

https://codereview.chromium.org/11035067/diff/59001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:74: 
On 2012/10/16 21:17:55, creis wrote:
> Please leave this line, to avoid code churn.

Done.

[Charlie Reis, 2012-10-17 21:27:08]

Looks like a few issues from the merge conflict.  Almost there!

https://codereview.chromium.org/11035067/diff/84001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/84001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:876: // This
test verifies that round trip postMessage works as expected.
All of this below is from a merge conflict.  (It was from a CL that landed and
then was reverted.)  Just delete it.

https://codereview.chromium.org/11035067/diff/84001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11035067/diff/84001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:399:
v8::String::New(src_.data(), src_.size()));
nit: No need to change the indent on these two lines.

https://codereview.chromium.org/11035067/diff/84001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/84001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:74:
window.addEventListener('message', receiveMessage, false);
Please remove this line.  (It's another merge conflict.)

[irobert, 2012-10-18 01:42:25]

Fix Merge Conflict

https://codereview.chromium.org/11035067/diff/84001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11035067/diff/84001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:876: // This
test verifies that round trip postMessage works as expected.
On 2012/10/17 21:27:08, creis wrote:
> All of this below is from a merge conflict.  (It was from a CL that landed and
> then was reverted.)  Just delete it.

Done.

https://codereview.chromium.org/11035067/diff/84001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11035067/diff/84001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:399:
v8::String::New(src_.data(), src_.size()));
On 2012/10/17 21:27:08, creis wrote:
> nit: No need to change the indent on these two lines.

Done.

https://codereview.chromium.org/11035067/diff/84001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/11035067/diff/84001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:74:
window.addEventListener('message', receiveMessage, false);
On 2012/10/17 21:27:08, creis wrote:
> Please remove this line.  (It's another merge conflict.)

Done.

[Charlie Reis, 2012-10-18 17:15:36]

LGTM.  Hopefully we can land this without more conflicts, but we'll see what
happens.

[commit-bot: I haz the power, 2012-10-18 17:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/irobert@chromium.org/11035067/70005

[commit-bot: I haz the power, 2012-10-18 17:15:56]

Failed to apply patch for
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/browser_plugin/browser_plugin_manager_impl.cc
  Hunk #1 FAILED at 37.
  Hunk #2 succeeded at 66 (offset 2 lines).
  Hunk #3 succeeded at 94 (offset 9 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/browser_plugin/browser_plugin_manager_impl.cc.rej

Patch:       content/renderer/browser_plugin/browser_plugin_manager_impl.cc
Index: content/renderer/browser_plugin/browser_plugin_manager_impl.cc
diff --git a/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
b/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
index
a88b34f4dbc87537ee9df054b1f69a5366c91eb4..79b9323a23fb717ee36717e48513a2259fccfac7
100644
--- a/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
+++ b/content/renderer/browser_plugin/browser_plugin_manager_impl.cc
@@ -37,13 +37,14 @@ bool BrowserPluginManagerImpl::OnControlMessageReceived(
   IPC_BEGIN_MESSAGE_MAP(BrowserPluginManagerImpl, message)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_UpdateRect, OnUpdateRect)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_GuestCrashed, OnGuestCrashed)
-    IPC_MESSAGE_HANDLER(BrowserPluginMsg_DidNavigate, OnDidNavigate)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_AdvanceFocus, OnAdvanceFocus)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_ShouldAcceptTouchEvents,
                         OnShouldAcceptTouchEvents)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadStart, OnLoadStart)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadAbort, OnLoadAbort)
     IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadRedirect, OnLoadRedirect)
+    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadCommit, OnLoadCommit)
+    IPC_MESSAGE_HANDLER(BrowserPluginMsg_LoadStop, OnLoadStop)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -64,14 +65,6 @@ void BrowserPluginManagerImpl::OnGuestCrashed(int
instance_id) {
     plugin->GuestCrashed();
 }
 
-void BrowserPluginManagerImpl::OnDidNavigate(
-    int instance_id,
-    const BrowserPluginMsg_DidNavigate_Params& params) {
-  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
-  if (plugin)
-    plugin->DidNavigate(params);
-}
-
 void BrowserPluginManagerImpl::OnAdvanceFocus(int instance_id, bool reverse) {
   BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
   if (plugin)
@@ -93,6 +86,20 @@ void BrowserPluginManagerImpl::OnLoadStart(int instance_id,
     plugin->LoadStart(url, is_top_level);
 }
 
+void BrowserPluginManagerImpl::OnLoadCommit(
+    int instance_id,
+    const BrowserPluginMsg_LoadCommit_Params& params) {
+  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
+  if (plugin)
+    plugin->LoadCommit(params);
+}
+
+void BrowserPluginManagerImpl::OnLoadStop(int instance_id) {
+  BrowserPlugin* plugin = GetBrowserPlugin(instance_id);
+  if (plugin)
+    plugin->LoadStop();
+}
+
 void BrowserPluginManagerImpl::OnLoadAbort(int instance_id,
                                            const GURL& url,
                                            bool is_top_level,

[irobert, 2012-10-18 18:13:40]

Upload CL

[irobert, 2012-10-18 18:33:57]

Fix

[commit-bot: I haz the power, 2012-10-18 18:52:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/irobert@chromium.org/11035067/72020

[Fady Samuel, 2012-10-18 19:35:45]

You need to update ShimSrcAttribute to listen for 'loadCommit', instead of
'navigation'.

[Charlie Reis, 2012-10-18 20:18:41]

On 2012/10/18 19:35:45, Fady Samuel wrote:
> You need to update ShimSrcAttribute to listen for 'loadCommit', instead of
> 'navigation'.

Ah, you're right, it's going to fail the CQ due to that.  Robert, can you take a
look?  I think https://chromiumcodereview.appspot.com/11139005/ contains the
files you need to change.

Fady, I notice that the ASAN try bots are failing the postMessage test here:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_asan...

Is it possible that's still flaky?  (We've had a lot of merge conflicts on this
CL due to postMessage, so I just want to be aware if that's going to get
reverted again.)

[irobert, 2012-10-18 20:53:18]

On 2012/10/18 19:35:45, Fady Samuel wrote:
> You need to update ShimSrcAttribute to listen for 'loadCommit', instead of
> 'navigation'.

Done.

[Charlie Reis, 2012-10-18 21:22:15]

Great, LGTM.  Fady, do you see anything else, or should we be safe to land?

[Fady Samuel, 2012-10-18 21:24:33]

LGTM

[commit-bot: I haz the power, 2012-10-18 21:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/irobert@chromium.org/11035067/65007

[commit-bot: I haz the power, 2012-10-19 00:56:14]

Change committed as 162877