Plumbed QUIC errors through to WebURLError which is where HAR files and error pages get them from. [chromium/src : master]

0 views

Skip to first unread message

Brad Lassey (Gerrit)

unread,

Feb 1, 2018, 2:42:10 PM2/1/18

to blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

This change is ready for review.

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Brad Lassey (Gerrit)

unread,

Feb 1, 2018, 2:42:59 PM2/1/18

to blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

This change is ready for review.

Patch set 2:Commit-Queue +1

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Matt Menke (Gerrit)

unread,

Feb 1, 2018, 2:54:41 PM2/1/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, creis...@chromium.org, extension...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Oops, sent a response on the bug before I realized you sent this out for review. Tell me if you want to go with this approach, and I'll do a full review.

Quick preview of my response: Needs a test in services/network, and maybe in content/browser/loader as well. Not sure what sort of tests we do, renderer-side. May just have to rely on integration tests once we do something with the extra error code.

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Brad Lassey (Gerrit)

unread,

Feb 16, 2018, 9:44:27 AM2/16/18

to blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

The OSX failure is known and the owners are disabling that test http://crbug.com/811685

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Matt Menke (Gerrit)

unread,

Feb 27, 2018, 4:47:47 PM2/27/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

I'm assuming this isn't ready for review? The try runs have consistently had a lot of red, and I don't recall explicitly being asked to review it.

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Matt Menke (Gerrit)

unread,

Mar 8, 2018, 12:16:34 PM3/8/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Patch Set 6:

I'm assuming this isn't ready for review? The try runs have consistently had a lot of red, and I don't recall explicitly being asked to review it.

Sorry, I hadn't realized this was ready for review - trybots were always red, and you never clearly asked me for review after we had the discussion about approach. In general, if you have to wait a day or two for a review, you should ping the reviewer, unless they've told you to expect a delay.

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Matt Menke (Gerrit)

unread,

Mar 8, 2018, 12:28:54 PM3/8/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Once again, so sorry for the delay! Should we display the QUIC errors on network error page instead of / in addition to ERR_QUIC_PROTOCOL_ERROR? These wouldn't be useful for end users, but developers may find them useful.

If so, the relevant code is in:

components/error_page/common/
chrome/renderer/net/

1 comment:

File net/base/net_errors.cc:
- Patch Set #6, Line 6: quic_error_codes
  nit: Blank line between primary header and other includes.

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Matt Menke (Gerrit)

unread,

Mar 8, 2018, 12:29:23 PM3/8/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, rdsmit...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

And this LGTM as-is (Modulo the nit), though if you update the error page, will need another pass.

Patch set 6:Code-Review +1

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Brad Lassey (Gerrit)

unread,

Mar 8, 2018, 2:00:23 PM3/8/18

to blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Patch Set 6:
(1 comment)

Once again, so sorry for the delay! Should we display the QUIC errors on network error page instead of / in addition to ERR_QUIC_PROTOCOL_ERROR? These wouldn't be useful for end users, but developers may find them useful.
If so, the relevant code is in:
components/error_page/common/
chrome/renderer/net/

If we want to do that, I think it can be a separate CL

Patch set 7:Commit-Queue +2

1 comment:

File net/base/net_errors.cc:

- Patch Set #6, Line 6:

- nit: Blank line between primary header and other includes.
  Done

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 8, 2018, 2:00:35 PM3/8/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"Plumbed QUIC errors through to WebURLError which is where HAR files and error pages get them from." https://chromium-review.googlesource.com/c/897706/7

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/897706/7

Bot data: {"action": "start", "triggered_at": "2018-03-08T19:00:19.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "1e7345066fc6ba8deb631cb0c2ba1ef1dffccf63"}

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 8, 2018, 2:17:19 PM3/8/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Try jobs failed on following builders:
  chromium_presubmit on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/chromium_presubmit/53509)

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Brad Lassey (Gerrit)

unread,

Mar 17, 2018, 11:38:28 AM3/17/18

to Kentaro Hara, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Matt Menke

Brad Lassey would like Kentaro Hara to review this change.

Plumbed QUIC errors through to WebURLError which is where HAR files and error pages get them from.

Added extended error codes to WebURLError, URLLoaderCompletionStatus and
ResourceError as well as adding a new ExtendedErrorToString method in
net_errors.h to take the extended error code and include the quic error
if applicable.

Bug:801669
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I503c0197a8e5835be6bbbc2a7209c4373e7a08fd
---
M content/browser/loader/mojo_async_resource_handler.cc
M content/renderer/loader/sync_load_context.cc
M content/renderer/loader/sync_load_response.h
M content/renderer/loader/web_url_loader_impl.cc
M content/renderer/render_frame_impl.cc
M net/BUILD.gn
M net/base/net_errors.cc
M net/base/net_errors.h
M net/quic/core/quic_error_codes.h
M services/network/public/cpp/url_loader_completion_status.cc
M services/network/public/cpp/url_loader_completion_status.h
M services/network/url_loader.cc
M third_party/WebKit/Source/platform/exported/WebURLError.cpp
M third_party/WebKit/Source/platform/loader/fetch/ResourceError.cpp
M third_party/WebKit/Source/platform/loader/fetch/ResourceError.h
M third_party/WebKit/public/platform/WebURLError.h
16 files changed, 56 insertions(+), 10 deletions(-)

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Kentaro Hara (Gerrit)

unread,

Mar 19, 2018, 4:28:08 AM3/19/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Commit Bot, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

LGTM

Patch set 7:Code-Review +1

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Brad Lassey (Gerrit)

unread,

Mar 19, 2018, 1:56:19 PM3/19/18

to blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Kentaro Hara, Commit Bot, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Patch set 7:Commit-Queue +2

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 19, 2018, 3:38:01 PM3/19/18

to Brad Lassey, blink-re...@chromium.org, blink-...@chromium.org, blundell+serv...@chromium.org, bnc+...@chromium.org, cbentze...@chromium.org, creis...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, mlamouri+wa...@chromium.org, nasko+c...@chromium.org, net-r...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, rdsmit...@chromium.org, yzshen...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Kentaro Hara, Matt Menke, Aaron Boodman, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Nate Chapin

Commit Bot merged this change.

Approvals:
  Kentaro Hara: Looks good to me
  Matt Menke: Looks good to me
  Brad Lassey: Commit

Plumbed QUIC errors through to WebURLError which is where HAR files and error pages get them from.

Added extended error codes to WebURLError, URLLoaderCompletionStatus and
ResourceError as well as adding a new ExtendedErrorToString method in
net_errors.h to take the extended error code and include the quic error
if applicable.

Bug: 801669
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I503c0197a8e5835be6bbbc2a7209c4373e7a08fd

Reviewed-on: https://chromium-review.googlesource.com/897706
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Matt Menke <mme...@chromium.org>
Commit-Queue: Brad Lassey <las...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544125}


---
M content/browser/loader/mojo_async_resource_handler.cc
M content/renderer/loader/sync_load_context.cc
M content/renderer/loader/sync_load_response.h
M content/renderer/loader/web_url_loader_impl.cc
M content/renderer/render_frame_impl.cc
M net/BUILD.gn
M net/base/net_errors.cc
M net/base/net_errors.h
M net/quic/core/quic_error_codes.h
M services/network/public/cpp/url_loader_completion_status.cc
M services/network/public/cpp/url_loader_completion_status.h
M services/network/url_loader.cc
M third_party/WebKit/Source/platform/exported/WebURLError.cpp
M third_party/WebKit/Source/platform/loader/fetch/ResourceError.cpp
M third_party/WebKit/Source/platform/loader/fetch/ResourceError.h
M third_party/WebKit/public/platform/WebURLError.h
16 files changed, 56 insertions(+), 10 deletions(-)

To view, visit change 897706. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu