Track whether a created WebContents has a resume pending.

Track whether a created WebContents has a resume pending.

On Android, it's possible that a WebContents is created and
cannot be loaded immediately, which occurs when Chrome needs
to start an Activity asynchronously.  There's currently no
good way to know when the WebContents is in this state, though.

* Start storing whether the WebContents has needs to resume loading
  in the WebContentsImpl, which can then be checked when the
  WebContentsImpl may begin accepting requests from the renderer.

* Clean up Android code that heuristically tracks whether the
  WebContents could be in this state.

* Existing Java and Chromium tests catch regressions.

BUG=508186
Committed: https://crrev.com/e6b7b46750d4a54a4005882a19d48334e5a9f628
Cr-Commit-Position: refs/heads/master@{#338323}

[gone, 2015-07-08 20:53:41]

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

[gone, 2015-07-08 20:53:42]

@jam: Do these changes to the WebContentsImpl seem reasonable?

[Maria, 2015-07-08 21:02:51]

lgtm

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1821:
contents->SetIsResumePending();
I think it's a bit odd to keep this set if the delegate is null.

I would probably rewrite this as

if(delegate) {
  contents->SetIsResumePending();
  if (delegate->ShouldResume...())
    contents->Resume..()

  delegate->AddNewContents()
}

[gone, 2015-07-08 21:04:45]

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1821:
contents->SetIsResumePending();
On 2015/07/08 21:02:51, Maria wrote:
> I think it's a bit odd to keep this set if the delegate is null.
> 
> I would probably rewrite this as
> 
> if(delegate) {
>   contents->SetIsResumePending();
>   if (delegate->ShouldResume...())
>     contents->Resume..()
> 
>   delegate->AddNewContents()
> }

The current codepath resumes loading of the WebContents if the delegate is NULL,
doesn't it?

[Maria, 2015-07-08 21:20:29]

lgtm

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1821:
contents->SetIsResumePending();
On 2015/07/08 21:04:45, dfalcantara wrote:
> On 2015/07/08 21:02:51, Maria wrote:
> > I think it's a bit odd to keep this set if the delegate is null.
> > 
> > I would probably rewrite this as
> > 
> > if(delegate) {
> >   contents->SetIsResumePending();
> >   if (delegate->ShouldResume...())
> >     contents->Resume..()
> > 
> >   delegate->AddNewContents()
> > }
> 
> The current codepath resumes loading of the WebContents if the delegate is
NULL,
> doesn't it?

Oops, you are right, I misread the code. This is fine as-is.

[jam, 2015-07-09 16:50:28]

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

https://codereview.chromium.org/1214723012/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1821:
contents->SetIsResumePending();
it seems there's no need for a function, and this could be written as simply:
contents->is_resume_pending_ = true;

https://codereview.chromium.org/1214723012/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2672: if (is_resume_pending_)
{
shouldn't this be a CHECK(is_resume_pending_)?

[gone, 2015-07-09 17:11:49]

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:1821:
contents->SetIsResumePending();
On 2015/07/09 16:50:28, jam wrote:
> it seems there's no need for a function, and this could be written as simply:
> contents->is_resume_pending_ = true;

Done.

https://chromiumcodereview.appspot.com/1214723012/diff/20001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:2672: if (is_resume_pending_)
{
On 2015/07/09 16:50:28, jam wrote:
> shouldn't this be a CHECK(is_resume_pending_)?

It could be, but that'd require exposing an IsResumePending() function to the
WebContents (or static casting the WebContents to a WebContentsImpl) so that the
WebContentsAndroid can check it.

[jam, 2015-07-10 16:35:41]

lgtm

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

https://codereview.chromium.org/1214723012/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2672: if (is_resume_pending_)
{
On 2015/07/09 17:11:49, dfalcantara wrote:
> On 2015/07/09 16:50:28, jam wrote:
> > shouldn't this be a CHECK(is_resume_pending_)?
> 
> It could be, but that'd require exposing an IsResumePending() function to the
> WebContents (or static casting the WebContents to a WebContentsImpl) so that
the
> WebContentsAndroid can check it.

oh, the java side always calls through to this even if it wasn't paused? i had
assumed not. if so, then fine to leave as is.

[gone, 2015-07-10 17:12:17]

Thanks!

[gone, 2015-07-10 17:12:20]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-07-10 17:12:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1214723012/#ps40001
(title: "Comments")

[commit-bot: I haz the power, 2015-07-10 17:12:42]

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

[commit-bot: I haz the power, 2015-07-10 17:48:03]

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

[commit-bot: I haz the power, 2015-07-10 17:48:04]

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

(exceeded global retry quota)

[gone, 2015-07-10 17:49:04]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-07-10 17:50:05]

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

[commit-bot: I haz the power, 2015-07-10 18:14:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-10 18:15:26]

Patchset 3 (id:??) landed as
https://crrev.com/e6b7b46750d4a54a4005882a19d48334e5a9f628
Cr-Commit-Position: refs/heads/master@{#338323}