[Offline pages] Refactor URL conversions from TabAndroid

[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing

BUG=565576
Committed: https://crrev.com/1d4c9c97f09b2e490a2f758c4a31b7c9f8cb6686
Cr-Commit-Position: refs/heads/master@{#365874}

[fgorski, 2015-12-15 00:56:18]

Description was changed from

==========
[Offline pages] Refactor URL conversions from TabAndroid

WORK IN PROGRESS

BUG=565576
==========

to

==========
[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing 

BUG=565576
==========

[fgorski, 2015-12-15 00:56:18]

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

[fgorski, 2015-12-15 00:56:29]

PTAL.

[dewittj, 2015-12-15 00:59:25]

Publishing a few comments from ps 2

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

https://codereview.chromium.org/1521193002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:21: // Returns true if
|url| might points to an offline page.
nit: might point to

https://codereview.chromium.org/1521193002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:26: GURL
GetOfflineURLByOnlineURL(content::BrowserContext* browser_context,
Nit: If not too much work, s/By/From/ would make this slightly more readable. 
Same with the others in this namespace.

[jianli, 2015-12-15 01:45:25]

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_model_factory.cc:18: #include
"content/public/browser/browser_context.h"
Do we need this include?

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:22: // Returns an
offline page related to the |online_url\, if one exists in the
\ => |
also, suggest removing if part since it causes confusion and adds no value.
 // Returns an offline page that is originated from |online_url|.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:41: // Returns an
offline page related to the |offline_url\, if one exists in the
\ => |
also, suggest removing if part since it causes confusion and adds no value.
 // Returns an offline page that is stored as |offline_url|.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:61: // TODO: bail out
if model is null
nit: either add DCHECK or do the bailout now.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:80: return
GURL::EmptyGURL();
We should return GURL() since here we don't return GURL reference.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:92: return
GURL::EmptyGURL();
ditto

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:28:
namespace {
nit: empty line

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:68: class
OfflinePageUtilsTest : public OfflinePageUtilsBasicTest {
This class should be grouped with all its tests.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:142:
PreloadStore();
We're really not interested in testing loading logic. So the simplest thing to
do is to just create the empty store and call AddOrUpdateOfflinePage a couple
times.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:170:
DisableOfflinePagesFeature();
Adding command line switch should normally be done in SetUp stage, instead of
after that.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:177:
EXPECT_FALSE(HasOfflinePages(profile()));
What's the difference between HasOfflinePagesDisabled and HasOfflinePagesEmpty?

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/test_offline_page_model_builder.h:22:
content::BrowserContext* context);
nit: empty line

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:781: if (offline_page_bookmark_id != -1)
What if offline_page_bookmark_id cannot be found in bookmark model when the
offline meta store fails to removes its record for various reasons?

https://codereview.chromium.org/1521193002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_test_store.cc (right):

https://codereview.chromium.org/1521193002/diff/40001/components/offline_page...
components/offline_pages/offline_page_test_store.cc:87: void
OfflinePageTestStore::SetStoreState(
I am not sure we need this. Why not just calling AddOrUpdateOfflinePage to add a
bunch items? You can update AddOrUpdateOfflinePage to skip calling PostTask if
provided callback is empty.

[fgorski, 2015-12-15 16:33:02]

Addressed and open for further discussion.

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

https://codereview.chromium.org/1521193002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:21: // Returns true if
|url| might points to an offline page.
On 2015/12/15 00:59:25, dewittj wrote:
> nit: might point to

Done.

https://codereview.chromium.org/1521193002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:26: GURL
GetOfflineURLByOnlineURL(content::BrowserContext* browser_context,
On 2015/12/15 00:59:25, dewittj wrote:
> Nit: If not too much work, s/By/From/ would make this slightly more readable. 
> Same with the others in this namespace.

This adheres to the naming convention used by the offline page model.
If you feel strongly about such change we can do it in a separate patch. I don't
have a strong opinion either way here.

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_model_factory.cc:18: #include
"content/public/browser/browser_context.h"
On 2015/12/15 01:45:24, jianli wrote:
> Do we need this include?

I am not sure what you mean. It is included by
chrome/browser/profiles/profile.h, but I don't think being explicit hurts us.
After all we are using the type in this file.

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:22: // Returns an
offline page related to the |online_url\, if one exists in the
On 2015/12/15 01:45:24, jianli wrote:
> \ => |
> also, suggest removing if part since it causes confusion and adds no value.
>  // Returns an offline page that is originated from |online_url|.

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:41: // Returns an
offline page related to the |offline_url\, if one exists in the
On 2015/12/15 01:45:24, jianli wrote:
> \ => |
> also, suggest removing if part since it causes confusion and adds no value.
>  // Returns an offline page that is stored as |offline_url|.

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:61: // TODO: bail out
if model is null
On 2015/12/15 01:45:24, jianli wrote:
> nit: either add DCHECK or do the bailout now.

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:80: return
GURL::EmptyGURL();
On 2015/12/15 01:45:24, jianli wrote:
> We should return GURL() since here we don't return GURL reference.

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:92: return
GURL::EmptyGURL();
On 2015/12/15 01:45:24, jianli wrote:
> ditto

Done.

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:28:
namespace {
On 2015/12/15 01:45:24, jianli wrote:
> nit: empty line

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:68: class
OfflinePageUtilsTest : public OfflinePageUtilsBasicTest {
On 2015/12/15 01:45:24, jianli wrote:
> This class should be grouped with all its tests. 

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:142:
PreloadStore();
On 2015/12/15 01:45:24, jianli wrote:
> We're really not interested in testing loading logic. So the simplest thing to
> do is to just create the empty store and call AddOrUpdateOfflinePage a couple
> times.

Moved the offline page creation related code to a separate method and removed
setstorestate.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:170:
DisableOfflinePagesFeature();
On 2015/12/15 01:45:24, jianli wrote:
> Adding command line switch should normally be done in SetUp stage, instead of
> after that.

Notice that I explicitly extracted the methods and a whole class to make
exception to that rule.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:177:
EXPECT_FALSE(HasOfflinePages(profile()));
On 2015/12/15 01:45:24, jianli wrote:
> What's the difference between HasOfflinePagesDisabled and
HasOfflinePagesEmpty?

Store is empty in the latter case and the feature is disabled in the former.

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/test_offline_page_model_builder.h:22:
content::BrowserContext* context);
On 2015/12/15 01:45:24, jianli wrote:
> nit: empty line

Done.

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:781: if (offline_page_bookmark_id != -1)
On 2015/12/15 01:45:24, jianli wrote:
> What if offline_page_bookmark_id cannot be found in bookmark model when the
> offline meta store fails to removes its record for various reasons?

Updated.
I have a separate patch(es) where I'll be addressing that problem more broadly.

https://codereview.chromium.org/1521193002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_test_store.cc (right):

https://codereview.chromium.org/1521193002/diff/40001/components/offline_page...
components/offline_pages/offline_page_test_store.cc:87: void
OfflinePageTestStore::SetStoreState(
On 2015/12/15 01:45:25, jianli wrote:
> I am not sure we need this. Why not just calling AddOrUpdateOfflinePage to add
a
> bunch items? You can update AddOrUpdateOfflinePage to skip calling PostTask if
> provided callback is empty.

Done.

[dewittj, 2015-12-15 18:30:31]

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_model_factory.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_model_factory.cc:18: #include
"content/public/browser/browser_context.h"
I think since you aren't dereferencing a pointer you can just forward declare
the class, rather than including the header file.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
Could these all just be static functions in OfflinePageModel instead of a new
utils file?

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

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:19: namespace android
{
why namespace android?

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:39: // const
base::FilePath::CharType kTestPage3FilePath[] =
nit: remove commented out code

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:144: store_
= static_cast<OfflinePageTestStore*>(model()->GetStoreForTesting());
Any way to get this without doing a static_cast?

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:187:
TEST_F(OfflinePageUtilsTest, MightBeOfflineURL) {
Could this be in the basic tests?

https://codereview.chromium.org/1521193002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:546: DVLOG(1) << "Store is loaded
by now";
nit: remove the log statements from this file.

[jianli, 2015-12-15 23:53:34]

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

https://codereview.chromium.org/1521193002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:177:
EXPECT_FALSE(HasOfflinePages(profile()));
On 2015/12/15 16:33:02, fgorski wrote:
> On 2015/12/15 01:45:24, jianli wrote:
> > What's the difference between HasOfflinePagesDisabled and
> HasOfflinePagesEmpty?
> 
> Store is empty in the latter case and the feature is disabled in the former.

Both tests only check if the store is empty. Thus we couldn't tell if enabling
or disabling the feature make any difference.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/12/15 18:30:31, dewittj wrote:
> Could these all just be static functions in OfflinePageModel instead of a new
> utils file?

Since we have quite a few static methods, it would be better to aggregate them
in a utility class that contains only static methods, IMO.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:58: 
nit: remove empty line

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:87:
DCHECK(browser_context);
nit: DCHECK is not needed since it is already in GetOfflinePageByOfflineURL.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:99:
DCHECK(browser_context);
ditto

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:111:
DCHECK(browser_context);
ditto

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:101:
TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesWhenFeatureDisabled) {
HasOfflinePagesWhenFeatureDisabled is confusing. There is no offline page when
feature is disabled.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:107:
TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesWhenStoreIsEmpty) {
ditto for test method name

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:118:
OfflinePageTestStore* store() { return store_; }
nit: add const modifier

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:119:
OfflinePageModel* model() { return model_; }
ditto

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:121: const
base::FilePath& test_file_path_1() { return test_file_path_1_; }
ditto for this and two below

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:776: // If the url points to an offline
page, replace it with the original url.
Update comment.

[fgorski, 2015-12-16 21:43:16]

PTAL

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_model_factory.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_model_factory.cc:18: #include
"content/public/browser/browser_context.h"
On 2015/12/15 18:30:31, dewittj wrote:
> I think since you aren't dereferencing a pointer you can just forward declare
> the class, rather than including the header file.

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/12/15 23:53:34, jianli wrote:
> On 2015/12/15 18:30:31, dewittj wrote:
> > Could these all just be static functions in OfflinePageModel instead of a
new
> > utils file?
> 
> Since we have quite a few static methods, it would be better to aggregate them
> in a utility class that contains only static methods, IMO.

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/12/15 18:30:31, dewittj wrote:
> Could these all just be static functions in OfflinePageModel instead of a new
> utils file?

Overall, yes we could have methods with similar functionality on the model.
What we are getting from this functions are 2 things:
* Extra check that the feature is enabled
* ability to query offline page model without having to write boilerplate
  code to get it. Browser context is enough in this case.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:58: 
On 2015/12/15 23:53:34, jianli wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:87:
DCHECK(browser_context);
On 2015/12/15 23:53:34, jianli wrote:
> nit: DCHECK is not needed since it is already in GetOfflinePageByOfflineURL.

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:99:
DCHECK(browser_context);
On 2015/12/15 23:53:34, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:111:
DCHECK(browser_context);
On 2015/12/15 23:53:34, jianli wrote:
> ditto

Done.

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

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:19: namespace android
{
On 2015/12/15 18:30:31, dewittj wrote:
> why namespace android?

Yeah, I was thinking about it. It is because of where the code sits, but given
that we only really apply the namespace to offline_page_bridge, I think it is
reasonable to remove it from here.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:39: // const
base::FilePath::CharType kTestPage3FilePath[] =
On 2015/12/15 18:30:31, dewittj wrote:
> nit: remove commented out code

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:101:
TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesWhenFeatureDisabled) {
On 2015/12/15 23:53:34, jianli wrote:
> HasOfflinePagesWhenFeatureDisabled is confusing. There is no offline page when
> feature is disabled.

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:107:
TEST_F(OfflinePageUtilsBasicTest, HasOfflinePagesWhenStoreIsEmpty) {
On 2015/12/15 23:53:34, jianli wrote:
> ditto for test method name

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:118:
OfflinePageTestStore* store() { return store_; }
On 2015/12/15 23:53:34, jianli wrote:
> nit: add const modifier

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:119:
OfflinePageModel* model() { return model_; }
On 2015/12/15 23:53:34, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:121: const
base::FilePath& test_file_path_1() { return test_file_path_1_; }
On 2015/12/15 23:53:34, jianli wrote:
> ditto for this and two below

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:144: store_
= static_cast<OfflinePageTestStore*>(model()->GetStoreForTesting());
On 2015/12/15 18:30:31, dewittj wrote:
> Any way to get this without doing a static_cast?

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:187:
TEST_F(OfflinePageUtilsTest, MightBeOfflineURL) {
On 2015/12/15 18:30:31, dewittj wrote:
> Could this be in the basic tests?

Done.

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:776: // If the url points to an offline
page, replace it with the original url.
On 2015/12/15 23:53:34, jianli wrote:
> Update comment.

Done.

https://codereview.chromium.org/1521193002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1521193002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:546: DVLOG(1) << "Store is loaded
by now";
On 2015/12/15 18:30:31, dewittj wrote:
> nit: remove the log statements from this file.

Done.

[dewittj, 2015-12-16 22:20:16]

lgtm

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_test_archiver.h (right):

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.h:53: void
set_file_name(const base::FilePath& file_name) {
I think that this be SetFileNameForTest instead.

[jianli, 2015-12-16 23:08:08]

lgtm

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

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:40: // Checks whether
a given |offline_url| points to an offline page.
nit: drop "given"

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:44: // Checks whether
a given |online_url| has a related offline page.
nit: "related to" sounds a bit not intuitive. Also no need to use "given" when
you specify the parameter name. How about:
  Checks whether an offline page for |online_url| exists or not.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:49: // Checks if there
are any offline pages saved in the |browser_context|
nit: suggest dropping the comment for browser_context since you do not add this
everywhere.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:32: int64
kTestPage1BookmarkId = 1234;
nit: why not also adding const for this and one below?

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:56: const
GURL& offline_url_1() const { return offline_url_1_; }
Please comment on these URLs, like their behaviors.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:114:
offline_url_1_ = net::FilePathToFileURL(file_path);
I think this trick is not better than trying to get something out of a test
store (using static_cast). How about still exposing a test store and getting
offline url of last_saved_page from test store in CreateOfflinePages?

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:142: //
Create path to 3rd file.
nit: Create a file that is not associated with any offline page

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_test_archiver.cc (right):

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.cc:16: base::FilePath
CreateArchiveFile(const base::FilePath& archives_dir,
Since this is only used in one place and the logic is simple, why not just
embedded in the calling place?

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.cc:56: if
(file_name_.empty())
nit: add brackets

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_test_archiver.h (right):

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.h:53: void
set_file_name(const base::FilePath& file_name) {
On 2015/12/16 22:20:16, dewittj wrote:
> I think that this be SetFileNameForTest instead.

Why not just passing in the constructor?

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.h:64: base::FilePath
file_name_;
nit: we tend to use filename

[fgorski, 2015-12-17 00:35:17]

fgorski@chromium.org changed reviewers:
+ newt@chromium.org, thakis@chromium.org

[fgorski, 2015-12-17 00:35:17]

thakis@ please review the .gn/.gyp files

newt@ please review tab_android and add_to_homescreen changes

[Nico, 2015-12-17 02:41:49]

gyp/gn files lgtm

[newt (away), 2015-12-17 03:08:45]

Two nits. Otherwise, tab_android and add_to_homescreen_data_fetcher lgtm

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

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:26: static GURL
GetOfflineURLByOnlineURL(content::BrowserContext* browser_context,
"By" doesn't seem like the right preposition. Maybe use "For" or "From"?

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:30: // in
nit: wrapping

[fgorski, 2015-12-17 19:00:14]

All addressed. Thanks for review.

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

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:26: static GURL
GetOfflineURLByOnlineURL(content::BrowserContext* browser_context,
On 2015/12/17 03:08:45, newt wrote:
> "By" doesn't seem like the right preposition. Maybe use "For" or "From"?

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:30: // in
On 2015/12/17 03:08:45, newt wrote:
> nit: wrapping

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:40: // Checks whether
a given |offline_url| points to an offline page.
On 2015/12/16 23:08:07, jianli wrote:
> nit: drop "given"

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:44: // Checks whether
a given |online_url| has a related offline page.
On 2015/12/16 23:08:07, jianli wrote:
> nit: "related to" sounds a bit not intuitive. Also no need to use "given" when
> you specify the parameter name. How about:
>   Checks whether an offline page for |online_url| exists or not.

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:49: // Checks if there
are any offline pages saved in the |browser_context|
On 2015/12/16 23:08:07, jianli wrote:
> nit: suggest dropping the comment for browser_context since you do not add
this
> everywhere.

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:32: int64
kTestPage1BookmarkId = 1234;
On 2015/12/16 23:08:08, jianli wrote:
> nit: why not also adding const for this and one below?

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:56: const
GURL& offline_url_1() const { return offline_url_1_; }
On 2015/12/16 23:08:08, jianli wrote:
> Please comment on these URLs, like their behaviors.

Done.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:114:
offline_url_1_ = net::FilePathToFileURL(file_path);
On 2015/12/16 23:08:07, jianli wrote:
> I think this trick is not better than trying to get something out of a test
> store (using static_cast). How about still exposing a test store and getting
> offline url of last_saved_page from test store in CreateOfflinePages?

Done, but I simply decided to ask the model, which does not break any
test presumptions here.

https://codereview.chromium.org/1521193002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:142: //
Create path to 3rd file.
On 2015/12/16 23:08:07, jianli wrote:
> nit: Create a file that is not associated with any offline page

Done.

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_test_archiver.cc (right):

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.cc:16: base::FilePath
CreateArchiveFile(const base::FilePath& archives_dir,
On 2015/12/16 23:08:08, jianli wrote:
> Since this is only used in one place and the logic is simple, why not just
> embedded in the calling place?

Done.

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.cc:56: if
(file_name_.empty())
On 2015/12/16 23:08:08, jianli wrote:
> nit: add brackets

Done.

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_test_archiver.h (right):

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.h:53: void
set_file_name(const base::FilePath& file_name) {
On 2015/12/16 23:08:08, jianli wrote:
> On 2015/12/16 22:20:16, dewittj wrote:
> > I think that this be SetFileNameForTest instead.
This is a test helper class so we don't need to add the suffix.

> 
> Why not just passing in the constructor?

Because it is optional in the same way set_delayed is.
I don't want to modify the constructor, because there are other tests dependent
on this thing. We make a separate patch to change this.

https://codereview.chromium.org/1521193002/diff/80001/components/offline_page...
components/offline_pages/offline_page_test_archiver.h:64: base::FilePath
file_name_;
On 2015/12/16 23:08:08, jianli wrote:
> nit: we tend to use filename

Done.

[fgorski, 2015-12-17 19:37:05]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-12-17 19:37:06]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
dewittj@chromium.org, jianli@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1521193002/#ps100001
(title: "Addressing final CR feedback")

[commit-bot: I haz the power, 2015-12-17 19:38:28]

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

[commit-bot: I haz the power, 2015-12-17 20:39:38]

Description was changed from

==========
[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing 

BUG=565576
==========

to

==========
[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing 

BUG=565576
==========

[commit-bot: I haz the power, 2015-12-17 20:39:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-17 20:40:26]

Description was changed from

==========
[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing 

BUG=565576
==========

to

==========
[Offline pages] Refactor URL conversions from TabAndroid

This patch moves logic from the tab_android.cc to
c/b/a/offline_pages/offline_page_utils.cc

It also introduces test for that logic, as well
as a simplified keyed service for testing

BUG=565576

Committed: https://crrev.com/1d4c9c97f09b2e490a2f758c4a31b7c9f8cb6686
Cr-Commit-Position: refs/heads/master@{#365874}
==========

[commit-bot: I haz the power, 2015-12-17 20:40:27]

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