Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss the call.
This CL fixes the issue.

BUG=648789
Committed: https://crrev.com/13b0778d13de1e5c0afe784792ad6709b2f5ffad
Cr-Commit-Position: refs/heads/master@{#420200}

[qinmin, 2016-09-20 23:13:55]

Description was changed from

==========
Fix an issue that PrerenderAdapter::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader
will miss the call.
This CL fixes the issue.

BUG=648789
==========

to

==========
Fix an issue that PrerenderAdapter::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader
will miss the call.
This CL fixes the issue.

BUG=648789
==========

[qinmin, 2016-09-20 23:14:29]

Description was changed from

==========
Fix an issue that PrerenderAdapter::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader
will miss the call.
This CL fixes the issue.

BUG=648789
==========

to

==========
Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss
the call.
This CL fixes the issue.

BUG=648789
==========

[qinmin, 2016-09-20 23:15:34]

qinmin@chromium.org changed reviewers:
+ dougarnett@chromium.org, fgorski@chromium.org

[qinmin, 2016-09-20 23:15:34]

PTAL

[dougarnett, 2016-09-21 16:16:51]

Interesting discovery. I wonder if there is any reason that we actually need the
start event though. Perhaps we should just drop it (ie, no PENDING state).

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader.cc:61: state_ =
State::PENDING;
Could just set LOADING here instead and drop the OnPrerenderStart logic and
maybe method.

[dougarnett, 2016-09-21 16:32:41]

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader.cc:61: state_ =
State::PENDING;
On 2016/09/21 16:16:51, dougarnett wrote:
> Could just set LOADING here instead and drop the OnPrerenderStart logic and
> maybe method.

BTW, I could do that clean up as it is a bit more involved.

[qinmin, 2016-09-21 16:40:04]

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2357013002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader.cc:61: state_ =
State::PENDING;
On 2016/09/21 16:32:40, dougarnett wrote:
> On 2016/09/21 16:16:51, dougarnett wrote:
> > Could just set LOADING here instead and drop the OnPrerenderStart logic and
> > maybe method.
> 
> BTW, I could do that clean up as it is a bit more involved. 

Can you take another look at the new patch, I have cleaned up the
OnPrerenderStart() method

[dougarnett, 2016-09-21 17:01:27]

lgtm with comment tweaks. 

Good clean up. Will not help the associated percentage bug though - that will
need feature work more than just bug fix I believe.

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerender_adapter.cc (right):

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerender_adapter.cc:45: // Note:
PrerenderHandle::OnPrerenderStart() will get called inside
maybe drop this comment with something similar by new NOTREACHED call

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerender_adapter.cc:89: NOTREACHED();
maybe some comment here instead, eg,

// Not expected as the start event will happen before the observer is set.

[qinmin, 2016-09-21 17:18:01]

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerender_adapter.cc (right):

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerender_adapter.cc:45: // Note:
PrerenderHandle::OnPrerenderStart() will get called inside
On 2016/09/21 17:01:27, dougarnett wrote:
> maybe drop this comment with something similar by new NOTREACHED call

Done.

https://codereview.chromium.org/2357013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerender_adapter.cc:89: NOTREACHED();
On 2016/09/21 17:01:27, dougarnett wrote:
> maybe some comment here instead, eg,
> 
> // Not expected as the start event will happen before the observer is set.

Done.

[fgorski, 2016-09-21 21:01:52]

lgtm

[qinmin, 2016-09-21 21:09:38]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-21 21:09:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2357013002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-09-21 21:10:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 22:21:52]

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

[commit-bot: I haz the power, 2016-09-21 22:21:53]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[qinmin, 2016-09-21 22:48:37]

Patchset #4 (id:60001) has been deleted

[qinmin, 2016-09-21 22:48:46]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:49:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:26:13]

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

[commit-bot: I haz the power, 2016-09-21 23:26:14]

Dry run: This issue passed the CQ dry run.

[qinmin, 2016-09-21 23:33:26]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-09-21 23:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2357013002/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-09-21 23:34:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:40:29]

Description was changed from

==========
Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss
the call.
This CL fixes the issue.

BUG=648789
==========

to

==========
Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss
the call.
This CL fixes the issue.

BUG=648789
==========

[commit-bot: I haz the power, 2016-09-21 23:40:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-22 00:31:17]

Description was changed from

==========
Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss
the call.
This CL fixes the issue.

BUG=648789
==========

to

==========
Fix an issue that PrerenderLoader::OnPrerenderStart() never gets called.

When calling PrerenderManager::AddPrerenderForOffline(),
OnPrerenderStart() is get called inside it.
Since the observer is not yet added to active_handle_, PrerenderLoader will miss
the call.
This CL fixes the issue.

BUG=648789

Committed: https://crrev.com/13b0778d13de1e5c0afe784792ad6709b2f5ffad
Cr-Commit-Position: refs/heads/master@{#420200}
==========

[commit-bot: I haz the power, 2016-09-22 00:31:18]

Patchset 4 (id:??) landed as
https://crrev.com/13b0778d13de1e5c0afe784792ad6709b2f5ffad
Cr-Commit-Position: refs/heads/master@{#420200}