NavigationThrottle: allow customization of net::Error when blocking

NavigationThrottle: allow customization of net::Error when blocking
a navigation.

Customizing the net error is desirable since the text of
ERR_BLOCKED_BY_CLIENT currently advises the user to try disabling
extensions, which can be situationally confusing (bug 649869).

Throttles can now return values either like this:

  return PROCEED;

or like this:

  return BLOCK_REQUEST;   // uses net::ERR_BLOCKED_BY_CLIENT, as before

or like this (new!):

  return {BLOCK_REQUEST, net::ERR_FAILED};

No throttles use the new syntax yet.

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

[ncarter (slow), 2017-05-08 18:35:44]

Description was changed from

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

BUG=
==========

to

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

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

[ncarter (slow), 2017-05-08 18:36:00]

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

[commit-bot: I haz the power, 2017-05-08 18:36:47]

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 18:49:48]

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

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

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-05-08 18:57:13]

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

[commit-bot: I haz the power, 2017-05-08 18:58:06]

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 19:10:16]

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

[commit-bot: I haz the power, 2017-05-08 19:10:17]

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_...)

[ncarter (slow), 2017-05-09 05:10:27]

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

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

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-09 05:28:41]

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

[commit-bot: I haz the power, 2017-05-09 05:28:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-...)

[ncarter (slow), 2017-05-09 22:21:37]

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

[commit-bot: I haz the power, 2017-05-09 22:23:07]

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-09 23:05:41]

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

[commit-bot: I haz the power, 2017-05-09 23:05:42]

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...)

[ncarter (slow), 2017-05-10 18:54:39]

Fix bad DCHECK

[ncarter (slow), 2017-05-10 18:54:41]

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

[commit-bot: I haz the power, 2017-05-10 18:56:16]

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-10 20:05:54]

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

[commit-bot: I haz the power, 2017-05-10 20:05:55]

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

[ncarter (slow), 2017-05-10 20:19:08]

CONTENT_EXPORT

[ncarter (slow), 2017-05-10 20:19:09]

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

[commit-bot: I haz the power, 2017-05-10 20:20:11]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-10 23:12:02]

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

[commit-bot: I haz the power, 2017-05-10 23:12:39]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-10 23:18:09]

Description was changed from

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

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

to

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

Customizing the net error is desirable since the text of ERR_BLOCKED_BY_CLIENT
currently advises the user to try disabling extensions, which can be
situationally confusing (bug 649869).

Throttles can now return values either like this:

  return PROCEED;

or like this:

  return BLOCK_REQUEST;   // uses net::ERR_BLOCKED_BY_CLIENT, as before

or like this (new!):

  return {BLOCK_REQUEST, net::ERR_FAILED};

No throttles use the new syntax yet.

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

[ncarter (slow), 2017-05-10 23:18:49]

Description was changed from

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

Customizing the net error is desirable since the text of ERR_BLOCKED_BY_CLIENT
currently advises the user to try disabling extensions, which can be
situationally confusing (bug 649869).

Throttles can now return values either like this:

  return PROCEED;

or like this:

  return BLOCK_REQUEST;   // uses net::ERR_BLOCKED_BY_CLIENT, as before

or like this (new!):

  return {BLOCK_REQUEST, net::ERR_FAILED};

No throttles use the new syntax yet.

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

to

==========
NavigationThrottle: allow customization of net::Error when blocking
a navigation.

Customizing the net error is desirable since the text of
ERR_BLOCKED_BY_CLIENT currently advises the user to try disabling
extensions, which can be situationally confusing (bug 649869).

Throttles can now return values either like this:

  return PROCEED;

or like this:

  return BLOCK_REQUEST;   // uses net::ERR_BLOCKED_BY_CLIENT, as before

or like this (new!):

  return {BLOCK_REQUEST, net::ERR_FAILED};

No throttles use the new syntax yet.

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

[ncarter (slow), 2017-05-10 23:19:12]

nick@chromium.org changed reviewers:
+ nasko@chromium.org

[ncarter (slow), 2017-05-10 23:19:12]

nasko, PTAL

[nasko, 2017-05-11 00:09:54]

This is awesome! I just have one comment.

https://codereview.chromium.org/2867833002/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2867833002/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:804: OnRequestFailed(false,
net::ERR_BLOCKED_BY_CLIENT);
Shouldn't we pass results.net_error_code() here? This way we are consistent with
the other On*ChecksComplete methods. I don't think this path is exercised by the
unit tests, but I expect a browser test that does a BLOCK_REQUEST with custom
error code in PlzNavigate mode will fail to work properly.

[commit-bot: I haz the power, 2017-05-11 00:58:37]

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

[commit-bot: I haz the power, 2017-05-11 00:58:38]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-05-11 18:05:44]

https://codereview.chromium.org/2867833002/diff/120001/content/public/browser...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/2867833002/diff/120001/content/public/browser...
content/public/browser/navigation_throttle.h:106: net::Error net_error_code_ :
16;
Chatting with dcheng@ over why we want to pack these, he pointed out that MSVC
might not be honoring that.

[ncarter (slow), 2017-05-11 21:05:12]

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

[commit-bot: I haz the power, 2017-05-11 21:06:25]

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-11 21:12:26]

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

[commit-bot: I haz the power, 2017-05-11 21:12:26]

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

[ncarter (slow), 2017-05-11 22:42:52]

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

[commit-bot: I haz the power, 2017-05-11 22:44:06]

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-12 00:55:20]

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

[commit-bot: I haz the power, 2017-05-12 00:55:21]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-05-17 21:42:15]

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

[commit-bot: I haz the power, 2017-05-17 21:43:28]

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-18 01:59:40]

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

[commit-bot: I haz the power, 2017-05-18 01:59:40]

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_...)

[lgarron, 2017-09-05 22:13:17]

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

[lgarron, 2017-09-05 22:13:18]

I ended up implementing pretty much the same thing for:

https://chromium-review.googlesource.com/c/chromium/src/+/647656
https://chromium-review.googlesource.com/c/chromium/src/+/625337

(In fact, I unintentionally performed the same swizzle of repurposing
"ThrottleCheckResult" into a class while swizzling the old "result" type into
something named "action"! :-D)

However, I also need to include an optional error_page_html string.
Here's a specific plan for meeting in the middle:

- Drop the SupervisedUserNavigationThrottle-specific code for this CL.
- Add a base::Optional<std::string> error_page_html field and remove the
comparison operators.
- ... don't worry about packing?

Does that sound reasonable?

[ncarter (slow), 2017-09-05 23:14:56]

That sounds super-reasonable, thanks!

On Tue, Sep 5, 2017 at 3:13 PM, <lgarron@chromium.org> wrote:

> I ended up implementing pretty much the same thing for:
>
> https://chromium-review.googlesource.com/c/chromium/src/+/647656
> https://chromium-review.googlesource.com/c/chromium/src/+/625337
>
> (In fact, I unintentionally performed the same swizzle of repurposing
> "ThrottleCheckResult" into a class while swizzling the old "result" type
> into
> something named "action"! :-D)
>
> However, I also need to include an optional error_page_html string.
> Here's a specific plan for meeting in the middle:
>
> - Drop the SupervisedUserNavigationThrottle-specific code for this CL.
> - Add a base::Optional<std::string> error_page_html field and remove the
> comparison operators.
> - ... don't worry about packing?
>
> Does that sound reasonable?
>
> https://codereview.chromium.org/2867833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.