Move failed URLRequest checking to OnUrlRequestCompleted

Move failed URLRequest checking to OnUrlRequestCompleted

Failed request checking done in OnUrlRequestDestroyed always returns as
failure - request status is always URLRequestStatus::CANCELED. So moving
to OnUrlRequestCompleted.

BUG=718612
Review-Url: https://codereview.chromium.org/2868733002
Cr-Commit-Position: refs/heads/master@{#472697}
Committed: https://chromium.googlesource.com/chromium/src/+/dde9d26e729f5ba0c92687c72fc80311878a36c8

[Raj, 2017-05-08 04:16:06]

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

[commit-bot: I haz the power, 2017-05-08 04:16:35]

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-05-08 04:53:46]

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

[commit-bot: I haz the power, 2017-05-08 04:53:47]

Dry run: Try jobs failed on following builders:
  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_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_...)

[Raj, 2017-05-08 05:48:38]

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

[commit-bot: I haz the power, 2017-05-08 05:48:56]

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-05-08 06:38:13]

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

[commit-bot: I haz the power, 2017-05-08 06:38:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Raj, 2017-05-08 19:43:25]

rajendrant@chromium.org changed reviewers:
+ tbansal@chromium.org

[Raj, 2017-05-08 19:43:26]

ptal
Will fix tests.

[tbansal1, 2017-05-09 01:14:43]

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:156:
ChromeDataUseRecorder* recorder = GetDataUseRecorder(*request);
why not GetOrCreateDataUseRecorderEntry (similar to OnUrlRequestDestroyed
below)?

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:163: if
(request_info &&
reverse the if-conditional for early return:
if(!request_info ||  request_info->GetResourceType() !=
content::RESOURCE_TYPE_MAIN_FRAME) {
return;
}

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:165: auto
navigation_iter = pending_navigation_data_use_map_.find(
IIUC (please confirm using the spec), you can simply delete the key from the
map:
pending_navigation_data_use_map_.erase(recorder->main_frame_request_id());

There is no need for getting the iterator, and deleting using the iterator. 

If you go this way, it might still be useful to keep the comments.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:170: if
(navigation_iter != pending_navigation_data_use_map_.end()) {
no parens.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:62: void
OnUrlRequestCompleted(net::URLRequest* request, bool started) override;
pass const ref instead of raw ptr?

[megjablon, 2017-05-10 20:41:25]

Description was changed from

==========
Move failed URLRequest checking to OnUrlRequestCompleted

Failed request checking done in OnUrlRequestDestroyed always returns as
failure - request status is always URLRequestStatus::CANCELED. So moving
to OnUrlRequestCompleted.

BUG=718612
==========

to

==========
Move failed URLRequest checking to OnUrlRequestCompleted

Failed request checking done in OnUrlRequestDestroyed always returns as
failure - request status is always URLRequestStatus::CANCELED. So moving
to OnUrlRequestCompleted.

BUG=718612
==========

[rajendrant, 2017-05-11 20:31:38]

rajendrant@google.com changed reviewers:
+ rajendrant@google.com

[rajendrant, 2017-05-11 20:31:39]

ptal

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:156:
ChromeDataUseRecorder* recorder = GetDataUseRecorder(*request);
On 2017/05/09 01:14:43, tbansal1 wrote:
> why not GetOrCreateDataUseRecorderEntry (similar to OnUrlRequestDestroyed
> below)?

GetOrCreateDataUseRecorderEntry is not needed here. There is no need to create
new entry and try to delete the same later.

GetOrCreateDataUseRecorderEntry is not needed in OnUrlRequestDestroyed as well.

Actually, GetOrCreateDataUseRecorder gets called in
DataUseAscriber::OnBeforeUrlRequest(), which creates the entry. Rest of the
places calling GetDataUseRecorder() is sufficient.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:163: if
(request_info &&
On 2017/05/09 01:14:43, tbansal1 wrote:
> reverse the if-conditional for early return:
> if(!request_info ||  request_info->GetResourceType() !=
> content::RESOURCE_TYPE_MAIN_FRAME) {
> return;
> }

Done.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:165: auto
navigation_iter = pending_navigation_data_use_map_.find(
On 2017/05/09 01:14:43, tbansal1 wrote:
> IIUC (please confirm using the spec), you can simply delete the key from the
> map:
> pending_navigation_data_use_map_.erase(recorder->main_frame_request_id());
> 
> There is no need for getting the iterator, and deleting using the iterator. 
> 
> If you go this way, it might still be useful to keep the comments.

Done.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:170: if
(navigation_iter != pending_navigation_data_use_map_.end()) {
On 2017/05/09 01:14:43, tbansal1 wrote:
> no parens.

Done.

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right):

https://codereview.chromium.org/2868733002/diff/20001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:62: void
OnUrlRequestCompleted(net::URLRequest* request, bool started) override;
On 2017/05/09 01:14:43, tbansal1 wrote:
> pass const ref instead of raw ptr?

Done.

[tbansal1, 2017-05-11 20:55:41]

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:157:
ChromeDataUseRecorder* recorder = GetDataUseRecorder(request);
Add DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:166: return;
parens are needed here since the if-conditional spans multiple lines.

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:168: // If
request was not successful, then NavigationHandle in
Do you have to check somewhere that |request| was not successful?

[rajendrant, 2017-05-16 01:04:14]

ptal

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right):

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:157:
ChromeDataUseRecorder* recorder = GetDataUseRecorder(request);
On 2017/05/11 20:55:41, tbansal1 wrote:
> Add DCHECK_CURRENTLY_ON(content::BrowserThread::IO);

Done.

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:166: return;
On 2017/05/11 20:55:41, tbansal1 wrote:
> parens are needed here since the if-conditional spans multiple lines.

Done.

https://codereview.chromium.org/2868733002/diff/60001/chrome/browser/data_use...
chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:168: // If
request was not successful, then NavigationHandle in
On 2017/05/11 20:55:41, tbansal1 wrote:
> Do you have to check somewhere that |request| was not successful?

Yes. This was missed.

[tbansal1, 2017-05-16 14:01:07]

tests are missing otherwise this looks good to me. Thanks for fixing this.

[rajendrant, 2017-05-16 21:57:54]

Added test

[tbansal1, 2017-05-16 22:02:38]

lgtm

[Raj, 2017-05-18 03:36:35]

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

[commit-bot: I haz the power, 2017-05-18 03:38:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rajendrant, 2017-05-18 03:40:14]

Patchset #7 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-05-18 03:45:40]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  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_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_chromeos_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_...)

[commit-bot: I haz the power, 2017-05-18 03:53:21]

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

[commit-bot: I haz the power, 2017-05-18 03:53:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_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_...)

[Raj, 2017-05-18 04:18:45]

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

[commit-bot: I haz the power, 2017-05-18 04:20:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rajendrant, 2017-05-18 06:03:04]

The CQ bit was checked by rajendrant@google.com

[rajendrant, 2017-05-18 06:03:04]

The CQ bit was unchecked by rajendrant@google.com

[rajendrant, 2017-05-18 06:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2868733002/#ps140001
(title: "rebased and fix test")

[rajendrant, 2017-05-18 06:03:36]

The CQ bit was unchecked by rajendrant@google.com

[rajendrant, 2017-05-18 06:03:45]

The CQ bit was checked by rajendrant@google.com

[commit-bot: I haz the power, 2017-05-18 06:04:09]

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-05-18 06:09:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495087384116590,
"parent_rev": "b2dde8c244c58dcf6d38d812ca064f911b1b0fe6", "commit_rev":
"dde9d26e729f5ba0c92687c72fc80311878a36c8"}

[commit-bot: I haz the power, 2017-05-18 06:09:11]

Description was changed from

==========
Move failed URLRequest checking to OnUrlRequestCompleted

Failed request checking done in OnUrlRequestDestroyed always returns as
failure - request status is always URLRequestStatus::CANCELED. So moving
to OnUrlRequestCompleted.

BUG=718612
==========

to

==========
Move failed URLRequest checking to OnUrlRequestCompleted

Failed request checking done in OnUrlRequestDestroyed always returns as
failure - request status is always URLRequestStatus::CANCELED. So moving
to OnUrlRequestCompleted.

BUG=718612

Review-Url: https://codereview.chromium.org/2868733002
Cr-Commit-Position: refs/heads/master@{#472697}
Committed:
https://chromium.googlesource.com/chromium/src/+/dde9d26e729f5ba0c92687c72fc8...
==========

[commit-bot: I haz the power, 2017-05-18 06:09:13]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/dde9d26e729f5ba0c92687c72fc8...