[Offline pages] Filter out pages cached by different tabs on tab helper

[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org

Committed: https://crrev.com/7a82d3168ecc9779d821f17ff69e6697d38ee8da
Cr-Commit-Position: refs/heads/master@{#402563}

[fgorski, 2016-06-27 21:45:33]

WDYT?

[dewittj, 2016-06-27 21:52:50]

drive-by:
seems like the strategy we agreed on, a few nits here and there.

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

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:169: const
OfflinePageItem* offline_page) {
nit: const OfflinePageItem& if there is no case where it's null

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:205: void
OfflinePageTabHelper::GetBestPageAndTryRedirect(
nit: pass in the model here so we only have to get it in one place.

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:228: 
Need to check for a bad android ID?

[Dmitry Titov, 2016-06-27 22:06:04]

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

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:226: if
(tab_android)
How about just early return here:
If (!tab_android)
  return;

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:231: if
(offline_page.client_id.name_space != kLastNNamespace ||
shouldn't it be:
if (namespace == kLastN && id == tab_id)
?

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right):

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:58: void
GetBestPageAndTryRedirect(const GURL& online_url,
These two named almost the same. Consider renaming a bit, to make it clearer
(only say what they actually do):
GetBastPageAndTryRedirect -> StartRedirectToOfflinePage
SelectBestPageAndTryRedirect -> SelectBestPageToRedirect

[fgorski, 2016-06-27 22:40:09]

PTAL.

Working on a test in a separate patch.

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

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:169: const
OfflinePageItem* offline_page) {
On 2016/06/27 21:52:50, dewittj wrote:
> nit: const OfflinePageItem& if there is no case where it's null

Done.

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:205: void
OfflinePageTabHelper::GetBestPageAndTryRedirect(
On 2016/06/27 21:52:50, dewittj wrote:
> nit: pass in the model here so we only have to get it in one place.

We call the method in 2 places.

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:226: if
(tab_android)
On 2016/06/27 22:06:03, Dmitry Titov wrote:
> How about just early return here:
> If (!tab_android)
>   return;

Good point.

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:228: 
On 2016/06/27 21:52:50, dewittj wrote:
> Need to check for a bad android ID?

Dmitry's comment pretty much resolved the problem.

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.cc:231: if
(offline_page.client_id.name_space != kLastNNamespace ||
On 2016/06/27 22:06:03, Dmitry Titov wrote:
> shouldn't it be:
> if (namespace == kLastN && id == tab_id)
> ?

Updated to explicit check for bookmark namespace and last n

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right):

https://codereview.chromium.org/2100043004/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:58: void
GetBestPageAndTryRedirect(const GURL& online_url,
On 2016/06/27 22:06:04, Dmitry Titov wrote:
> These two named almost the same. Consider renaming a bit, to make it clearer
> (only say what they actually do):
> GetBastPageAndTryRedirect -> StartRedirectToOfflinePage
> SelectBestPageAndTryRedirect -> SelectBestPageToRedirect

Done.

[Dmitry Titov, 2016-06-27 22:47:43]

lgtm

[fgorski, 2016-06-28 18:48:30]

Description was changed from

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and
  filter out the ones with different Tab IDs


BUG=622424
R=dimich@chromium.org
==========

to

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter
out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to
handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org
==========

[fgorski, 2016-06-28 18:49:05]

PTAL (fixed tests, added test, refactored GetTabId, fixed policy)

[Dmitry Titov, 2016-06-28 19:43:31]

LGTM, with a comment nit:

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

https://codereview.chromium.org/2100043004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:68: // redirect
to that offline page, and caching it in memory on the helper.
"and caching it" sounds like the whole page was cached somehow. Maybe "caching
metadata of the page locally"?

[fgorski, 2016-06-28 20:26:43]

updated.

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

https://codereview.chromium.org/2100043004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_tab_helper.h:68: // redirect
to that offline page, and caching it in memory on the helper.
On 2016/06/28 19:43:31, Dmitry Titov wrote:
> "and caching it" sounds like the whole page was cached somehow. Maybe "caching
> metadata of the page locally"?

Done.

[fgorski, 2016-06-28 20:27:28]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-06-28 20:27:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2100043004/#ps60001
(title: "Updating comment to address CR")

[commit-bot: I haz the power, 2016-06-28 20:28:54]

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-06-28 22:09:46]

Description was changed from

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter
out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to
handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org
==========

to

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter
out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to
handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org
==========

[commit-bot: I haz the power, 2016-06-28 22:09:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-28 22:12:34]

Description was changed from

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter
out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to
handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org
==========

to

==========
[Offline pages] Filter out pages cached by different tabs on tab helper

* Updating a tab helper to fetch all offline pages with a given URL and filter
out the ones with different Tab IDs
* Adds test for that behavior
* Refactors GetTabId to OfflinePageUtils
* Adds Delegate for getting tab id in OPTabHelper, to fix tests
* Updates Client policy for last_n to allow multiple pages for single URL (to
handle multiple tabs opening the same page)

BUG=622424
R=dimich@chromium.org

Committed: https://crrev.com/7a82d3168ecc9779d821f17ff69e6697d38ee8da
Cr-Commit-Position: refs/heads/master@{#402563}
==========

[commit-bot: I haz the power, 2016-06-28 22:12:35]

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