Make createWindow (mostly) work with OOPIF

Make createWindow (mostly) work with OOPIF

If a RemoteFrame is targeted by window.open(), we correctly plumb the navigation out of blink.

We no longer assume that ChromeClient::createWindow will return a Page with a local main frame. However, there are a several places where we wouldn't have the plumbing to set the correct state even if a Page with a remote main frame was ever returned.

BUG=463742
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195063

[Nate Chapin, 2015-04-28 23:25:51]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2015-04-28 23:25:52]

https://codereview.chromium.org/1109213002/diff/60001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/1109213002/diff/60001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:261: IntRect
LocalDOMWindow::adjustWindowRect(LocalFrame& frame, const IntRect&
pendingChanges)
This was only ever called immediately before calling Chrome::setWindowRect(), so
add a Widget* parameter to setWindowRect() and merge the two.

https://codereview.chromium.org/1109213002/diff/60001/Source/core/page/Create...
File Source/core/page/CreateWindow.cpp (left):

https://codereview.chromium.org/1109213002/diff/60001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:153: bool hasUserGesture =
UserGestureIndicator::processingUserGesture();
I can't find any reason this is actually necessary.

https://codereview.chromium.org/1109213002/diff/60001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:198:
newFrame->document()->setReferrerPolicy(openerFrame.document()->referrerPolicy());
The test that was added when this line was added still passes with out it, so I
believe it is vestigial.

[dcheng, 2015-04-29 21:37:47]

https://codereview.chromium.org/1109213002/diff/80001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/frame/Frame...
Source/core/frame/Frame.h:68: virtual Widget* widget() const = 0;
It feels weird that we need to expose this. It looks like we just need to get
screenAvailableRect(). At first, I thought this might be because this is
multiscreen aware--but I can only find two HostWindow implementations in Blink:
Chrome and PopupContainerClient.

Given that, I think it might be nicer to have screenAvailableRect() take a
Chrome* argument, and then use that to calculate the available screen rect. That
way, we don't have to expose this at all.

+mlamouri who's touched some of the WebScreenInfo stuff more recently, in case
there's something I'm missing here about multidisplay.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Chrome...
File Source/core/page/Chrome.cpp (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Chrome...
Source/core/page/Chrome.cpp:49: using namespace std;
This is generally discourage in Chrome, and we probably shouldn't do this in
Blink either. I'd prefer to see max and min explicitly qualified with std (or
just using std::max/using std::min like we used to)

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:117: // to the real frame in a different
process.
The new style is TODO(foo). Maybe file a bug? alexmos@ has done a bunch of work
in terms of getting sandbox flags replication to work, and this shouldn't be too
hard to add on top of that.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:161: // the initial empty document's url as
about:blank?
TODO(dcheng): for this one, and point to https://crbug.com/471239. I talked to
nasko@ todya too and we might have some ideas for making more progress on this.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:190: // FIXME: Form submissions on
RemoteFrames don't work yet.
Oh dear...

[Nate Chapin, 2015-05-01 20:30:53]

https://codereview.chromium.org/1109213002/diff/80001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/frame/Frame...
Source/core/frame/Frame.h:68: virtual Widget* widget() const = 0;
On 2015/04/29 21:37:46, dcheng wrote:
> It feels weird that we need to expose this. It looks like we just need to get
> screenAvailableRect(). At first, I thought this might be because this is
> multiscreen aware--but I can only find two HostWindow implementations in
Blink:
> Chrome and PopupContainerClient.
> 
> Given that, I think it might be nicer to have screenAvailableRect() take a
> Chrome* argument, and then use that to calculate the available screen rect.
That
> way, we don't have to expose this at all.
> 
> +mlamouri who's touched some of the WebScreenInfo stuff more recently, in case
> there's something I'm missing here about multidisplay.

Done.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Chrome...
File Source/core/page/Chrome.cpp (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Chrome...
Source/core/page/Chrome.cpp:49: using namespace std;
On 2015/04/29 21:37:46, dcheng wrote:
> This is generally discourage in Chrome, and we probably shouldn't do this in
> Blink either. I'd prefer to see max and min explicitly qualified with std (or
> just using std::max/using std::min like we used to)

Done.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:117: // to the real frame in a different
process.
On 2015/04/29 21:37:47, dcheng wrote:
> The new style is TODO(foo). Maybe file a bug? alexmos@ has done a bunch of
work
> in terms of getting sandbox flags replication to work, and this shouldn't be
too
> hard to add on top of that.

Done.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:161: // the initial empty document's url as
about:blank?
On 2015/04/29 21:37:46, dcheng wrote:
> TODO(dcheng): for this one, and point to https://crbug.com/471239. I talked to
> nasko@ todya too and we might have some ideas for making more progress on
this.

Done.

https://codereview.chromium.org/1109213002/diff/80001/Source/core/page/Create...
Source/core/page/CreateWindow.cpp:190: // FIXME: Form submissions on
RemoteFrames don't work yet.
On 2015/04/29 21:37:46, dcheng wrote:
> Oh dear...

Oh dear? TODO'd.

[dcheng, 2015-05-04 17:38:11]

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Chrom...
File Source/core/page/Chrome.cpp (right):

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Chrom...
Source/core/page/Chrome.cpp:104: 
Nit: remove extra newline.

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
File Source/core/page/CreateWindow.cpp (left):

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
Source/core/page/CreateWindow.cpp:153: bool hasUserGesture =
UserGestureIndicator::processingUserGesture();
I think this might be plumbed through into a few places in Chrome. I looked at
where
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
was called, and traced several locations through. One example of where it
appears to get used is
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/1109213002/diff/120001/Source/web/PopupContai...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/1109213002/diff/120001/Source/web/PopupContai...
Source/web/PopupContainer.cpp:206: IntRect screen = rootWidget->hostWindow() ?
screenAvailableRect(*rootWidget->hostWindow()) : IntRect();
Does m_frameView->root() not work here?

[Nate Chapin, 2015-05-04 18:09:13]

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
File Source/core/page/CreateWindow.cpp (left):

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
Source/core/page/CreateWindow.cpp:153: bool hasUserGesture =
UserGestureIndicator::processingUserGesture();
On 2015/05/04 17:38:11, dcheng wrote:
> I think this might be plumbed through into a few places in Chrome. I looked at
> where
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> was called, and traced several locations through. One example of where it
> appears to get used is
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

FrameLoader::load() will call setHasUserGesture() and should catch all
same-process cases. It's not clear to me what to do about user gestures for
RemoteFrames.

[dcheng, 2015-05-05 21:54:45]

lgtm

(sorry for the delay; since you didn't reply to the other comments, I assumed
you hadn't uploaded a patchset that addressed them yet. I should have
double-checked though)

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
File Source/core/page/CreateWindow.cpp (left):

https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
Source/core/page/CreateWindow.cpp:153: bool hasUserGesture =
UserGestureIndicator::processingUserGesture();
On 2015/05/04 at 18:09:13, Nate Chapin wrote:
> On 2015/05/04 17:38:11, dcheng wrote:
> > I think this might be plumbed through into a few places in Chrome. I looked
at
> > where
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > was called, and traced several locations through. One example of where it
> > appears to get used is
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> FrameLoader::load() will call setHasUserGesture() and should catch all
same-process cases. It's not clear to me what to do about user gestures for
RemoteFrames.


Hmm... I guess as long as it gets set on the ResourceRequest that the embedder
eventually processes, and we correctly persist this on process swaps, we should
be fine.

Do you happen to know if this path is covered by layout tests?

[Nate Chapin, 2015-05-06 18:06:33]

On 2015/05/05 21:54:45, dcheng wrote:
> lgtm
> 
> (sorry for the delay; since you didn't reply to the other comments, I assumed
> you hadn't uploaded a patchset that addressed them yet. I should have
> double-checked though)

Sorry, I marked them Done but didn't actually send the draft comments.

> 
>
https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
> File Source/core/page/CreateWindow.cpp (left):
> 
>
https://codereview.chromium.org/1109213002/diff/120001/Source/core/page/Creat...
> Source/core/page/CreateWindow.cpp:153: bool hasUserGesture =
> UserGestureIndicator::processingUserGesture();
> On 2015/05/04 at 18:09:13, Nate Chapin wrote:
> > On 2015/05/04 17:38:11, dcheng wrote:
> > > I think this might be plumbed through into a few places in Chrome. I
looked
> at
> > > where
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > was called, and traced several locations through. One example of where it
> > > appears to get used is
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > FrameLoader::load() will call setHasUserGesture() and should catch all
> same-process cases. It's not clear to me what to do about user gestures for
> RemoteFrames.
> 
> 
> Hmm... I guess as long as it gets set on the ResourceRequest that the embedder
> eventually processes, and we correctly persist this on process swaps, we
should
> be fine.
> 
> Do you happen to know if this path is covered by layout tests?

I removed the setHasUserGesture() in FrameLoader, and all the layout tests still
passed, so I'm guessing not.

[Nate Chapin, 2015-05-06 18:56:05]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-05-06 18:56:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109213002/140001

[commit-bot: I haz the power, 2015-05-06 19:02:49]

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

[commit-bot: I haz the power, 2015-05-06 19:02:50]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[esprehn, 2015-05-07 17:27:16]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-05-07 17:27:17]

lgtm

https://codereview.chromium.org/1109213002/diff/140001/Source/platform/Platfo...
File Source/platform/PlatformScreen.cpp (left):

https://codereview.chromium.org/1109213002/diff/140001/Source/platform/Platfo...
Source/platform/PlatformScreen.cpp:85: HostWindow* hostWindow =
toHostWindow(widget);
Yay, I dream of the day when Widget is no more.

[Nate Chapin, 2015-05-07 17:30:25]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-05-07 17:30:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109213002/140001

[commit-bot: I haz the power, 2015-05-07 17:33:47]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195063

[tasak, 2015-05-08 08:01:52]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1135633004/ by tasak@google.com.

The reason for reverting is: This breaks the following browser_tests:
ActivityLogApiTest.TriggerEvent
ExtensionApiTest.Events
ExtensionBrowserTest.WindowOpenExtension
WebNavigationApiTest.OpenTab
WebstoreInlineInstallerTest.ShouldBlockInlineInstallFromPopupWindow
.