Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421743002:
Implement chrome://network-errors for direct access to network error interstitials (Closed)
|
Created:
5 years, 2 months ago by edwardjung Modified:
5 years ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement chrome://network-errors for direct access to network error interstitials
+ chrome://network-errors/ lists all the network errors.
+ chrome://network-error/<err_code> shows a specific network error.
BUG=
Committed: https://crrev.com/6e6ebd18b801a34bf703c095f343cfbb574f2a13
Cr-Commit-Position: refs/heads/master@{#362968}
Patch Set 1 #
Total comments: 35
Patch Set 2 : Address comments. Changed way the error code listing is obtained. #Patch Set 3 : Add unit test #
Total comments: 8
Patch Set 4 : Fix unit test, made listing mobile friendly #
Total comments: 11
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Total comments: 24
Patch Set 7 : Address reviewer comments #Patch Set 8 : #Patch Set 9 : #
Total comments: 10
Patch Set 10 : Switch to use URLRequestErrorJob to display error pages #Patch Set 11 : Clean up NetworkErrorRequestJob remnants #
Total comments: 14
Patch Set 12 : Address dbeam comments #Patch Set 13 : Fix presubmit errors #
Total comments: 8
Patch Set 14 : Address comments from jochen #Messages
Total messages: 49 (14 generated)
Matt, do you mind doing a quick pass of this implementation as a sanity check. I'm using a macro to generate the error code list which I wasn't sure if it was the best way to do this. Ideally I would only show a list of the errors we have custom messaging for the LocalizeErrorMaps defined in common::LocalizedError but I had a hard time exposing a function that I could access from content::NetworkErrorRequestJob. Thanks.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... File content/browser/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. (c) not needed in new files. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:12: #include "net/url_request/url_request_error_job.h" Not needed, since you're using another approach (Which is fine) https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:17: NetworkErrorRequestJob::NetworkErrorRequestJob( I think we want browser tests for this, both that it displays an error page, and for the index. Error pages are generated in chrome/, but I think we can actually make a content/ test for this - a WebContentsObserver does see error codes for navigations. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:20: : net::URLRequestSimpleJob(request, network_delegate) { BUG: Need to initialize error_code_. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:24: int offset = parsed.CountCharactersBefore(url::Parsed::PATH, false) + 1; Just use this: request->url().path().substr(1, std::string::npos); https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:26: if (offset < static_cast<int>(spec.size())) { Using above suggestion, this just becomes: if (!base::StringToInt(whatever, &error_code_)) error_code_ = 0; https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:33: void NetworkErrorRequestJob::ListAllErrorCodes(std::string* data) { This can just go in an anonymous namespace. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:42: data->append(net::EscapeForHTML(unescaped_title)); Why is this needed? https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:48: data->append("<li><a href=" #value ">" #label "</a></li>"); I really don't like to add all these strings. There are a couple ways to fix this. net::GetNetConstants() returns a dictionary which contains a list of network errors in its "netError" field. We could either use that, or move the array with the into net_errors.h, and provide some public way to access it (Problem with making it accessible it easily accessible is we don't know its length). Or we could add a method IsValidNetError, and make it use the same backing code as ErrorToShortString, and then just call it on all numbers from -2 to -2000... We also want to exclude net::OK (Which is 0, so will just display all error codes) and maybe net::ERR_IO_PENDING from the list (Which will just cause the navigation to hang). https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:51: data->append("</ul>"); </body></html>? https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:61: data->clear(); Not needed. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:64: return error_code_; BUG: This will cause invalid memory accesses if error_code_ is > 0, and a hung request if it's ERR_IO_PENDING (Though that one is not really the end of the world). I also don't think we want to allow passing non-standard network error codes to the renderer. That will, at least, hit a NOTREACHED in debug builds. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:67: return net::OK; I'd actually include this case first (As the "error" / fallback case), and get rid of the else. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... File content/browser/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. (c) not needed in new files. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:6: #define CONTENT_BROWSER_NETWORK_ERROR_REQUEST_JOB_H_ This file should probably be in content/browser/net https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:18: net::NetworkDelegate* network_delegate); Should probably forward declare URLRequest and NetworkDelegate. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:34: DISALLOW_IMPLICIT_CONSTRUCTORS(NetworkErrorRequestJob); include macros.h DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... content/browser/webui/url_data_manager_backend.cc:511: if (request->url().SchemeIs(kChromeUIScheme) && Should have a comment about this. https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... content/browser/webui/url_data_manager_backend.cc:512: request->url().host() == kChromeUINetworkErrorHost) { Hrm...Doesn't look like there's an easy way to put this in chrome/, unfortunately, or I'd say it should be there.
Thanks for the comments. I've been away on a research trip so only just picking this back up. I've updated the listing using the suggested dictionary over the macro. I still need to put together a test for the page. Unfortunately your previous comments have been lost since I moved the files. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... File content/browser/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/10/22 15:41:31, mmenke wrote: > (c) not needed in new files. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:12: #include "net/url_request/url_request_error_job.h" On 2015/10/22 15:41:31, mmenke wrote: > Not needed, since you're using another approach (Which is fine) Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:20: : net::URLRequestSimpleJob(request, network_delegate) { On 2015/10/22 15:41:31, mmenke wrote: > BUG: Need to initialize error_code_. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:24: int offset = parsed.CountCharactersBefore(url::Parsed::PATH, false) + 1; On 2015/10/22 15:41:31, mmenke wrote: > Just use this: request->url().path().substr(1, std::string::npos); Thanks. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:26: if (offset < static_cast<int>(spec.size())) { On 2015/10/22 15:41:31, mmenke wrote: > Using above suggestion, this just becomes: > > if (!base::StringToInt(whatever, &error_code_)) > error_code_ = 0; Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:33: void NetworkErrorRequestJob::ListAllErrorCodes(std::string* data) { On 2015/10/22 15:41:30, mmenke wrote: > This can just go in an anonymous namespace. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:42: data->append(net::EscapeForHTML(unescaped_title)); On 2015/10/22 15:41:31, mmenke wrote: > Why is this needed? Removed. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:51: data->append("</ul>"); On 2015/10/22 15:41:30, mmenke wrote: > </body></html>? Technically not needed in HTML5. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:61: data->clear(); On 2015/10/22 15:41:30, mmenke wrote: > Not needed. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.cc:67: return net::OK; On 2015/10/22 15:41:30, mmenke wrote: > I'd actually include this case first (As the "error" / fallback case), and get > rid of the else. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... File content/browser/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/10/22 15:41:31, mmenke wrote: > (c) not needed in new files. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:6: #define CONTENT_BROWSER_NETWORK_ERROR_REQUEST_JOB_H_ On 2015/10/22 15:41:31, mmenke wrote: > This file should probably be in content/browser/net Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:18: net::NetworkDelegate* network_delegate); On 2015/10/22 15:41:31, mmenke wrote: > Should probably forward declare URLRequest and NetworkDelegate. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/network_err... content/browser/network_error_request_job.h:34: DISALLOW_IMPLICIT_CONSTRUCTORS(NetworkErrorRequestJob); On 2015/10/22 15:41:31, mmenke wrote: > include macros.h > > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... content/browser/webui/url_data_manager_backend.cc:511: if (request->url().SchemeIs(kChromeUIScheme) && On 2015/10/22 15:41:31, mmenke wrote: > Should have a comment about this. Done. https://codereview.chromium.org/1421743002/diff/1/content/browser/webui/url_d... content/browser/webui/url_data_manager_backend.cc:512: request->url().host() == kChromeUINetworkErrorHost) { On 2015/10/22 15:41:31, mmenke wrote: > Hrm...Doesn't look like there's an easy way to put this in chrome/, > unfortunately, or I'd say it should be there. Acknowledged.
Re content browser tests. This is new to me, I want to parse the actual HTML content of the page to match an error code. Is there an example you can point me to. Or would checking the response code in the headers be sufficient.
On 2015/11/06 15:37:38, edwardjung wrote: > Re content browser tests. This is new to me, I want to parse the actual HTML > content of the page to match an error code. Is there an example you can point me > to. Or would checking the response code in the headers be sufficient. We already have tests for Chrome's error pages in chrome/, I don't think we need more. Content's error pages are blank, but we pass the errors over the WebContents observer. It looks like url_data_manager_backend does have its own unit tests, which simulate network requests. I was thinking browser tests would be easier, but since we already have a test fixture, let's just use that. Looks like you can test everything adequately with them. You can get the error codes from URLRequest::Status() (Check .status is URLRequestStatus::FAILED, and check the error code as well). The TestDelegate records the body for the index, if you want to have some minimal test for that as well.
> You can get the error codes from URLRequest::Status() (Check .status is > URLRequestStatus::FAILED, and check the error code as well). The TestDelegate > records the body for the index, if you want to have some minimal test for that > as well. Okay I gave this a try. However the request status seems to come back as io_pending all the time in the two requests I put together. So it seems the request doesn't ever finish. Am I missing something here? Have a look at url_data_manager_backend_unittest.cc Thanks.
On 2015/11/11 15:08:55, edwardjung wrote: > > You can get the error codes from URLRequest::Status() (Check .status is > > URLRequestStatus::FAILED, and check the error code as well). The TestDelegate > > records the body for the index, if you want to have some minimal test for that > > as well. > > Okay I gave this a try. However the request status seems to come back as > io_pending all the time in the two requests I put together. So it seems the > request doesn't ever finish. Am I missing something here? > > Have a look at url_data_manager_backend_unittest.cc > > Thanks. Hrm..Looks right to me. I can debug it for you, but I probably won't have time until Tuesday.
> Hrm..Looks right to me. I can debug it for you, but I probably won't have time > until Tuesday. That would be great. Thanks.
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend_unittest.cc (right): https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend_unittest.cc:115: base::RunLoop().RunUntilIdle(); Run() (The delegate exits the message loop when needed) https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend_unittest.cc:122: scoped_ptr<net::URLRequest> error_request = Suggest just moving this into a separate test. Fewer issues to worry about (You're using the wrong request in the lines below this point). https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend_unittest.cc:126: base::RunLoop().RunUntilIdle(); Run() https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u... content/browser/webui/url_data_manager_backend_unittest.cc:130: Add a test with a URL with an invalid error code? GURL("chrome://network-errors/-123456789");?
edwardjung@chromium.org changed reviewers: + estade@chromium.org, jochen@chromium.org
Thanks for the pointers Matt.
estade and jochen, could you review the following files respectively:
estade:
content/content_browser.gypi
content/browser/webui/url_data_manager_backend.cc
content/browser/webui/url_data_manager_backend_unittest.cc
jochen:
chrome/common/url_constants.cc
content/public/common/url_constants.cc
content/public/common/url_constants.h
content/browser/net/network_error_request_job.cc
content/browser/net/network_error_request_job.h
Thanks.
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:115:
base::RunLoop().RunUntilIdle();
On 2015/11/17 22:25:49, mmenke wrote:
> Run() (The delegate exits the message loop when needed)
Thanks. Fixed.
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:122:
scoped_ptr<net::URLRequest> error_request =
On 2015/11/17 22:25:50, mmenke wrote:
> Suggest just moving this into a separate test. Fewer issues to worry about
> (You're using the wrong request in the lines below this point).
Done.
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:126:
base::RunLoop().RunUntilIdle();
On 2015/11/17 22:25:49, mmenke wrote:
> Run()
Done.
https://codereview.chromium.org/1421743002/diff/40001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend_unittest.cc:130:
On 2015/11/17 22:25:49, mmenke wrote:
> Add a test with a URL with an invalid error code?
>
> GURL("chrome://network-errors/-123456789");?
Done.
https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:42: data->append("<title>"); this seems like a pretty awkward way to do this, it's hard to read and would be very unwieldy if you wanted to add more stuff to it https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:47: data->append("</head><body style=\"margin:1em; line-height:1.5em\">"); and it doesn't allow for separation of content and presentation
Any suggestions / examples? https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:42: data->append("<title>"); On 2015/11/19 16:35:21, Evan Stade wrote: > this seems like a pretty awkward way to do this, it's hard to read and would be > very unwieldy if you wanted to add more stuff to it What do you recommend. Is there a template I should use? It seemed like each chrome:// page is built differently. . https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:47: data->append("</head><body style=\"margin:1em; line-height:1.5em\">"); On 2015/11/19 16:35:21, Evan Stade wrote: > and it doesn't allow for separation of content and presentation Agreed, but didn't know whether creating a new stylesheet just for this was okay. It would be good to have a base stylesheet for all of these pages so they look consistent
estade@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:42: data->append("<title>"); On 2015/11/19 17:10:14, edwardjung wrote: > On 2015/11/19 16:35:21, Evan Stade wrote: > > this seems like a pretty awkward way to do this, it's hard to read and would > be > > very unwieldy if you wanted to add more stuff to it > > What do you recommend. Is there a template I should use? It seemed like each > chrome:// page is built differently. . I have no idea why a few of the chrome:// internals pages use the simple request job and construct html, css and so forth in a C++ file. You should instead follow the example of the ones that don't: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... +dbeam in case he knows of some reason why this codepath which you're copying has any actual reason to exist.
On 2015/11/19 19:07:14, Evan Stade wrote: > https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... > File content/browser/net/network_error_request_job.cc (right): > > https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... > content/browser/net/network_error_request_job.cc:42: data->append("<title>"); > On 2015/11/19 17:10:14, edwardjung wrote: > > On 2015/11/19 16:35:21, Evan Stade wrote: > > > this seems like a pretty awkward way to do this, it's hard to read and would > > be > > > very unwieldy if you wanted to add more stuff to it > > > > What do you recommend. Is there a template I should use? It seemed like each > > chrome:// page is built differently. . > > I have no idea why a few of the chrome:// internals pages use the simple request > job and construct html, css and so forth in a C++ file. You should instead > follow the example of the ones that don't: I don't think that works, except for the index file. WebUI is not designed to return errors, I believe, so would probably have to use a different domain for the index and for errors.
https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:36: data->append("<!DOCTYPE html>\n<html>\n<head>\n"); an additional nit: <!doctype html> https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:42: data->append("<title>"); On 2015/11/19 19:07:14, Evan Stade wrote: > On 2015/11/19 17:10:14, edwardjung wrote: > > On 2015/11/19 16:35:21, Evan Stade wrote: > > > this seems like a pretty awkward way to do this, it's hard to read and would > > be > > > very unwieldy if you wanted to add more stuff to it > > > > What do you recommend. Is there a template I should use? It seemed like each > > chrome:// page is built differently. . > > I have no idea why a few of the chrome:// internals pages use the simple request > job and construct html, css and so forth in a C++ file. You should instead > follow the example of the ones that don't: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > +dbeam in case he knows of some reason why this codepath which you're copying > has any actual reason to exist. no, they should not exist. agree with estade@ that you should be using HTML for structure, external CSS, and JS for dynamic population like the overwhelming majority of pages already do https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.h:10: #include "base/basictypes.h" what is basictypes for?
Thanks for the suggestions. Have moved the listing code out. It's not yet complete. Have a question in the comments. https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:36: data->append("<!DOCTYPE html>\n<html>\n<head>\n"); On 2015/11/19 20:12:52, Dan Beam wrote: > an additional nit: <!doctype html> Will fix in the HTML template https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.cc:42: data->append("<title>"); On 2015/11/19 20:12:52, Dan Beam wrote: > On 2015/11/19 19:07:14, Evan Stade wrote: > > On 2015/11/19 17:10:14, edwardjung wrote: > > > On 2015/11/19 16:35:21, Evan Stade wrote: > > > > this seems like a pretty awkward way to do this, it's hard to read and > would > > > be > > > > very unwieldy if you wanted to add more stuff to it > > > > > > What do you recommend. Is there a template I should use? It seemed like each > > > chrome:// page is built differently. . > > > > I have no idea why a few of the chrome:// internals pages use the simple > request > > job and construct html, css and so forth in a C++ file. You should instead > > follow the example of the ones that don't: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > +dbeam in case he knows of some reason why this codepath which you're copying > > has any actual reason to exist. > > no, they should not exist. > > agree with estade@ that you should be using HTML for structure, external CSS, > and JS for dynamic population like the overwhelming majority of pages already do Okay I'll switch to using the content_web_ui_controller_factory for the listing of the network errors. Due to the limitation mentioned by mmenke, I'll still leave the actual displaying of network errors to be handled by this class. https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... File content/browser/net/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/60001/content/browser/net/net... content/browser/net/network_error_request_job.h:10: #include "base/basictypes.h" On 2015/11/19 20:12:52, Dan Beam wrote: > what is basictypes for? Removed https://codereview.chromium.org/1421743002/diff/80001/content/browser/webui/c... File content/browser/webui/content_web_ui_controller_factory.cc (right): https://codereview.chromium.org/1421743002/diff/80001/content/browser/webui/c... content/browser/webui/content_web_ui_controller_factory.cc:41: url.host() == kChromeUINetworkErrorsListingHost) { @estade, do I need to do anything more than registering the host here? When accessing the URL, it's forever loading and never returning any page content. It should at least return the template. Is my network_errors_listing_ui class looking okay?
Description was changed from ========== Implement chrome://network-errors for direct access to network error interstitials BUG= ========== to ========== Implement chrome://network-errors for direct access to network error interstitials + chrome://network-errors/ lists all the network errors. + chrome://network-error/<err_code> shows a specific network error. BUG= ==========
estade / dbeam please take another look at the updated implementation for the listings page. I fixed the issues I had previously. Thanks.
did you try this code? https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_error_request_job.cc:39: } nit: no curlies https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. remove (c), probably ought to be 2015 https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:19: static const char kErrorIdField[] = "errorid"; errorId https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.h (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.h:16: // ~NetworkErrorsListingUI() override; remove or uncomment https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... File content/browser/resources/net/network_errors_listing.css (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.css:5: */ /* Copyright 2015 The Chromium Authors. All rights reserved. * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.css:8: font-family: Arial, sans-serif; should this be the same font as other webui pages or does it not matter? https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... File content/browser/resources/net/network_errors_listing.js (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:10: xhr.open('GET', 'network-error-data.json', false); don't do synchronous XHR https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:14: } nit: no curlies https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:17: /** @param {Array} errorCode preferably with Array<...> where ... is the type of each item https://developers.google.com/closure/compiler/docs/js-for-compiler https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:34: var data = requestData(); why are you making a method to only use it once? https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:35: $('pages').textContent = ''; what is this doing? clearing #pages? https://codereview.chromium.org/1421743002/diff/100001/content/public/common/... File content/public/common/url_constants.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/public/common/... content/public/common/url_constants.cc:51: const char kChromeUINetworkErrorsListingURL[] = "chrome://network-error"; should this be... chrome://network-errors?
Thanks for the quick response. I should have been more careful checking the code before sending it out. https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_error_request_job.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_error_request_job.cc:39: } On 2015/11/24 20:08:20, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2015/11/24 20:08:20, Dan Beam wrote: > remove (c), probably ought to be 2015 Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:19: static const char kErrorIdField[] = "errorid"; On 2015/11/24 20:08:20, Dan Beam wrote: > errorId Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.h (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.h:16: // ~NetworkErrorsListingUI() override; On 2015/11/24 20:08:20, Dan Beam wrote: > remove or uncomment Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... File content/browser/resources/net/network_errors_listing.css (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.css:5: */ On 2015/11/24 20:08:20, Dan Beam wrote: > /* Copyright 2015 The Chromium Authors. All rights reserved. > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE file. */ Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.css:8: font-family: Arial, sans-serif; On 2015/11/24 20:08:20, Dan Beam wrote: > should this be the same font as other webui pages or does it not matter? Good point. Switched to use Roboto. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... File content/browser/resources/net/network_errors_listing.js (right): https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:10: xhr.open('GET', 'network-error-data.json', false); On 2015/11/24 20:08:20, Dan Beam wrote: > don't do synchronous XHR Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:14: } On 2015/11/24 20:08:20, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:17: On 2015/11/24 20:08:20, Dan Beam wrote: > /** @param {Array} errorCode > > preferably with Array<...> where ... is the type of each item > > https://developers.google.com/closure/compiler/docs/js-for-compiler Done. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:34: var data = requestData(); On 2015/11/24 20:08:20, Dan Beam wrote: > why are you making a method to only use it once? Removed. https://codereview.chromium.org/1421743002/diff/100001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:35: $('pages').textContent = ''; On 2015/11/24 20:08:20, Dan Beam wrote: > what is this doing? clearing #pages? Removed. https://codereview.chromium.org/1421743002/diff/100001/content/public/common/... File content/public/common/url_constants.cc (right): https://codereview.chromium.org/1421743002/diff/100001/content/public/common/... content/public/common/url_constants.cc:51: const char kChromeUINetworkErrorsListingURL[] = "chrome://network-error"; On 2015/11/24 20:08:20, Dan Beam wrote: > should this be... chrome://network-errors? Done.
https://codereview.chromium.org/1421743002/diff/160001/content/browser/net/ne... File content/browser/net/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/160001/content/browser/net/ne... content/browser/net/network_error_request_job.h:17: class NetworkErrorRequestJob : public net::URLRequestSimpleJob { I think we can get rid of this class, and use URLRequestErrorJob. https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... File content/browser/webui/url_data_manager_backend_unittest.cc (right): https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:118: EXPECT_EQ(error_request->status().error(), net::ERR_NAME_NOT_RESOLVED); This test passes? But you're not using kChromeUINetworkErrorHost... https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:118: EXPECT_EQ(error_request->status().error(), net::ERR_NAME_NOT_RESOLVED); EXPECT_EQ(expected, actual) (x2) https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:130: "ERR_<unknown>"); EXPECT_EQ(expected, actual) (x2) https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:130: "ERR_<unknown>"); I don't think we want to be sending invalid error codes to blink - I'd rather just return ERR_UNEXPECTED or a blank page or something on invalid error codes.
@mmenke, addressed your comments. I now do a check on the error code passed in. If it's invalid, a normal invalid url error code is returned. https://codereview.chromium.org/1421743002/diff/160001/content/browser/net/ne... File content/browser/net/network_error_request_job.h (right): https://codereview.chromium.org/1421743002/diff/160001/content/browser/net/ne... content/browser/net/network_error_request_job.h:17: class NetworkErrorRequestJob : public net::URLRequestSimpleJob { On 2015/11/24 22:42:35, mmenke wrote: > I think we can get rid of this class, and use URLRequestErrorJob. Done. https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... File content/browser/webui/url_data_manager_backend_unittest.cc (right): https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:118: EXPECT_EQ(error_request->status().error(), net::ERR_NAME_NOT_RESOLVED); On 2015/11/24 22:42:35, mmenke wrote: > This test passes? But you're not using kChromeUINetworkErrorHost... My oversight. Hadn't updated the test. https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:118: EXPECT_EQ(error_request->status().error(), net::ERR_NAME_NOT_RESOLVED); On 2015/11/24 22:42:35, mmenke wrote: > EXPECT_EQ(expected, actual) (x2) Done. https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:130: "ERR_<unknown>"); On 2015/11/24 22:42:35, mmenke wrote: > EXPECT_EQ(expected, actual) (x2) Done. https://codereview.chromium.org/1421743002/diff/160001/content/browser/webui/... content/browser/webui/url_data_manager_backend_unittest.cc:130: "ERR_<unknown>"); On 2015/11/24 22:42:35, mmenke wrote: > I don't think we want to be sending invalid error codes to blink - I'd rather > just return ERR_UNEXPECTED or a blank page or something on invalid error codes. Changed so that it will now return ERR_INVALID_URL as per normal invalid URL requests.
https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... File content/browser/resources/net/network_errors_listing.html (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.html:17: <script src="chrome://resources/js/action_link.js"></script> remove action_link.js https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... File content/browser/resources/net/network_errors_listing.js (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:14: var errorPageURL = 'chrome://network-error/'; nit: errorPageUrl https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:30: xhr.open('GET', 'network-error-data.json', true); nit: do you need the async param (true)? https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:33: var data = JSON.parse(xhr.responseText); do you want to handle the semi-likely case that this throws? https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:36: }); indent off https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:37: xhr.send(null); why not just send()? https://codereview.chromium.org/1421743002/diff/200001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:540: request->url().path().substr(1, std::string::npos); this is equivalent to substr(1), I believe
Thanks dbeam. https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... File content/browser/resources/net/network_errors_listing.html (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.html:17: <script src="chrome://resources/js/action_link.js"></script> On 2015/12/01 04:24:17, Dan Beam wrote: > remove action_link.js Done. https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... File content/browser/resources/net/network_errors_listing.js (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:14: var errorPageURL = 'chrome://network-error/'; On 2015/12/01 04:24:17, Dan Beam wrote: > nit: errorPageUrl Done. https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:30: xhr.open('GET', 'network-error-data.json', true); On 2015/12/01 04:24:17, Dan Beam wrote: > nit: do you need the async param (true)? Removed, I'm still in the mindset of developing for legacy browser support. https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:33: var data = JSON.parse(xhr.responseText); On 2015/12/01 04:24:17, Dan Beam wrote: > do you want to handle the semi-likely case that this throws? Done https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:36: }); On 2015/12/01 04:24:17, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1421743002/diff/200001/content/browser/resour... content/browser/resources/net/network_errors_listing.js:37: xhr.send(null); On 2015/12/01 04:24:17, Dan Beam wrote: > why not just send()? Done https://codereview.chromium.org/1421743002/diff/200001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/200001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:540: request->url().path().substr(1, std::string::npos); On 2015/12/01 04:24:17, Dan Beam wrote: > this is equivalent to substr(1), I believe Yep, confirmed.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421743002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421743002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421743002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421743002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
lgtm with comments addressed https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.cc (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:72: NetworkErrorsListingUI::NetworkErrorsListingUI(WebUI* web_ui) : is that actually clang-formatted? https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.h (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.h:16: explicit NetworkErrorsListingUI(WebUI* web_ui); please add a virtual dtor https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:469: bool IsValidNetworkErrorCode(int error_code) { nit. empty line above this one https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:481: for (base::DictionaryValue::Iterator itr(*net_error_codes_dict); won't that crash if net_error_codes_dict is still a nulltr at this point?
Thanks jochen and dbeam. https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.cc (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.cc:72: NetworkErrorsListingUI::NetworkErrorsListingUI(WebUI* web_ui) : On 2015/12/02 12:35:08, jochen wrote: > is that actually clang-formatted? Fixed. https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... File content/browser/net/network_errors_listing_ui.h (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/net/ne... content/browser/net/network_errors_listing_ui.h:16: explicit NetworkErrorsListingUI(WebUI* web_ui); On 2015/12/02 12:35:08, jochen wrote: > please add a virtual dtor Done. https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:469: bool IsValidNetworkErrorCode(int error_code) { On 2015/12/02 12:35:08, jochen wrote: > nit. empty line above this one Done. https://codereview.chromium.org/1421743002/diff/240001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:481: for (base::DictionaryValue::Iterator itr(*net_error_codes_dict); On 2015/12/02 12:35:08, jochen wrote: > won't that crash if net_error_codes_dict is still a nulltr at this point? Added a check before iterating
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421743002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421743002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1421743002/#ps260001 (title: "Address comments from jochen")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421743002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421743002/260001
Message was sent while issue was closed.
Description was changed from ========== Implement chrome://network-errors for direct access to network error interstitials + chrome://network-errors/ lists all the network errors. + chrome://network-error/<err_code> shows a specific network error. BUG= ========== to ========== Implement chrome://network-errors for direct access to network error interstitials + chrome://network-errors/ lists all the network errors. + chrome://network-error/<err_code> shows a specific network error. BUG= ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implement chrome://network-errors for direct access to network error interstitials + chrome://network-errors/ lists all the network errors. + chrome://network-error/<err_code> shows a specific network error. BUG= ========== to ========== Implement chrome://network-errors for direct access to network error interstitials + chrome://network-errors/ lists all the network errors. + chrome://network-error/<err_code> shows a specific network error. BUG= Committed: https://crrev.com/6e6ebd18b801a34bf703c095f343cfbb574f2a13 Cr-Commit-Position: refs/heads/master@{#362968} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/6e6ebd18b801a34bf703c095f343cfbb574f2a13 Cr-Commit-Position: refs/heads/master@{#362968} |
