Support getting offline pages also by original URL

Support getting offline pages also by original URL

BUG=618716
Committed: https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7
Cr-Commit-Position: refs/heads/master@{#433013}

[jianli, 2016-11-16 02:19:45]

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

[commit-bot: I haz the power, 2016-11-16 02:20:23]

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

[jianli, 2016-11-16 02:20:35]

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

[commit-bot: I haz the power, 2016-11-16 02:23:21]

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

[commit-bot: I haz the power, 2016-11-16 02:23:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jianli, 2016-11-16 23:14:35]

Patchset #3 (id:40001) has been deleted

[jianli, 2016-11-16 23:14:54]

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

[commit-bot: I haz the power, 2016-11-16 23:16:23]

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

[fgorski, 2016-11-16 23:43:46]

mostly looks good.
I'd go for an enum. It'll be cleaner.

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_request_job.cc:595: //
TODO(jianli): kicks off the redirect. For now, use the default.
Please document it better. I don't understand what happens here and what is the
plan for future.

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:60: DCHECK(url ==
page.original_url);
what about the fragment here?
Why is there no symmetry with the code above?

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:33: bool
search_by_final_url_only,
Can we have an enum for this, as opposed to a boolean?
I don't like that we put this everywhere in the code:
false /* search_by_final_url_only */

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:169: bool
search_by_final_url_only,
enum here would be good too.

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:635: if (url ==
id_page_pair.second.url ||
this does exact matching, whereas outside of this we were dropping fragment. Can
you make sure that it WAI?

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:991: int i = -1;
Add a comment about this trick.

[jianli, 2016-11-17 01:12:13]

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

[jianli, 2016-11-17 01:12:15]

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_request_job.cc:595: //
TODO(jianli): kicks off the redirect. For now, use the default.
On 2016/11/16 23:43:46, fgorski wrote:
> Please document it better. I don't understand what happens here and what is
the
> plan for future.

This will be done in my 3rd patch which is going to be sent out for review soon.

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:60: DCHECK(url ==
page.original_url);
On 2016/11/16 23:43:46, fgorski wrote:
> what about the fragment here?
> Why is there no symmetry with the code above?

With more thought, I decided to change to do match without fragment for original
url.

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

https://codereview.chromium.org/2503853004/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:33: bool
search_by_final_url_only,
On 2016/11/16 23:43:46, fgorski wrote:
> Can we have an enum for this, as opposed to a boolean?
> I don't like that we put this everywhere in the code:
> false /* search_by_final_url_only */

Done.

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:169: bool
search_by_final_url_only,
On 2016/11/16 23:43:46, fgorski wrote:
> enum here would be good too.

Done.

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:635: if (url ==
id_page_pair.second.url ||
On 2016/11/16 23:43:46, fgorski wrote:
> this does exact matching, whereas outside of this we were dropping fragment.
Can
> you make sure that it WAI?

With more thought, changed to also search against original url with fragment
stripped.

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

https://codereview.chromium.org/2503853004/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:991: int i = -1;
On 2016/11/16 23:43:46, fgorski wrote:
> Add a comment about this trick.

Done.

[commit-bot: I haz the power, 2016-11-17 01:13:29]

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

[jianli, 2016-11-17 01:48:57]

I realized that it would be safer if we would only allow exact match against
original url used in redirect. So I reverted it to previous behavior.

[jianli, 2016-11-17 01:50:06]

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

[commit-bot: I haz the power, 2016-11-17 01:50:54]

Dry run: 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-11-17 03:53:23]

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

[commit-bot: I haz the power, 2016-11-17 03:53:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[vitaliii, 2016-11-17 17:07:59]

Description was changed from

==========
Support getting offline pages also by original URL

BUG=618716
==========

to

==========
Support getting offline pages also by original URL

BUG=618716
==========

[fgorski, 2016-11-17 17:23:53]

lgtm

[vitaliii, 2016-11-17 17:29:15]

On 2016/11/17 17:23:53, fgorski wrote:
> lgtm

Could someone elaborate on why you've choose matching both original url and
final url instead of original url only?
We were going to match original url only, but it seems that your approach should
work too.
I am wondering whether we missing anything here.

[vitaliii, 2016-11-17 18:41:11]

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

https://codereview.chromium.org/2503853004/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:414:
OfflinePageModel::URLSearchMode::SEARCH_BY_FINAL_URL_ONLY,
If you replace this with SEARCH_BY_ALL_URLS, then our (NTP's) code will work as
expected with sites, which redirect.
If you don't, we will *not* be terribly broken and no crashes.
Please ask fgorski@ for more details, in case I am not available.

[jianli, 2016-11-17 20:59:26]

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

https://codereview.chromium.org/2503853004/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:414:
OfflinePageModel::URLSearchMode::SEARCH_BY_FINAL_URL_ONLY,
On 2016/11/17 18:41:11, vitaliii wrote:
> If you replace this with SEARCH_BY_ALL_URLS, then our (NTP's) code will work
as
> expected with sites, which redirect.
> If you don't, we will *not* be terribly broken and no crashes.
> Please ask fgorski@ for more details, in case I am not available. 

Done.

[jianli, 2016-11-17 21:00:33]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-11-17 21:00:33]

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/2503853004/#ps120001
(title: "Make NTP redirect work per comment")

[commit-bot: I haz the power, 2016-11-17 21:03: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-11-17 23:18:19]

Description was changed from

==========
Support getting offline pages also by original URL

BUG=618716
==========

to

==========
Support getting offline pages also by original URL

BUG=618716
==========

[commit-bot: I haz the power, 2016-11-17 23:18:21]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-11-17 23:21:48]

Description was changed from

==========
Support getting offline pages also by original URL

BUG=618716
==========

to

==========
Support getting offline pages also by original URL

BUG=618716

Committed: https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7
Cr-Commit-Position: refs/heads/master@{#433013}
==========

[commit-bot: I haz the power, 2016-11-17 23:21:49]

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