Refactors offline page tab helper to stash the offline page item on redirect.

Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's rendering the omnibox UI.

BUG=607684
Committed: https://crrev.com/44d11cb4bed3575d3c89abae66b61d6f76f221f8
Cr-Commit-Position: refs/heads/master@{#400542}

[dewittj, 2016-06-09 16:41:28]

Patchset #2 (id:20001) has been deleted

[dewittj, 2016-06-09 20:32:01]

Patchset #3 (id:60001) has been deleted

[dewittj, 2016-06-09 20:32:08]

Patchset #2 (id:40001) has been deleted

[dewittj, 2016-06-13 20:57:37]

Description was changed from

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

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

to

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

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

[dewittj, 2016-06-13 20:57:38]

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

[dewittj, 2016-06-13 20:58:03]

Description was changed from

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

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

to

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

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

[dewittj, 2016-06-13 20:58:28]

jianli: ptal

[jianli, 2016-06-14 00:38:44]

Do we really need to introduce another user data that is associated with
WebContents? I think OfflinePageTabHelper is already this kind of user data that
is tied with WebContents. Can we simple store the offline page in
OfflinePageTabHelper?

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

https://codereview.chromium.org/2040163003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:194:
Redirect(from_url, redirect_url, std::move(offline_page));
Do we need std::move?

[dewittj, 2016-06-14 19:43:14]

On 2016/06/14 00:38:44, jianli wrote:
> Do we really need to introduce another user data that is associated with
> WebContents? I think OfflinePageTabHelper is already this kind of user data
that
> is tied with WebContents. Can we simple store the offline page in
> OfflinePageTabHelper?

Done.

[dewittj, 2016-06-14 19:43:26]

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

https://codereview.chromium.org/2040163003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:194:
Redirect(from_url, redirect_url, std::move(offline_page));
On 2016/06/14 00:38:44, jianli wrote:
> Do we need std::move?

Done.

[jianli, 2016-06-15 00:44:00]

lgtm

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:215:
offline_page ? base::MakeUnique<OfflinePageItem>(*offline_page) : nullptr;
I think whether redirecting to online should be checked by
"!to_url.SchemeIsFile()". Passing nullptr to offline_page which indeed was not
null in the caller was very confusing.

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:20: struct
OfflinePageItem;
nit: add an empty line

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:61:
std::unique_ptr<OfflinePageItem> offline_page_;
Please comment this.

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.h:61: static const
OfflinePageItem* GetCurrentOfflinePage(
nit: GetOfflinePageFromWebContents

[dewittj, 2016-06-15 17:21:05]

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:215:
offline_page ? base::MakeUnique<OfflinePageItem>(*offline_page) : nullptr;
On 2016/06/15 00:44:00, jianli wrote:
> I think whether redirecting to online should be checked by
> "!to_url.SchemeIsFile()". Passing nullptr to offline_page which indeed was not
> null in the caller was very confusing.

PTAL, I removed the SchemeIsFile call altogether.

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:20: struct
OfflinePageItem;
On 2016/06/15 00:44:00, jianli wrote:
> nit: add an empty line

Done.

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:61:
std::unique_ptr<OfflinePageItem> offline_page_;
On 2016/06/15 00:44:00, jianli wrote:
> Please comment this.

Done.

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

https://codereview.chromium.org/2040163003/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_utils.h:61: static const
OfflinePageItem* GetCurrentOfflinePage(
On 2016/06/15 00:44:00, jianli wrote:
> nit: GetOfflinePageFromWebContents

Done.

[fgorski, 2016-06-17 18:51:50]

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

[fgorski, 2016-06-17 18:51:50]

Probably use https://bugs.chromium.org/p/chromium/issues/detail?id=607684

[dewittj, 2016-06-17 21:34:05]

Description was changed from

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

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

to

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

BUG=607684
==========

[dewittj, 2016-06-17 21:37:24]

jianli: ptal at most recent patch.  Thanks

[jianli, 2016-06-17 21:48:59]

lgtm

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

https://codereview.chromium.org/2040163003/diff/200001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:62: if
(offline_page_ && navigated_url != offline_page_->GetOfflineURL()) {
nit: remove brackets

[dewittj, 2016-06-17 22:43:21]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2016-06-17 22:43:22]

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/2040163003/#ps220001
(title: "fix nit.")

[commit-bot: I haz the power, 2016-06-17 22:43:50]

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

[commit-bot: I haz the power, 2016-06-17 23:30:41]

Description was changed from

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

BUG=607684
==========

to

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

BUG=607684
==========

[commit-bot: I haz the power, 2016-06-17 23:30:43]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-06-17 23:33:45]

Description was changed from

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

BUG=607684
==========

to

==========
Refactors offline page tab helper to stash the offline page item on redirect.

This allows the Tab to pull the offline page item synchronously when it's
rendering the omnibox UI.

BUG=607684

Committed: https://crrev.com/44d11cb4bed3575d3c89abae66b61d6f76f221f8
Cr-Commit-Position: refs/heads/master@{#400542}
==========

[commit-bot: I haz the power, 2016-06-17 23:33:46]

Patchset 9 (id:??) landed as
https://crrev.com/44d11cb4bed3575d3c89abae66b61d6f76f221f8
Cr-Commit-Position: refs/heads/master@{#400542}