[Offlining] Updates the tab helper to use the async OfflinePageModel API

[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add histogram checks to all tab helper tests.

BUG=589526
Committed: https://crrev.com/fd1614e5e1ca3ad2b67589eb7682a20c20152948
Cr-Commit-Position: refs/heads/master@{#398077}

[dewittj, 2016-06-03 23:58:30]

Description was changed from

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

BUG=
==========

to

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add
histogram checks to all tab helper tests.

BUG=589526
==========

[dewittj, 2016-06-03 23:58:30]

dewittj@chromium.org changed reviewers:
+ jianli@chromium.org

[jianli, 2016-06-04 00:27:20]

lgtm

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:68: void
ReportAccessedOfflinePage(content::BrowserContext* browser_context,
Please move it to anonymous namespace.

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:73: if
(online_url.is_valid()) {
nit: remove {}

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:128:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork", false);
Can we combine both UMA reporting lines, like:
  bool do_redirect = redirect_url.is_valid() && transition !=
ui::PAGE_TRANSITION_FORWARD_BACK;
  UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork",
do_redirect);
  if (!do_redirect)
    return;

  ...

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:144: if
(!redirect_url.is_valid()) {
nit: remove {}

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right):

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:12: #include
"net/base/net_errors.h"
Why is this needed?

[dewittj, 2016-06-06 16:51:31]

dewittj@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dewittj, 2016-06-06 16:53:15]

dfalcantara: PTAL at tab_android and add_to_homescreen_data_fetcher, thanks

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:68: void
ReportAccessedOfflinePage(content::BrowserContext* browser_context,
On 2016/06/04 00:27:20, jianli wrote:
> Please move it to anonymous namespace.

Done.

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:73: if
(online_url.is_valid()) {
On 2016/06/04 00:27:19, jianli wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:128:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork", false);
On 2016/06/04 00:27:20, jianli wrote:
> Can we combine both UMA reporting lines, like:
>   bool do_redirect = redirect_url.is_valid() && transition !=
> ui::PAGE_TRANSITION_FORWARD_BACK;
>   UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork",
> do_redirect);
>   if (!do_redirect)
>     return;
> 
>   ...

great idea.  Done.

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:144: if
(!redirect_url.is_valid()) {
On 2016/06/04 00:27:20, jianli wrote:
> nit: remove {}

Done.

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right):

https://codereview.chromium.org/2040573002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:12: #include
"net/base/net_errors.h"
On 2016/06/04 00:27:20, jianli wrote:
> Why is this needed?

Done.

[gone, 2016-06-06 18:11:47]

tab_android and add_to_homescreen_data_fetcher lgtm

[dewittj, 2016-06-06 18:25:43]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-06-06 18:25:44]

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

[commit-bot: I haz the power, 2016-06-06 18:26:43]

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

[commit-bot: I haz the power, 2016-06-06 18:36:14]

Description was changed from

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add
histogram checks to all tab helper tests.

BUG=589526
==========

to

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add
histogram checks to all tab helper tests.

BUG=589526
==========

[commit-bot: I haz the power, 2016-06-06 18:36:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-06 18:39:45]

Description was changed from

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add
histogram checks to all tab helper tests.

BUG=589526
==========

to

==========
[Offlining] Updates the tab helper to use the async OfflinePageModel API

Adds a test to make sure sequential navs are handled correctly, and also add
histogram checks to all tab helper tests.

BUG=589526

Committed: https://crrev.com/fd1614e5e1ca3ad2b67589eb7682a20c20152948
Cr-Commit-Position: refs/heads/master@{#398077}
==========

[commit-bot: I haz the power, 2016-06-06 18:39:46]

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