Track original URL when downloading a redirected page

Track original URL when downloading a redirected page

BUG=653194
TEST=tests updated
TBR=mmenke@chromium.org,bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2713013002
Cr-Commit-Position: refs/heads/master@{#453444}
Committed: https://chromium.googlesource.com/chromium/src/+/918265d09b535e2b35653aae88dfa5268608ff71

[jianli, 2017-02-23 21:34:20]

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

[jianli, 2017-02-23 21:34:24]

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

[commit-bot: I haz the power, 2017-02-23 21:34:58]

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, 2017-02-23 22:38:25]

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

[commit-bot: I haz the power, 2017-02-23 22:38:25]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-02-27 17:34:36]

Mostly OK. Some explicit documentation of presumptions missing as well as a test
for upgrade scenario.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:158: // when
redirects occur.
please sync. I think Cathy was making a similar change recently.
It may make sense to add Cathy to this review.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:355:
params.user_requested = static_cast<bool>(user_requested);
are you missing availability here?

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:249:
params.user_requested = true;
are you missing availability here?

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:98: // Returns the
original of the given web contents. Empty URL is returned if
nit: original URL

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:99: // no redirect
occurs.
nit: occurred.

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:192:
offline_pages::ClientId(kDownloadNamespace, kTestPage3ClientId);
are you missing availability here?

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:302: base::GenerateGUID());
are you missing availability and user_requested?

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
params.user_requested = true;
availability?

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:76: bool
user_requested;
document the default (if you wish to use it in code).

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:78: // Request
availability.
if we want to roll with the default, lets document the default.

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:247:
RequestCoordinator::SavePageLaterParams params;
Please add test for default parameters of save_page_later_params, if you wish to
use that in our code (user_requested and availability is what I would like to
have covered)

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_queue_store_unittest.cc:286:
request.set_original_url(kUrl2);
please add upgrade test as well.
E.g.
https://cs.chromium.org/chromium/src/components/offline_pages/core/offline_pa...

[jianli, 2017-02-27 23:20:03]

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

[jianli, 2017-02-27 23:20:07]

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:158: // when
redirects occur.
On 2017/02/27 17:34:35, fgorski wrote:
> please sync. I think Cathy was making a similar change recently.
> It may make sense to add Cathy to this review.

Synced. Cathy ported my previous original URL change from prerender.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
File
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc:355:
params.user_requested = static_cast<bool>(user_requested);
On 2017/02/27 17:34:35, fgorski wrote:
> are you missing availability here?

By default, the availability is set to ENABLED_FOR_OFFLINER in the constructor.
I want to avoid passing any value that is not explicitly required to simplify
the code.

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.cc:249:
params.user_requested = true;
On 2017/02/27 17:34:35, fgorski wrote:
> are you missing availability here?

ditto

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:98: // Returns the
original of the given web contents. Empty URL is returned if
On 2017/02/27 17:34:36, fgorski wrote:
> nit: original URL

Done.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils.h:99: // no redirect
occurs.
On 2017/02/27 17:34:36, fgorski wrote:
> nit: occurred.

Done.

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

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:192:
offline_pages::ClientId(kDownloadNamespace, kTestPage3ClientId);
On 2017/02/27 17:34:36, fgorski wrote:
> are you missing availability here?

ditto.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/net/net_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/net/net_...
chrome/browser/net/net_error_tab_helper.cc:302: base::GenerateGUID());
On 2017/02/27 17:34:36, fgorski wrote:
> are you missing availability and user_requested?

ditto.

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:291:
params.user_requested = true;
On 2017/02/27 17:34:36, fgorski wrote:
> availability?

ditto.

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:76: bool
user_requested;
On 2017/02/27 17:34:36, fgorski wrote:
> document the default (if you wish to use it in code).

Done.

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:78: // Request
availability.
On 2017/02/27 17:34:36, fgorski wrote:
> if we want to roll with the default, lets document the default.

Done.

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_coordinator_unittest.cc:247:
RequestCoordinator::SavePageLaterParams params;
On 2017/02/27 17:34:36, fgorski wrote:
> Please add test for default parameters of save_page_later_params, if you wish
to
> use that in our code (user_requested and availability is what I would like to
> have covered)

Done. Added in SavePageLater.

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
File components/offline_pages/core/background/request_queue_store_unittest.cc
(right):

https://codereview.chromium.org/2713013002/diff/20001/components/offline_page...
components/offline_pages/core/background/request_queue_store_unittest.cc:286:
request.set_original_url(kUrl2);
On 2017/02/27 17:34:36, fgorski wrote:
> please add upgrade test as well.
> E.g.
>
https://cs.chromium.org/chromium/src/components/offline_pages/core/offline_pa...

Done.

[commit-bot: I haz the power, 2017-02-27 23:20:48]

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, 2017-02-27 23:25:38]

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

[commit-bot: I haz the power, 2017-02-27 23:25:39]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fgorski, 2017-02-27 23:52:06]

lgtm with nits.

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:75: // Whether
the user requests the save action. Default to true.
nit: defaults

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:78: // Request
availability. Default to ENABLED_FOR_OFFLINER.
nit: defaults

[jianli, 2017-02-27 23:57:00]

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

[commit-bot: I haz the power, 2017-02-27 23:57:35]

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

[jianli, 2017-02-27 23:58:35]

Description was changed from

==========
Track original URL when downloading a redirected page

BUG=653194
TEST=tests updated
==========

to

==========
Track original URL when downloading a redirected page

BUG=653194
TEST=tests updated
TBR=mmenke@chromium.org,bauerb@chromium.org
==========

[jianli, 2017-02-27 23:58:35]

jianli@chromium.org changed reviewers:
+ bauerb@chromium.org, mmenke@chromium.org

[jianli, 2017-02-27 23:59:01]

jianli@chromium.org changed reviewers:
- bauerb@chromium.org, mmenke@chromium.org

[jianli, 2017-02-27 23:59:10]

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:75: // Whether
the user requests the save action. Default to true.
On 2017/02/27 23:52:06, fgorski wrote:
> nit: defaults

Done.

https://codereview.chromium.org/2713013002/diff/50001/components/offline_page...
components/offline_pages/core/background/request_coordinator.h:78: // Request
availability. Default to ENABLED_FOR_OFFLINER.
On 2017/02/27 23:52:06, fgorski wrote:
> nit: defaults

Done.

[jianli, 2017-02-27 23:59:19]

jianli@chromium.org changed reviewers:
+ bauerb@chromium.org, mmenke@chromium.org

[jianli, 2017-02-28 00:02:12]

TBR mmenke@chromium.org for function paramater update in net 
TBR bauerb@chromium.org for function paramater update in webui

[commit-bot: I haz the power, 2017-02-28 00:04:09]

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

[commit-bot: I haz the power, 2017-02-28 00:04:10]

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...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jianli, 2017-02-28 00:12:29]

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

[commit-bot: I haz the power, 2017-02-28 00:12:55]

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, 2017-02-28 01:33:39]

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

[commit-bot: I haz the power, 2017-02-28 01:33:40]

Dry run: This issue passed the CQ dry run.

[jianli, 2017-02-28 01:43:16]

The CQ bit was checked by jianli@chromium.org

[jianli, 2017-02-28 01:43:16]

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/2713013002/#ps90001
(title: "Update BUILD.gn to fix DEP check in analyze")

[commit-bot: I haz the power, 2017-02-28 01:43:53]

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, 2017-02-28 01:50:21]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1488246196371850,
"parent_rev": "8dfb4c2e89fc1b591f4ea59ae8772a8de8820f30", "commit_rev":
"918265d09b535e2b35653aae88dfa5268608ff71"}

[commit-bot: I haz the power, 2017-02-28 01:51:20]

Description was changed from

==========
Track original URL when downloading a redirected page

BUG=653194
TEST=tests updated
TBR=mmenke@chromium.org,bauerb@chromium.org
==========

to

==========
Track original URL when downloading a redirected page

BUG=653194
TEST=tests updated
TBR=mmenke@chromium.org,bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2713013002
Cr-Commit-Position: refs/heads/master@{#453444}
Committed:
https://chromium.googlesource.com/chromium/src/+/918265d09b535e2b35653aae88df...
==========

[commit-bot: I haz the power, 2017-02-28 01:51:21]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/918265d09b535e2b35653aae88df...