Minor improvements to RecentTabHelper.

Minor improvements to RecentTabHelper.

Adds minor improvements to RecentTabHelper:

- Better organize the state kept during the async call chain of a snapshot
  save: always invalidate pointers when needed and convert if-checks of
  |download_info| to DCHECKs.
- Add more verifications the the navigation type before accepting to
  save a snapshot for it.
- Removed the unneeded IsSamePage method.
- Added isSnapshottingForLastN to make it clearer what the request_id checks
  exist for.
- Move member initializers to the header file.
- Some minor code simplifications.

This is a preparation step to change the snapshot mechanism to use user
interaction triggers instead of navigation events.

BUG=678367
Review-Url: https://codereview.chromium.org/2635023005
Cr-Commit-Position: refs/heads/master@{#444262}
Committed: https://chromium.googlesource.com/chromium/src/+/fd10fe94ba828949dcddd148fb73c3797ab83ef3

[carlosk, 2017-01-17 22:49:12]

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

[commit-bot: I haz the power, 2017-01-17 22:49:55]

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

[carlosk, 2017-01-17 23:09:56]

carlosk@chromium.org changed reviewers:
+ dewittj@chromium.org, dimich@chromium.org

[carlosk, 2017-01-17 23:09:57]

dimich@: PTAL.

dewittj@: FYI.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (left):

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:150: if
(download_info_ && !navigation_handle->IsSamePage()) {
The same page check now happens earlier and the download_info_ check happens
inside ReportDownloadStatusToRequestCoordinator anyway.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:240: !IsSamePage()) {
This IsSamePage check is not needed: if anything happens to the committed URL we
will receive a DidFinishNavigation with the commit where all callback pointers
will be invalidated. Replaced with DCHECK just to play it safe.

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

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:82:
weak_ptr_factory_.InvalidateWeakPtrs();
This change is what justifies the change of if-checks to DCHECKs for the
callbacks further below. I couldn't find any use case where resetting
download_info_ (just below) wouldn't justify invalidating all weak pointers, so
to stop any interrupt any ongoing snapshot save by interrupting its callback
chain.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:136:
navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {
IsExternalProtocol: if a navigation is handled externally the current page
contents should not change so we should not snapshot IMO. It might be the case
that this and HasCommitted are mutually exclusive (I'm not sure) so this is
just-in-case.

IsSamePage: same page navigations shouldn't change the page contents either.
There's even some pages (I think YouTube video pages are one example) that
execute a same page navigation during a regular load through javascript.

[dewittj, 2017-01-17 23:21:53]

lgtm with some nits.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (left):

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:150: if
(download_info_ && !navigation_handle->IsSamePage()) {
On 2017/01/17 23:09:57, carlosk wrote:
> The same page check now happens earlier and the download_info_ check happens
> inside ReportDownloadStatusToRequestCoordinator anyway.

Perhaps a minor rename to reflect the download_info_ check would be helpful?
OTOH, the name is already pretty long :)

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

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:266: if
(!download_info_ || isSnapshottingForLastN())
!download_info_ is redundant here unless you take my advice to move
|isSnapshottingForLastN| into DownloadPageInfo.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.h (right):

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:76:
DownloadPageInfo(const ClientId& client_id,
nit: rename to SnapshotProgressInfo (or something that is not tied to download
exactly)

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:80:
page_snapshot_completed_(false) {}
Maybe another constructor that just accepts a tab_id would make it clear that
there are really two types of DownloadPageInfo?

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:87: // 0 if this is a
"last_1" info.
nit: last_1 is used here and last N is used elsewhere.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:105: bool
isSnapshottingForLastN() const;
This method feels like it should be part of DownloadPageInfo.

[Dmitry Titov, 2017-01-18 00:43:35]

lgtm, agreed with notes from Justin

[carlosk, 2017-01-18 02:50:07]

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

[carlosk, 2017-01-18 02:50:27]

The CQ bit was unchecked by carlosk@chromium.org

[carlosk, 2017-01-18 02:52:19]

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

[commit-bot: I haz the power, 2017-01-18 02:52:44]

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

[carlosk, 2017-01-18 03:06:10]

Thanks.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (left):

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:150: if
(download_info_ && !navigation_handle->IsSamePage()) {
On 2017/01/17 23:21:52, dewittj wrote:
> On 2017/01/17 23:09:57, carlosk wrote:
> > The same page check now happens earlier and the download_info_ check happens
> > inside ReportDownloadStatusToRequestCoordinator anyway.
> 
> Perhaps a minor rename to reflect the download_info_ check would be helpful?
> OTOH, the name is already pretty long :)

Keeping as is as discussed offline.

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

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:136:
navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {
On 2017/01/17 23:09:56, carlosk wrote:
> IsExternalProtocol: if a navigation is handled externally the current page
> contents should not change so we should not snapshot IMO. It might be the case
> that this and HasCommitted are mutually exclusive (I'm not sure) so this is
> just-in-case.
> 
> IsSamePage: same page navigations shouldn't change the page contents either.
> There's even some pages (I think YouTube video pages are one example) that
> execute a same page navigation during a regular load through javascript.

I had to remove the IsExternalProtocol check because it was causing 3 tests to
crash. I'm asking around to see if that's WAI or a bug.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:266: if
(!download_info_ || isSnapshottingForLastN())
On 2017/01/17 23:21:52, dewittj wrote:
> !download_info_ is redundant here unless you take my advice to move
> |isSnapshottingForLastN| into DownloadPageInfo.

It is not redundant: here the value is "or-ed" (||) whereas in
isSnapshottingForLastN it is "and-ed" (&&). But I'm eliminating that other
methods because right now this is the only use of it.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.h (right):

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:76:
DownloadPageInfo(const ClientId& client_id,
On 2017/01/17 23:21:52, dewittj wrote:
> nit: rename to SnapshotProgressInfo (or something that is not tied to download
> exactly)

Done.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:80:
page_snapshot_completed_(false) {}
On 2017/01/17 23:21:52, dewittj wrote:
> Maybe another constructor that just accepts a tab_id would make it clear that
> there are really two types of DownloadPageInfo?

Done.

I also made its members follow the correct naming rule for structs and have
their values default initialized.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:87: // 0 if this is a
"last_1" info.
On 2017/01/17 23:21:52, dewittj wrote:
> nit: last_1 is used here and last N is used elsewhere.

Done.

https://codereview.chromium.org/2635023005/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:105: bool
isSnapshottingForLastN() const;
On 2017/01/17 23:21:52, dewittj wrote:
> This method feels like it should be part of DownloadPageInfo.

Done.

[carlosk, 2017-01-18 03:06:32]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-01-18 03:06:32]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2635023005/#ps20002
(title: "Removed unneeded DCHECK_CURRENTLY_ON.")

[commit-bot: I haz the power, 2017-01-18 03:07:01]

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, 2017-01-18 03:41:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20002, "attempt_start_ts": 1484708792522630,
"parent_rev": "7c80c0275ad52371798fd266c23f52db997a6df0", "commit_rev":
"fd10fe94ba828949dcddd148fb73c3797ab83ef3"}

[commit-bot: I haz the power, 2017-01-18 03:42:21]

Description was changed from

==========
Minor improvements to RecentTabHelper.

Adds minor improvements to RecentTabHelper:

- Better organize the state kept during the async call chain of a snapshot
  save: always invalidate pointers when needed and convert if-checks of
  |download_info| to DCHECKs.
- Add more verifications the the navigation type before accepting to
  save a snapshot for it.
- Removed the unneeded IsSamePage method.
- Added isSnapshottingForLastN to make it clearer what the request_id checks
  exist for.
- Move member initializers to the header file.
- Some minor code simplifications.

This is a preparation step to change the snapshot mechanism to use user
interaction triggers instead of navigation events.

BUG=678367
==========

to

==========
Minor improvements to RecentTabHelper.

Adds minor improvements to RecentTabHelper:

- Better organize the state kept during the async call chain of a snapshot
  save: always invalidate pointers when needed and convert if-checks of
  |download_info| to DCHECKs.
- Add more verifications the the navigation type before accepting to
  save a snapshot for it.
- Removed the unneeded IsSamePage method.
- Added isSnapshottingForLastN to make it clearer what the request_id checks
  exist for.
- Move member initializers to the header file.
- Some minor code simplifications.

This is a preparation step to change the snapshot mechanism to use user
interaction triggers instead of navigation events.

BUG=678367

Review-Url: https://codereview.chromium.org/2635023005
Cr-Commit-Position: refs/heads/master@{#444262}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd10fe94ba828949dcddd148fb73...
==========

[commit-bot: I haz the power, 2017-01-18 03:42:22]

Committed patchset #4 (id:20002) as
https://chromium.googlesource.com/chromium/src/+/fd10fe94ba828949dcddd148fb73...