Switch DomAutomationController to be a RenderFrameObserver.

Switch DomAutomationController to be a RenderFrameObserver.

BUG=304341
TBR=nasko@chromium.org

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

[mkosiba (inactive), 2014-02-11 15:37:46]

PTAL.

I was going back and forth between WebContents/RenderFrameHost but neither seem
like a good match. In the end WebContents "wins" since it requires less churn on
the calling side.

Ideally we'd change the ExecuteScriptHelper to work out out browser-side which
frame we're targeting which would make it trivial to filter the notifications
based on Source<RenderFrameHost>.
Should I file a bug for that?

[nasko, 2014-02-11 17:32:33]

Awesome! Just a few minor things and it will be good to go. Pass it to creis@
for OWNERS review once you fix those.

Usually I prefer replacing RenderView with RenderFrame, but in this case it
makes total sense to use WebContents. Until we have a very compelling case to
use RenderFrame, code simplicity and less churn wins.

https://codereview.chromium.org/157713002/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/157713002/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:2194: void
RenderViewHostImpl::OnDomOperationResponse(
Remove the method from the header file as well.

https://codereview.chromium.org/157713002/diff/190001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/157713002/diff/190001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2711: const
DomOperationNotificationDetails& details) {
style: only 4 spaces for indent.

https://codereview.chromium.org/157713002/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/157713002/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:1085: if (!world_id) {
nit: Why not keep the pattern from the RV version of the method? I assume we
will eventually move all the code from the other method to here.

[mkosiba (inactive), 2014-02-14 15:04:41]

nasko@ - Thanks!
+creis@ for OWNERS review.

https://codereview.chromium.org/157713002/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/157713002/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:2194: void
RenderViewHostImpl::OnDomOperationResponse(
On 2014/02/11 17:32:33, nasko wrote:
> Remove the method from the header file as well.

Done.

https://codereview.chromium.org/157713002/diff/190001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/157713002/diff/190001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2711: const
DomOperationNotificationDetails& details) {
On 2014/02/11 17:32:33, nasko wrote:
> style: only 4 spaces for indent.

Done.

[Charlie Reis, 2014-02-15 01:47:32]

Great!  If I understand correctly, there's one semantic change: anyone who
listens for NOTIFICATION_DOM_OPERATION_RESPONSE used to stop listening after a
cross-process navigation, because the RVH changed.  Now they will continue to
get notifications for the lifetime of the WebContents.

I'm willing to guess that that's an improvement, and that the old behavior might
have been buggy in some cases.  It's possible there could be some fallout from
this change if some code expected to stop listening after a swap, so we should
keep an eye out for that.

A few thoughts below.

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
File chrome/browser/browser_keyevents_browsertest.cc (right):

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
chrome/browser/browser_keyevents_browsertest.cc:23: #include
"content/public/browser/render_frame_host.h"
This doesn't look needed, right?

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
chrome/browser/browser_keyevents_browsertest.cc:38: using
content::RenderFrameHost;
Unneeded?

https://codereview.chromium.org/157713002/diff/310001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/157713002/diff/310001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:353: void
RenderFrameHostImpl::OnDomOperationResponse(const std::string& json_string,
Do we need to handle this message here?  Maybe we can just handle it in
WebContentsImpl (and InterstitialPageImpl) if all we're doing is passing it to
the delegate.  Then it doesn't need to be on RenderFrameHostDelegate either.

https://codereview.chromium.org/157713002/diff/310001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/157713002/diff/310001/content/public/test/bro...
content/public/test/browser_test_utils.cc:21: #include
"content/public/browser/render_frame_host.h"
Looks unneeded?

https://codereview.chromium.org/157713002/diff/310001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/157713002/diff/310001/content/renderer/render...
content/renderer/render_frame_impl.cc:1199: if
(render_view_->GetEnabledBindings() & BINDINGS_POLICY_DOM_AUTOMATION) {
nit: No braces.

[mkosiba (inactive), 2014-02-17 17:09:44]

On 2014/02/15 01:47:32, Charlie Reis wrote:
> Great!  If I understand correctly, there's one semantic change: anyone who
> listens for NOTIFICATION_DOM_OPERATION_RESPONSE used to stop listening after a
> cross-process navigation, because the RVH changed.  Now they will continue to
> get notifications for the lifetime of the WebContents.

Thanks! You're correct - there is a semantic change.  Most of the code that's
affected is test code wanting to "execute JS and wait for result" so RVH swaps
should not be a problem. If I understand correctly the InterstitialPage doesn't
do RVH swaps, so that should be OK too, right?

> I'm willing to guess that that's an improvement, and that the old behavior
might
> have been buggy in some cases.  It's possible there could be some fallout from
> this change if some code expected to stop listening after a swap, so we should
> keep an eye out for that.

Agreed.IMO the ideal post-RFH refactor version of this code should resolve
the target RFH browser side which should clear things up.
Do you think it's worth filing a bug to track this?

[mkosiba (inactive), 2014-02-17 17:42:31]

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
File chrome/browser/browser_keyevents_browsertest.cc (right):

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
chrome/browser/browser_keyevents_browsertest.cc:23: #include
"content/public/browser/render_frame_host.h"
On 2014/02/15 01:47:33, Charlie Reis wrote:
> This doesn't look needed, right?

Done.

https://codereview.chromium.org/157713002/diff/310001/chrome/browser/browser_...
chrome/browser/browser_keyevents_browsertest.cc:38: using
content::RenderFrameHost;
On 2014/02/15 01:47:33, Charlie Reis wrote:
> Unneeded?

Done.

https://codereview.chromium.org/157713002/diff/310001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/157713002/diff/310001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:353: void
RenderFrameHostImpl::OnDomOperationResponse(const std::string& json_string,
On 2014/02/15 01:47:33, Charlie Reis wrote:
> Do we need to handle this message here?  Maybe we can just handle it in
> WebContentsImpl (and InterstitialPageImpl) if all we're doing is passing it to
> the delegate.  Then it doesn't need to be on RenderFrameHostDelegate either.

yes, that does indeed make more sense.

https://codereview.chromium.org/157713002/diff/310001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/157713002/diff/310001/content/renderer/render...
content/renderer/render_frame_impl.cc:1199: if
(render_view_->GetEnabledBindings() & BINDINGS_POLICY_DOM_AUTOMATION) {
On 2014/02/15 01:47:33, Charlie Reis wrote:
> nit: No braces.

Done.

[mkosiba (inactive), 2014-02-18 17:34:56]

ping?

[Charlie Reis, 2014-02-18 17:40:27]

On 2014/02/18 17:34:56, mkosiba wrote:
> ping?

Sorry, we had a holiday in the US yesterday.  I'll take a look this morning.

[mkosiba (inactive), 2014-02-18 17:42:59]

On 2014/02/18 17:40:27, Charlie Reis wrote:
> On 2014/02/18 17:34:56, mkosiba wrote:
> > ping?
> 
> Sorry, we had a holiday in the US yesterday.  I'll take a look this morning.

ah, sorry, didn't realize. Feel free to look at it later, I'm going to leave the
office in a couple of minutes anyway.

[Charlie Reis, 2014-02-18 18:06:15]

Thanks, LGTM with nits.  Sorry for the delay!

On 2014/02/17 17:09:44, mkosiba wrote:
> On 2014/02/15 01:47:32, Charlie Reis wrote:
> > Great!  If I understand correctly, there's one semantic change: anyone who
> > listens for NOTIFICATION_DOM_OPERATION_RESPONSE used to stop listening after
a
> > cross-process navigation, because the RVH changed.  Now they will continue
to
> > get notifications for the lifetime of the WebContents.
> 
> Thanks! You're correct - there is a semantic change.  Most of the code that's
> affected is test code wanting to "execute JS and wait for result" so RVH swaps
> should not be a problem. If I understand correctly the InterstitialPage
doesn't
> do RVH swaps, so that should be OK too, right?

Correct.

> > I'm willing to guess that that's an improvement, and that the old behavior
> might
> > have been buggy in some cases.  It's possible there could be some fallout
from
> > this change if some code expected to stop listening after a swap, so we
should
> > keep an eye out for that.
> 
> Agreed.IMO the ideal post-RFH refactor version of this code should resolve
> the target RFH browser side which should clear things up.
> Do you think it's worth filing a bug to track this?

Well, we do deliver the IPC to the correct RFH, and we can always use that info
if the observers eventually need it.  Doesn't seem to be a problem for now, so I
wouldn't worry about the bug.

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:366: bool
InterstitialPageImpl::OnMessageReceived(RenderViewHost* render_view_host,
This isn't used for anything, is it?  Maybe it's better to leave it out
unless/until it's needed, since I'm guessing we'll eventually remove this
class's dependence on RenderViewHostDelegate.

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:20: #include
"content/public/browser/dom_operation_notification_details.h"
nit: Probably not needed anymore.

[Charlie Reis, 2014-02-18 18:06:45]

Ah, and I suppose you'll need a chrome/ owner to approve the last bits.

[mkosiba (inactive), 2014-02-18 19:10:03]

Thanks! I'll upload an updated version tomorrow

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:366: bool
InterstitialPageImpl::OnMessageReceived(RenderViewHost* render_view_host,
On 2014/02/18 18:06:15, Charlie Reis wrote:
> This isn't used for anything, is it?  Maybe it's better to leave it out
> unless/until it's needed, since I'm guessing we'll eventually remove this
> class's dependence on RenderViewHostDelegate.

This is the only sane-looking way I found to work around the
-Woverloaded-virtual error I got when compiling with clang.
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/17...

I'd appreciate a better approach for dealing with this. There doesn't
seem to be precedent for removing the warning in the Chromium tree
either.

[Charlie Reis, 2014-02-18 19:38:55]

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:366: bool
InterstitialPageImpl::OnMessageReceived(RenderViewHost* render_view_host,
On 2014/02/18 19:10:04, mkosiba wrote:
> On 2014/02/18 18:06:15, Charlie Reis wrote:
> > This isn't used for anything, is it?  Maybe it's better to leave it out
> > unless/until it's needed, since I'm guessing we'll eventually remove this
> > class's dependence on RenderViewHostDelegate.
> 
> This is the only sane-looking way I found to work around the
> -Woverloaded-virtual error I got when compiling with clang.
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/17...
> 
> I'd appreciate a better approach for dealing with this. There doesn't
> seem to be precedent for removing the warning in the Chromium tree
> either.

Ah, fun.  I don't have a better idea for that, so LGTM as is.

[mkosiba (inactive), 2014-02-20 14:46:53]

+sky@ for chrome/ OWNERS review

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/157713002/diff/550001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:20: #include
"content/public/browser/dom_operation_notification_details.h"
On 2014/02/18 18:06:15, Charlie Reis wrote:
> nit: Probably not needed anymore.

Done.

[mkosiba (inactive), 2014-02-21 12:03:22]

+sky@ for chromium/ OWNERS for real this time

[sky, 2014-02-21 17:03:53]

chrome LGTM

[mkosiba (inactive), 2014-02-21 18:27:16]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/157713002/730001

[commit-bot: I haz the power, 2014-02-21 18:32:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:32:06]

Try jobs failed on following builders: mac_chromium_rel

[mkosiba (inactive), 2014-02-21 18:42:30]

tbr'ing nasko@ for IPC

[mkosiba (inactive), 2014-02-21 18:42:37]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:43:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/157713002/730001

[commit-bot: I haz the power, 2014-02-21 18:44:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 18:44:03]

Try jobs failed on following builders: mac_chromium_rel

[mkosiba (inactive), 2014-02-21 22:49:35]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/157713002/730001

[commit-bot: I haz the power, 2014-02-22 01:24:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/157713002/730001

[commit-bot: I haz the power, 2014-02-22 07:58:43]

Change committed as 252764