Fix window.open issue in Document mode on Android.

Fix window.open issue in Document mode on Android.

Defers unblocking renderer's requests until the asynchronous activity
and tab loading process is complete and we are ready to handle
additional requests.

Adds new android-specific API to resume the requests and web contents
delegate API to allow the deferral for resuming renderer request
handling.

BUG=432562
Committed: https://crrev.com/44bdc473c520ad18b4d6dfee19c341f5b9d3a500
Cr-Commit-Position: refs/heads/master@{#327414}

[Maria, 2015-04-18 01:30:33]

mariakhomenko@chromium.org changed reviewers:
+ jam@chromium.org

[Maria, 2015-04-18 01:30:34]

Hi John,

I have a working solution for my issue, though it's not the approach we
discussed. I found that I need the web contents created for the flow to work, so
I just avoid unblocking requests that RenderWidgetHelper::CreateNewWindow set a
block on until we are ready. Please take a look and let me know if this looks ok
to you.

[jam, 2015-04-20 16:05:41]

lgtm

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1717: if
(delegate->ShouldResumeRequestsForCreatedWindow())
nit: instead of calling the line below twice, perhaps move this if statement out
of "if (delegate)" and make it "if (!delegate || delegate->Should...)"?

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1811: WebContentsImpl&
new_contents) {
nit: not sure why this method is needed? i.e. why not just move the body to
ResumeLoadingCreatedWebContents? also the code above (line 1718) would be easier
to read if just contents->ResumeRequests...

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:690: void
ResumeLoadingCreatedWebContents();
nit: add comment and put it in the other android ifdef at line 129

https://codereview.chromium.org/1095913002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1095913002/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:636: // Used by Android only in
DocumentMode to resume loading contents created
nit: remove the redundant "by Android only" since this is in an android ifdef

[Maria, 2015-04-20 23:40:07]

I have a question on the second comment, let me know what you think.

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1717: if
(delegate->ShouldResumeRequestsForCreatedWindow())
On 2015/04/20 16:05:41, jam wrote:
> nit: instead of calling the line below twice, perhaps move this if statement
out
> of "if (delegate)" and make it "if (!delegate || delegate->Should...)"?

Done.

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1811: WebContentsImpl&
new_contents) {
On 2015/04/20 16:05:41, jam wrote:
> nit: not sure why this method is needed? i.e. why not just move the body to
> ResumeLoadingCreatedWebContents? also the code above (line 1718) would be
easier
> to read if just contents->ResumeRequests...

The reason it's needed is that it gets called on all platforms. And the public
method I am exposing in webcontents in ifdef'd to Android.

Let me know if you prefer I get rid of this and make the other method open to
all platforms.

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1095913002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:690: void
ResumeLoadingCreatedWebContents();
On 2015/04/20 16:05:41, jam wrote:
> nit: add comment and put it in the other android ifdef at line 129

Done.

https://codereview.chromium.org/1095913002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1095913002/diff/1/content/public/browser/web_...
content/public/browser/web_contents.h:636: // Used by Android only in
DocumentMode to resume loading contents created
On 2015/04/20 16:05:41, jam wrote:
> nit: remove the redundant "by Android only" since this is in an android ifdef

Done.

[Maria, 2015-04-22 21:11:38]

mariakhomenko@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Maria, 2015-04-22 21:11:39]

Ted, can you take a look for chrome/browser/android OWNERS?

[Ted C, 2015-04-22 21:15:28]

owners lgtm (w/ a could comment nits)

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:218: // Returns whether the page
should resume accepting requests for the newly
drop the [ly] from newly here.

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:219: // window. This is used when
window creation of asynchronous
creation [is] asyn..

[Maria, 2015-04-23 21:02:33]

Made changes based on discussion with jam. PTAL.

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:218: // Returns whether the page
should resume accepting requests for the newly
On 2015/04/22 21:15:27, Ted C wrote:
> drop the [ly] from newly here.

Done.

https://codereview.chromium.org/1095913002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:219: // window. This is used when
window creation of asynchronous
On 2015/04/22 21:15:27, Ted C wrote:
> creation [is] asyn..

Done.

[jam, 2015-04-23 21:39:06]

lgtm

https://codereview.chromium.org/1095913002/diff/120001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1095913002/diff/120001/content/public/browser...
content/public/browser/web_contents.h:637: #if defined(OS_ANDROID)
nit: blank line above

[Maria, 2015-04-23 21:46:22]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2015-04-23 21:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1095913002/#ps140001
(title: "Add whitespace")

[commit-bot: I haz the power, 2015-04-23 21:47:12]

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

[commit-bot: I haz the power, 2015-04-23 21:52:03]

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

[commit-bot: I haz the power, 2015-04-23 21:52:04]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Maria, 2015-04-23 22:56:03]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2015-04-23 22:56:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1095913002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-23 22:56:29]

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

[commit-bot: I haz the power, 2015-04-23 23:14:46]

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

[commit-bot: I haz the power, 2015-04-23 23:14:47]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maria, 2015-04-24 01:21:02]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2015-04-24 01:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1095913002/#ps180001
(title: "Add missing override macro")

[commit-bot: I haz the power, 2015-04-24 01:21:32]

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

[commit-bot: I haz the power, 2015-04-24 03:25:38]

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

[commit-bot: I haz the power, 2015-04-24 03:25:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maria, 2015-04-25 01:40:28]

mariakhomenko@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Maria, 2015-04-25 01:40:29]

+ Fady for a BrowserPluginGuest question below.

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

https://codereview.chromium.org/1095913002/diff/190001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1705: if
(!BrowserPluginGuest::IsGuest(contents) &&
This check should probably be within web contents delegate
ShouldResumeRequestsForCreatedWindow(), but I have trouble finding where browser
plugin guest is defining their webcontents delegate.

[Fady Samuel, 2015-04-28 19:50:23]

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

https://codereview.chromium.org/1095913002/diff/190001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1705: if
(!BrowserPluginGuest::IsGuest(contents) &&
On 2015/04/25 01:40:28, Maria wrote:
> This check should probably be within web contents delegate
> ShouldResumeRequestsForCreatedWindow(), but I have trouble finding where
browser
> plugin guest is defining their webcontents delegate.

Stick it in GuestViewBase::ShouldResumeRequestsForCreatedWindow()

[Maria, 2015-04-28 22:53:42]

Added delegate method to guest_view_base. Did not mark it final because I think
there may be legitimate reasons to override in subclasses. PTAL.

[Fady Samuel, 2015-04-28 22:57:04]

https://codereview.chromium.org/1095913002/diff/210001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1095913002/diff/210001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1788: if
(BrowserPluginGuest::IsGuest(new_contents))
Is this code necessary then?

[Fady Samuel, 2015-04-28 23:10:57]

https://codereview.chromium.org/1095913002/diff/210001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1095913002/diff/210001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1788: if
(BrowserPluginGuest::IsGuest(new_contents))
On 2015/04/28 22:57:04, Fady Samuel wrote:
> Is this code necessary then?

Chatted about it online. If we don't return the new_contents, we will return
NULL in the next check and fail. GuestView lgtm

[Maria, 2015-04-28 23:11:35]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2015-04-28 23:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1095913002/#ps210001
(title: "Add GuestViewBase delegate method")

[commit-bot: I haz the power, 2015-04-28 23:12:17]

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

[commit-bot: I haz the power, 2015-04-29 01:56:03]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-04-29 01:56:48]

Patchset 12 (id:??) landed as
https://crrev.com/44bdc473c520ad18b4d6dfee19c341f5b9d3a500
Cr-Commit-Position: refs/heads/master@{#327414}