Convert ClientSideDetectionHost to use the new navigation callbacks.

Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2669013002
Cr-Commit-Position: refs/heads/master@{#448880}
Committed: https://chromium.googlesource.com/chromium/src/+/7fe6ef1594443f4ada0819085d946a9fd4054628

[jam, 2017-02-01 21:36:06]

Description was changed from

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
==========

to

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
==========

[jam, 2017-02-01 21:36:06]

jam@chromium.org changed reviewers:
+ mattm@chromium.org

[jam, 2017-02-06 19:23:23]

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

[commit-bot: I haz the power, 2017-02-06 19:24:02]

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-06 19:36:36]

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

[commit-bot: I haz the power, 2017-02-06 19:36:37]

Dry run: Try jobs failed on following builders:
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-02-06 20:13:12]

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

[jam, 2017-02-06 20:13:13]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-06 20:13:36]

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-06 20:30:48]

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

[commit-bot: I haz the power, 2017-02-06 20:30:48]

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

[jam, 2017-02-06 20:48:00]

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

[commit-bot: I haz the power, 2017-02-06 20:48:41]

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

[jam, 2017-02-06 21:03:56]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-06 21:19:08]

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

[commit-bot: I haz the power, 2017-02-06 21:19:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-02-06 21:53:58]

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

[commit-bot: I haz the power, 2017-02-06 21:54:41]

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-06 22:26:49]

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

[commit-bot: I haz the power, 2017-02-06 22:26:50]

Dry run: Try jobs failed on following builders:
  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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-02-07 05:44:56]

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

[jam, 2017-02-07 05:45:09]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-07 05:45:09]

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-07 06:11:27]

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

[commit-bot: I haz the power, 2017-02-07 06:11:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-02-07 20:35:48]

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.h:59: static void
IgnoreMimeTypeCheckForTesting(bool ignore);
Why is this necessary now? If the new stuff means test code no longer gets a
default of text/html, I'd rather have the tests explicitly set that than add
test-only conditions just for this.

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.h:59: static void
IgnoreMimeTypeCheckForTesting(bool ignore);
Also the comment for SetContentsMimeType might need to be updated?
https://cs.chromium.org/chromium/src/content/public/test/test_renderer_host.h...

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
(right):

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:1000:
SetContentsMimeType("application/xhtml+xml");
(also this one is currently getting ignored, but irrelevant if you do my other
comment)

[jam, 2017-02-07 20:39:40]

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host.h (right):

https://codereview.chromium.org/2669013002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host.h:59: static void
IgnoreMimeTypeCheckForTesting(bool ignore);
On 2017/02/07 20:35:47, mattm wrote:
> Why is this necessary now? If the new stuff means test code no longer gets a
> default of text/html,

Correct, that's the reason.

The only place that cared about this was the 5 or so tests in this unittest
file. That's why I changed only this test, instead of adding test-only methods
to NavigationHandle and RenderFrameHost to enable setting this.

> I'd rather have the tests explicitly set that than add
> test-only conditions just for this.

[jam, 2017-02-07 21:27:49]

actually, hold off on reviewing this, I may have found a way to maintain the old
behavior

[jam, 2017-02-07 22:30:28]

Description was changed from

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
==========

to

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-02-07 22:30:32]

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

[commit-bot: I haz the power, 2017-02-07 22:31:08]

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

[jam, 2017-02-07 23:57:24]

ptal, I now set the mime type on the navigation handle from the test harness,
thanks for pointing me this way.

[mattm, 2017-02-08 00:01:09]

lgtm

[commit-bot: I haz the power, 2017-02-08 00:11:39]

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

[commit-bot: I haz the power, 2017-02-08 00:11:42]

Dry run: This issue passed the CQ dry run.

[jam, 2017-02-08 00:19:23]

The CQ bit was checked by jam@chromium.org

[jam, 2017-02-08 00:19:54]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-02-08 01:44:12]

The CQ bit was checked by jam@chromium.org

[jam, 2017-02-08 01:44:13]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2669013002/#ps120001
(title: "nit")

[commit-bot: I haz the power, 2017-02-08 01:44:38]

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-08 03:01:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486518252936890,
"parent_rev": "217d9ba6d751851b48921ac11921791d319462bf", "commit_rev":
"7fe6ef1594443f4ada0819085d946a9fd4054628"}

[commit-bot: I haz the power, 2017-02-08 03:01:36]

Description was changed from

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert ClientSideDetectionHost to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2669013002
Cr-Commit-Position: refs/heads/master@{#448880}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fe6ef1594443f4ada0819085d94...
==========

[commit-bot: I haz the power, 2017-02-08 03:01:37]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7fe6ef1594443f4ada0819085d94...