Redirect immediately to offline copy on no network

Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940, 605951
Committed: https://crrev.com/732834fe7d24a687d481f09e2c88cb22219ceb6b
Cr-Commit-Position: refs/heads/master@{#392170}

[jianli, 2016-04-18 23:07:14]

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

[fgorski, 2016-05-04 21:45:12]

please add the bug id: 605951

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

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:103: GURL
offline_url = offline_pages::OfflinePageUtils::GetOfflineURLForOnlineURL(
no change here?

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:119:
OfflinePageUtils::MarkPageAccessed(
should we mark page access when it is loaded without being redirected?

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

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:50: const GURL&
offline_url);
nit: align

[jianli, 2016-05-05 00:23:13]

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

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:103: GURL
offline_url = offline_pages::OfflinePageUtils::GetOfflineURLForOnlineURL(
On 2016/05/04 21:45:12, fgorski wrote:
> no change here?

The plan is that we still access the sync API for this on M52.

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:119:
OfflinePageUtils::MarkPageAccessed(
On 2016/05/04 21:45:12, fgorski wrote:
> should we mark page access when it is loaded without being redirected?

Done.

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

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:50: const GURL&
offline_url);
On 2016/05/04 21:45:12, fgorski wrote:
> nit: align

Done.

[fgorski, 2016-05-05 04:19:30]

Please add relevant comment.

Also I noticed that you are not updating tests (and we do lack coverage in this
area). Would it be possible to add tests here? We still have about 2 weeks to
branch.

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

https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:103: GURL
offline_url = offline_pages::OfflinePageUtils::GetOfflineURLForOnlineURL(
On 2016/05/05 00:23:13, jianli wrote:
> On 2016/05/04 21:45:12, fgorski wrote:
> > no change here?
> 
> The plan is that we still access the sync API for this on M52.

OK. What I think I didn't get was, the fact that in DidStartNavigation we
redirect because of connection state, while in DidFinishNavigation, it is
because we hit the error page.

Perhaps we could have a high level comment somewhere for that?

[jianli, 2016-05-05 23:51:44]

Description was changed from

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940
==========

to

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940,605951
==========

[jianli, 2016-05-05 23:55:46]

Patchset #4 (id:60001) has been deleted

[jianli, 2016-05-05 23:56:54]

On 2016/05/05 04:19:30, fgorski wrote:
> Please add relevant comment.
> 
> Also I noticed that you are not updating tests (and we do lack coverage in
this
> area). Would it be possible to add tests here? We still have about 2 weeks to
> branch.
> 
>
https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
> File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1902443003/diff/20001/chrome/browser/android/...
> chrome/browser/android/offline_pages/offline_page_tab_helper.cc:103: GURL
> offline_url = offline_pages::OfflinePageUtils::GetOfflineURLForOnlineURL(
> On 2016/05/05 00:23:13, jianli wrote:
> > On 2016/05/04 21:45:12, fgorski wrote:
> > > no change here?
> > 
> > The plan is that we still access the sync API for this on M52.
> 
> OK. What I think I didn't get was, the fact that in DidStartNavigation we
> redirect because of connection state, while in DidFinishNavigation, it is
> because we hit the error page.
> 
> Perhaps we could have a high level comment somewhere for that?

Added comment and updated tests.

[fgorski, 2016-05-06 07:07:17]

lgtm mod rebase

[jianli, 2016-05-06 21:29:41]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-05-06 21:29:41]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1902443003/#ps120001
(title: "Rebase and update tests")

[commit-bot: I haz the power, 2016-05-06 21:29:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902443003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902443003/120001

[commit-bot: I haz the power, 2016-05-06 22:06:58]

Description was changed from

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940,605951
==========

to

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940,605951
==========

[commit-bot: I haz the power, 2016-05-06 22:07:00]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-06 22:08:55]

Description was changed from

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940,605951
==========

to

==========
Redirect immediately to offline copy on no network

Also mark the offline page as accessed when this occurs.

BUG=603940,605951

Committed: https://crrev.com/732834fe7d24a687d481f09e2c88cb22219ceb6b
Cr-Commit-Position: refs/heads/master@{#392170}
==========

[commit-bot: I haz the power, 2016-05-06 22:08:56]

Patchset 6 (id:??) landed as
https://crrev.com/732834fe7d24a687d481f09e2c88cb22219ceb6b
Cr-Commit-Position: refs/heads/master@{#392170}