Support serving offline page by offline ID

Support serving offline page by offline ID
BUG=643404
TEST=new tests added

Committed: https://crrev.com/d28e413d6a8607db4f82b5e2dc35c58bea543098
Cr-Commit-Position: refs/heads/master@{#418089}

[jianli, 2016-09-08 01:24:17]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 01:24:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2016-09-08 01:24:51]

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

[fgorski, 2016-09-08 02:25:01]

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

[fgorski, 2016-09-08 02:25:02]

minor drive by comments.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:157: if (key ==
kOfflinePageHeaderPersistKey)
is there a place where you call toLower? Or should you make comparisons case
insensitive, perhaps?
This applies to all key comparisons.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:423: const
GURL& online_url,
nit: since we no longer have the concept of offline_url, perhaps calling it
simply url makes sense?

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:431: // If the
found offline page does not has same online URL as the request URL,
nit: does not have the same URL as the online URL

[commit-bot: I haz the power, 2016-09-08 02:45:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 02:45:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Dmitry Titov, 2016-09-09 04:33:07]

https://codereview.chromium.org/2322833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2322833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:178:
headers.put("X-Chrome-offline", "reason=download namespace=download id=" +
guid);
no need to pass namespace here, guid is unique already.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:120: class
OfflinePageHeader {
Nice to add a class for that. 
Please add a class comment and comments to public members.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:125: bool
succeeded_to_parse() const { return succeeded_to_parse_; }
Naming suggestion: successfully_parsed_

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:126: bool
need_to_persist() const { return need_to_persist_; }
Shouldn't this always be 'true' ?
Again, if persistence is not a part of this CL, this probably should not be
here.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:127: const
std::string& reason() const { return reason_; }
this looks like enum that this class should expose, to avoid comparing strings
later in the code. That would encapsulate string comparisons in this class.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:133: 
extra empty line

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:167:
succeeded_to_parse_ = true;
Are there valid combinations and invalid combinations? For example, setting id
but not namespace?

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:532: // If an
id, either offline ID or client ID, is present in the offline header,
This logic is too complicated. It's hard to recall it looking at headers for the
request w/o inspecting the code. The culprit is merging client_id.id and
offline_id in the header, and this logic later tries to figure out which id was
actually specified. Also there is namespace that adds confusion and potentially
will need a wildcard or list (when async_loading is added)

Instead, I'd propose to use explicit instructions in the custom header, as
following:
client_id=<guid> or
offline_id=12345

Note there is no need to pass namespace since those are both unique ids.

As you say, the header that has both or neither or has one but there is no page
wiht such id would fall back to online URL.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.h:23: // The name
used in name-value pair of kLoadingOfflinePageHeader to tell if the
Need to update comment here - there is no more kLoadingOfflinePageHeader. Same
in below comments.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.h:25: extern const
char kOfflinePageHeaderPersistKey[];
Is this from a separate CL? This CL does not seem to deal with persistence.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:363:
std::move(clock));
This is very unorthodox thing you are doing here with capturing a raw pointer
and then std::move'ing it out... I don't think this is how the std::unique_ptr's
are used :)

Correct solutions exist but they are a bit more involved, which is perhaps why
you went this route. I'd rather avoid doing the whole thing with test clock in
this potentially mergeable CL, especially considering the test is only
marginally improved by it.

[jianli, 2016-09-10 00:56:04]

https://codereview.chromium.org/2322833002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java
(right):

https://codereview.chromium.org/2322833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadBridge.java:178:
headers.put("X-Chrome-offline", "reason=download namespace=download id=" +
guid);
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> no need to pass namespace here, guid is unique already.

Changed to pass id per discussion.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:120: class
OfflinePageHeader {
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Nice to add a class for that. 
> Please add a class comment and comments to public members.

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:125: bool
succeeded_to_parse() const { return succeeded_to_parse_; }
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Naming suggestion: successfully_parsed_

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:126: bool
need_to_persist() const { return need_to_persist_; }
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Shouldn't this always be 'true' ?
I think it should always be false because we should only persist if needed.

> Again, if persistence is not a part of this CL, this probably should not be
> here.
Yes, persistence is in another CL that will be landed before this one. So I
would like this patch to also contain the parsing for this field.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:127: const
std::string& reason() const { return reason_; }
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> this looks like enum that this class should expose, to avoid comparing strings
> later in the code. That would encapsulate string comparisons in this class.

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:133: 
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> extra empty line

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:157: if (key ==
kOfflinePageHeaderPersistKey)
On 2016/09/08 02:25:02, fgorski wrote:
> is there a place where you call toLower? Or should you make comparisons case
> insensitive, perhaps?
> This applies to all key comparisons.

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:167:
succeeded_to_parse_ = true;
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Are there valid combinations and invalid combinations? For example, setting id
> but not namespace?

After I remove namespace field, all remaining 3 fields are orthogonal.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:423: const
GURL& online_url,
On 2016/09/08 02:25:02, fgorski wrote:
> nit: since we no longer have the concept of offline_url, perhaps calling it
> simply url makes sense?

Agreed. But I rather make another patch after landing removing offline URL to
replace all occurrences of online_url to url since we still have quite a few
references in offline API.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:431: // If the
found offline page does not has same online URL as the request URL,
On 2016/09/08 02:25:02, fgorski wrote:
> nit: does not have the same URL as the online URL

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.cc:532: // If an
id, either offline ID or client ID, is present in the offline header,
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> This logic is too complicated. It's hard to recall it looking at headers for
the
> request w/o inspecting the code. The culprit is merging client_id.id and
> offline_id in the header, and this logic later tries to figure out which id
was
> actually specified. Also there is namespace that adds confusion and
potentially
> will need a wildcard or list (when async_loading is added)
> 
> Instead, I'd propose to use explicit instructions in the custom header, as
> following:
> client_id=<guid> or
> offline_id=12345
> 
> Note there is no need to pass namespace since those are both unique ids.
> 
> As you say, the header that has both or neither or has one but there is no
page
> wiht such id would fall back to online URL.

Per discussion, we only support passing offline id now.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.h:23: // The name
used in name-value pair of kLoadingOfflinePageHeader to tell if the
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Need to update comment here - there is no more kLoadingOfflinePageHeader. Same
> in below comments.

Done.

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job.h:25: extern const
char kOfflinePageHeaderPersistKey[];
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> Is this from a separate CL? This CL does not seem to deal with persistence.

Another CL (https://codereview.chromium.org/2310363002/) adds the "persist"
field, but the parsing of it is done strictly in place to allow easy merge into
M54 if needed.

I just want this OfflinePageHeader parsing class to also handle this additional
field. After landing these 2 patches, I am going to have a 3rd patch to move
OfflinePageHeader into components/offline_pages/request_header in order for
ContentSerializedNavigationBuilder to call the common header parsing code.

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

https://codereview.chromium.org/2322833002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:363:
std::move(clock));
On 2016/09/09 04:33:06, Dmitry Titov wrote:
> This is very unorthodox thing you are doing here with capturing a raw pointer
> and then std::move'ing it out... I don't think this is how the
std::unique_ptr's
> are used :)
> 
> Correct solutions exist but they are a bit more involved, which is perhaps why
> you went this route. I'd rather avoid doing the whole thing with test clock in
> this potentially mergeable CL, especially considering the test is only
> marginally improved by it. 

Updated per discussion.

[jianli, 2016-09-10 00:56:12]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[jianli, 2016-09-10 00:56:28]

Description was changed from

==========
Support serving offline page by offline ID or client ID

BUG=643404
TEST=new tests added
==========

to

==========
Support serving offline page by offline ID
BUG=643404
TEST=new tests added
==========

[commit-bot: I haz the power, 2016-09-10 00:56:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-09-10 01:24:41]

lgtm

[commit-bot: I haz the power, 2016-09-10 02:05:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 02:05:39]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jianli, 2016-09-12 21:46:34]

The CQ bit was checked by jianli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:47:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jianli, 2016-09-12 22:31:09]

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

[jianli, 2016-09-12 22:31:09]

erg, for simple change in profiles/profile_impl_io_data.cc which removes
unneeded parameter

[Elliot Glaysher, 2016-09-12 22:42:32]

profiles lgtm

[commit-bot: I haz the power, 2016-09-12 22:51:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:51:26]

Dry run: This issue passed the CQ dry run.

[jianli, 2016-09-12 22:53:47]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-09-12 22:53:47]

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/2322833002/#ps40001
(title: "Remove unused variable to fix trybot")

[commit-bot: I haz the power, 2016-09-12 22:54:16]

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-09-12 22:58:46]

Description was changed from

==========
Support serving offline page by offline ID
BUG=643404
TEST=new tests added
==========

to

==========
Support serving offline page by offline ID
BUG=643404
TEST=new tests added
==========

[commit-bot: I haz the power, 2016-09-12 22:58:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-12 23:00:57]

Description was changed from

==========
Support serving offline page by offline ID
BUG=643404
TEST=new tests added
==========

to

==========
Support serving offline page by offline ID
BUG=643404
TEST=new tests added

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

[commit-bot: I haz the power, 2016-09-12 23:00:58]

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