Move modal dialogs from WebViewClient to WebFrameClient, part 1/3.

Move modal dialogs from WebViewClient to WebFrameClient, part 1/3.

BUG=304341
TEST=modal dialogs still work

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

[Avi (use Gerrit), 2014-04-04 22:50:18]

John, all but the IPC, Nasko, the IPC.

The only interesting part is AreJavaScriptMessagesSuppressed(), which is a
page-level concept, so we find the boolean of the main frame.

[jam, 2014-04-05 00:21:37]

if you avoid the duplication in the renderer side now, you can also remove the
obsolete methods in the browser

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:697: return
&main_frame->are_javascript_messages_suppressed_;
why keep this in RFH at all instead of in the delegate then?

I ask because i worry that we will have per-page state on RFH if we
differentiate between mainframe and subframes, and it'll start being confusing
about whether we should put state/methods on WebContents or RFH as a result. why
not keep RFH dumb?

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2043: DCHECK(!frame_ || frame_ == frame);
please move renderviewimpl to call renderframeimpl's version until webkit is
rolled and we can remove the code in renderviewimpl. that way, we don't have
duplicate code in the interim.

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:286: virtual void
runModalAlertDialog(blink::WebFrame* frame,
I'm guessing these methods are moving from WebViewClient to WebFrameClient? can
the new ones not have a frame pointer?

[nasko, 2014-04-07 15:45:50]

My general concern is that we duplicate the code path from RenderView into
RenderFrame and now we have two. I would rather see all the RenderView code
removed, the ViewHostMsg IPC removed, and the WebContents having only one
version, without the OBSOLETE methods.

In the moves so far, when code moves from RV to RF and callers need to change,
we just use the top-level RF since it is equivalent to the existing behavior. If
we take the same approach here, it will make it a clean move.

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:172: // Notifies the
RenderView that the JavaScript message that was shown was
nit: RenderFrame

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2086: if (is_swapped_out_)
This should check the swapped out sate on the render_view.

[Avi (use Gerrit), 2014-04-07 15:49:53]

On 2014/04/07 15:45:50, nasko wrote:
> My general concern is that we duplicate the code path from RenderView into
> RenderFrame and now we have two. I would rather see all the RenderView code
> removed, the ViewHostMsg IPC removed, and the WebContents having only one
> version, without the OBSOLETE methods.

I can't do that. This needs to be a three-sided patch. Part 2 is
https://codereview.chromium.org/226413004/. The duplicate code will live for as
long as it takes to roll Blink DEPS.

[Avi (use Gerrit), 2014-04-07 15:51:53]

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2086: if (is_swapped_out_)
On 2014/04/07 15:45:51, nasko wrote:
> This should check the swapped out sate on the render_view.

Huh? I thought that the RenderFrameHost swapped state was already properly wired
up. It's not?

[nasko, 2014-04-07 15:53:12]

On 2014/04/07 15:49:53, Avi wrote:
> On 2014/04/07 15:45:50, nasko wrote:
> > My general concern is that we duplicate the code path from RenderView into
> > RenderFrame and now we have two. I would rather see all the RenderView code
> > removed, the ViewHostMsg IPC removed, and the WebContents having only one
> > version, without the OBSOLETE methods.
> 
> I can't do that. This needs to be a three-sided patch. Part 2 is
> https://codereview.chromium.org/226413004/. The duplicate code will live for
as
> long as it takes to roll Blink DEPS.

Why not? My second paragraph described exactly what we did in previous CLs.
RenderVIew::Method can just call main_render_frame_->Method passing through all
the same parameters.
Once the blink change lands, just the code in RV is removed.

[nasko, 2014-04-07 15:55:15]

On 2014/04/07 15:51:53, Avi wrote:
>
https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:2086: if (is_swapped_out_)
> On 2014/04/07 15:45:51, nasko wrote:
> > This should check the swapped out sate on the render_view.
> 
> Huh? I thought that the RenderFrameHost swapped state was already properly
wired
> up. It's not?

It isn't quite right at this point and instead of fixing it, I will be replacing
it with proxies for subframes. Only the top-level frame will linger around using
swapped out for a bit, but even that will go away hopefully not too far in the
future.

[Avi (use Gerrit), 2014-04-07 16:01:37]

On 2014/04/07 15:53:12, nasko wrote:
> On 2014/04/07 15:49:53, Avi wrote:
> > On 2014/04/07 15:45:50, nasko wrote:
> > > My general concern is that we duplicate the code path from RenderView into
> > > RenderFrame and now we have two. I would rather see all the RenderView
code
> > > removed, the ViewHostMsg IPC removed, and the WebContents having only one
> > > version, without the OBSOLETE methods.
> > 
> > I can't do that. This needs to be a three-sided patch. Part 2 is
> > https://codereview.chromium.org/226413004/. The duplicate code will live for
> as
> > long as it takes to roll Blink DEPS.
> 
> Why not? My second paragraph described exactly what we did in previous CLs.
> RenderVIew::Method can just call main_render_frame_->Method passing through
all
> the same parameters.
> Once the blink change lands, just the code in RV is removed.

Because it hurt my brain too much to figure out how to do so. This is a
synchronous IPC call, so at the end of the dialog, the WebContents has to call
back into either the RVH or the RFH. I took the path of least resistance to say
"hey, this will exist for a dozen revs, so let's leave some duplicated code in
there for those dozen revs that I know is correct rather than try to be tricky
for those dozen revs and risk getting it wrong."


> > Huh? I thought that the RenderFrameHost swapped state was already properly
wired
> > up. It's not?

> It isn't quite right at this point and instead of fixing it, I will be
replacing
> it with proxies for subframes.

Oh. That's not obvious from the code. If is_swapped_out_ is broken, can you
remove it or otherwise annotate that it is not to be relied on?

[Avi (use Gerrit), 2014-04-07 16:12:05]

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:286: virtual void
runModalAlertDialog(blink::WebFrame* frame,
On 2014/04/05 00:21:38, jam wrote:
> I'm guessing these methods are moving from WebViewClient to WebFrameClient?
can
> the new ones not have a frame pointer?

The problem is that RenderViewImpl is both a WebFrameClient as well as a
WebViewClient. If I change the parameters I need to also change the name, or
else I get an error that these hide their overloaded virtual functions.

[Avi (use Gerrit), 2014-04-07 16:19:49]

BTW, WRT calling through...

OOoooooooooooh. I get now the easy fix.

:\ My brain. It doesn't think good.

[Avi (use Gerrit), 2014-04-07 18:03:46]

John, Nasko, ptal.

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:697: return
&main_frame->are_javascript_messages_suppressed_;
On 2014/04/05 00:21:38, jam wrote:
> why keep this in RFH at all instead of in the delegate then?
> 
> I ask because i worry that we will have per-page state on RFH if we
> differentiate between mainframe and subframes, and it'll start being confusing
> about whether we should put state/methods on WebContents or RFH as a result.
why
> not keep RFH dumb?

This was added in https://codereview.chromium.org/9477 to save the user. The
scenario is:

1) Web page puts up a zillion alerts.
2) User suppresses them.
3) User navigates.
4) OnBeforeUnload fires "do you want to leave", but is suppressed.
5) Navigation is cancelled and the user is screwed.

RFH needs to know the suppression state so it can prevent this scenario.

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:172: // Notifies the
RenderView that the JavaScript message that was shown was
On 2014/04/07 15:45:51, nasko wrote:
> nit: RenderFrame

Done.

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2043: DCHECK(!frame_ || frame_ == frame);
On 2014/04/05 00:21:38, jam wrote:
> please move renderviewimpl to call renderframeimpl's version until webkit is
> rolled and we can remove the code in renderviewimpl. that way, we don't have
> duplicate code in the interim.

Done.

https://codereview.chromium.org/226503002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2086: if (is_swapped_out_)
On 2014/04/07 15:45:51, nasko wrote:
> This should check the swapped out sate on the render_view.

Done.

[jam, 2014-04-07 18:11:35]

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:697: return
&main_frame->are_javascript_messages_suppressed_;
On 2014/04/07 18:03:46, Avi wrote:
> On 2014/04/05 00:21:38, jam wrote:
> > why keep this in RFH at all instead of in the delegate then?
> > 
> > I ask because i worry that we will have per-page state on RFH if we
> > differentiate between mainframe and subframes, and it'll start being
confusing
> > about whether we should put state/methods on WebContents or RFH as a result.
> why
> > not keep RFH dumb?
> 
> This was added in https://codereview.chromium.org/9477 to save the user. The
> scenario is:
> 
> 1) Web page puts up a zillion alerts.
> 2) User suppresses them.
> 3) User navigates.
> 4) OnBeforeUnload fires "do you want to leave", but is suppressed.
> 5) Navigation is cancelled and the user is screwed.
> 
> RFH needs to know the suppression state so it can prevent this scenario.

What I meant is why keep state on a main RFH, instead of the delegate (i.e.
WebContents). The question is for per-page state like this, do we want to keep
it on the main RFH, or in a central place? I'm worried about having data members
in RFH that are only used if the object is a main frame, and having RFHs special
case operations to their main RFH. It seems simpler if RFHs are dumb to the fact
that they're a main or sub frame, and instead ask their delegate for per-page
state.

[Avi (use Gerrit), 2014-04-07 19:53:54]

So I plumbed through "the dialog was suppressed" in the reply, so it's available
where it's needed, and dropped the weird bool* param. I did need to side-channel
the value to WebContentsImplTest.NoJSMessageOnInterstitials though, but I
couldn't think of a better way.

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/226503002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:697: return
&main_frame->are_javascript_messages_suppressed_;
Gotcha.

[jam, 2014-04-07 20:50:20]

lgtm

[Avi (use Gerrit), 2014-04-07 23:31:10]

Please take a look at the latest patch, patch 5. Because of the inheritance fun
that I noted on the list, I need to put stub implementations onto
RenderViewImpl. So while I was there, I dropped all the redundant WebFrame
parameters.

PTAL.

[nasko, 2014-04-08 14:05:03]

LGTM with a couple of nits.

https://codereview.chromium.org/226503002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/226503002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:29: 
nit: no empty line needed

https://codereview.chromium.org/226503002/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/226503002/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:460: // -- Stub implementations --
nit: Are the following methods also stubs? If it is supposed to mean "end stubs"
it isn't obvious on first read.

[Avi (use Gerrit), 2014-04-08 15:09:22]

Done

https://codereview.chromium.org/226503002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/226503002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:29: 
On 2014/04/08 14:05:04, nasko wrote:
> nit: no empty line needed

Done.

https://codereview.chromium.org/226503002/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/226503002/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.h:460: // -- Stub implementations --
On 2014/04/08 14:05:04, nasko wrote:
> nit: Are the following methods also stubs? If it is supposed to mean "end
stubs"
> it isn't obvious on first read.

Done.

[Avi (use Gerrit), 2014-04-08 15:34:42]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-04-08 15:34:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/226503002/100001

[commit-bot: I haz the power, 2014-04-08 16:04:43]

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

[commit-bot: I haz the power, 2014-04-08 16:04:44]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Avi (use Gerrit), 2014-04-08 19:30:34]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-04-08 19:32:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/226503002/100001

[commit-bot: I haz the power, 2014-04-08 22:14:51]

Change committed as 262520