Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1228233007:
Shows error messages in the inspector when .respondWith() is called with wrong responses. (Closed)
|
Created:
5 years, 5 months ago by horo Modified:
5 years, 5 months ago CC:
blink-reviews, falken, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionShows error messages in the inspector when .respondWith() is called with wrong responses.
To let the developer know about the reason of the failure.
BUG=505784
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199287
Patch Set 1 #Patch Set 2 : #
Total comments: 25
Patch Set 3 : incorporated falken's comment #
Total comments: 6
Patch Set 4 : incorporated falken's comment #
Messages
Total messages: 23 (7 generated)
Patchset #2 (id:20001) has been deleted
horo@chromium.org changed reviewers: + falken@chromium.org
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
falken@ Could you please review this?
falken@chromium.org changed reviewers: + mkwst@chromium.org
Thanks for working on this! I think the messages could use a "professional" eye, +mkwst. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:30: { I suspect a more user-friendly way to present this to developers is something like: "The FetchEvent for \"<requestURL.string>\" resulted in a network error response: " and then each case outputs a reason. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:34: errorMessage = "An unexpected error occured while handling the FetchEvent (" + requestURL.string() + ")."; "an unexpected error occurred." (this should probably also be done in the default case) https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:36: case WebServiceWorkerResponseErrorPromiseRejected: Can you comment why this doesn't have a message (I believe it's because the rejected promise is already displayed to console). https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:40: errorMessage = "event.preventDefault() for the FetchEvent (" + requestURL.string() + ") without calling event.respondWith() is treated as a network error."; "preventDefault() was called without calling respondWith()." https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:43: errorMessage = "The object with which respond to the request (" + requestURL.string() + ") must be a Response object."; "an object that was not a Response was passed to respondWith()" https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:45: case WebServiceWorkerResponseErrorResponseTypeError: ditto, comment https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:49: errorMessage = "Opaque type response can't respond to the request (" + requestURL.string() + ") which type isn't no-cors."; "an opaque response was used for a request whose type is not no-cors" (yay triple negatives) https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:52: errorMessage = "The type of the Response object with which respond to the client request (" + requestURL.string() + ") must be basic or default."; "the response for a client request must have type \"basic\" or \"default\"" (are these types exposed to script? if not we shouldn't quote them) https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:55: errorMessage = "The body used response can't respond to the request (" + requestURL.string() + ")."; "a Response whose \"bodyUsed\" is \"true\" cannot be used to respond to a request" https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:58: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage)); I suspect this should be a Warning or Info, it's conceivable the developer is intentionally doing this. +mkwst https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.h (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.h:26: static RespondWithObserver* create(ExecutionContext*, int eventID, const KURL&, WebURLRequest::FetchRequestMode, WebURLRequest::FrameType); I think this needs a param name, it's not obvious what it's for.
Thank you for the review! https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:30: { On 2015/07/10 11:20:44, falken wrote: > I suspect a more user-friendly way to present this to developers is something > like: > > "The FetchEvent for \"<requestURL.string>\" resulted in a network error > response: " > > and then each case outputs a reason. Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:34: errorMessage = "An unexpected error occured while handling the FetchEvent (" + requestURL.string() + ")."; On 2015/07/10 11:20:44, falken wrote: > "an unexpected error occurred." > (this should probably also be done in the default case) Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:36: case WebServiceWorkerResponseErrorPromiseRejected: On 2015/07/10 11:20:44, falken wrote: > Can you comment why this doesn't have a message (I believe it's because the > rejected promise is already displayed to console). Humm. It may better to show more detailed message from here. Added new message for it. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:40: errorMessage = "event.preventDefault() for the FetchEvent (" + requestURL.string() + ") without calling event.respondWith() is treated as a network error."; On 2015/07/10 11:20:44, falken wrote: > "preventDefault() was called without calling respondWith()." Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:43: errorMessage = "The object with which respond to the request (" + requestURL.string() + ") must be a Response object."; On 2015/07/10 11:20:44, falken wrote: > "an object that was not a Response was passed to respondWith()" Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:45: case WebServiceWorkerResponseErrorResponseTypeError: On 2015/07/10 11:20:44, falken wrote: > ditto, comment Added. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:49: errorMessage = "Opaque type response can't respond to the request (" + requestURL.string() + ") which type isn't no-cors."; On 2015/07/10 11:20:44, falken wrote: > "an opaque response was used for a request whose type is not no-cors" (yay > triple negatives) Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:52: errorMessage = "The type of the Response object with which respond to the client request (" + requestURL.string() + ") must be basic or default."; On 2015/07/10 11:20:44, falken wrote: > "the response for a client request must have type \"basic\" or \"default\"" (are > these types exposed to script? if not we shouldn't quote them) Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:55: errorMessage = "The body used response can't respond to the request (" + requestURL.string() + ")."; On 2015/07/10 11:20:44, falken wrote: > "a Response whose \"bodyUsed\" is \"true\" cannot be used to respond to a > request" Done. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:58: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage)); On 2015/07/10 11:20:44, falken wrote: > I suspect this should be a Warning or Info, it's conceivable the developer is > intentionally doing this. +mkwst Changed to Info. https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.h (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.h:26: static RespondWithObserver* create(ExecutionContext*, int eventID, const KURL&, WebURLRequest::FetchRequestMode, WebURLRequest::FrameType); On 2015/07/10 11:20:44, falken wrote: > I think this needs a param name, it's not obvious what it's for. Done.
The strings look good! I'm a bit concerned about spamming the console however, see below: https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:58: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage)); On 2015/07/10 at 12:13:26, horo wrote: > On 2015/07/10 11:20:44, falken wrote: > > I suspect this should be a Warning or Info, it's conceivable the developer is > > intentionally doing this. +mkwst > > Changed to Info. The "intentionally doing this" worries me a bit; we generally try to keep the console clean unless there's a real error that someone needs to deal with. If there are good reasons that a developer might be producing these kinds of scenarios (feature-detecting something, etc), it would be better to wrap the error up into something the developer could inspect. We might throw an exception, for instance, which could be caught and suppressed. It doesn't look like `respondWith` returns anything useful in the case of an error (which means it's a great idea to throw some messages somewhere!). Have you considered altering it to either throw in the case of an irrecoverable error, or to return a promise which would be rejected with one of the messages you've put together here in the case of error?
CC pfeldman: you might be interested in console message/DevTools discussion https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:58: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage)); On 2015/07/10 13:01:09, Mike West wrote: > On 2015/07/10 at 12:13:26, horo wrote: > > On 2015/07/10 11:20:44, falken wrote: > > > I suspect this should be a Warning or Info, it's conceivable the developer > is > > > intentionally doing this. +mkwst > > > > Changed to Info. > > The "intentionally doing this" worries me a bit; we generally try to keep the > console clean unless there's a real error that someone needs to deal with. If > there are good reasons that a developer might be producing these kinds of > scenarios (feature-detecting something, etc), it would be better to wrap the > error up into something the developer could inspect. We might throw an > exception, for instance, which could be caught and suppressed. > > It doesn't look like `respondWith` returns anything useful in the case of an > error (which means it's a great idea to throw some messages somewhere!). Have > you considered altering it to either throw in the case of an irrecoverable > error, or to return a promise which would be rejected with one of the messages > you've put together here in the case of error? +pfeldman FYI I don't have a very compelling use case in mind. The only thing I could think was using Service Worker as a kind of network simulation/HTTP proxy server that intentionally creates network errors. I mean the spec does allow the developer to use "preventDefault()" to create a network error. What might be better long term is to expose detailed SW network activity in DevTools Network tab. I'd say for now these console messages are OK, as currently these errors are pretty much impossible to debug. I'll also bring up changing the return value of respondWith() in a spec issue.
https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/40001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:58: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, errorMessage)); On 2015/07/13 00:51:36, falken wrote: > On 2015/07/10 13:01:09, Mike West wrote: > > On 2015/07/10 at 12:13:26, horo wrote: > > > On 2015/07/10 11:20:44, falken wrote: > > > > I suspect this should be a Warning or Info, it's conceivable the developer > > is > > > > intentionally doing this. +mkwst > > > > > > Changed to Info. > > > > The "intentionally doing this" worries me a bit; we generally try to keep the > > console clean unless there's a real error that someone needs to deal with. If > > there are good reasons that a developer might be producing these kinds of > > scenarios (feature-detecting something, etc), it would be better to wrap the > > error up into something the developer could inspect. We might throw an > > exception, for instance, which could be caught and suppressed. > > > > It doesn't look like `respondWith` returns anything useful in the case of an > > error (which means it's a great idea to throw some messages somewhere!). Have > > you considered altering it to either throw in the case of an irrecoverable > > error, or to return a promise which would be rejected with one of the messages > > you've put together here in the case of error? > > +pfeldman FYI > > I don't have a very compelling use case in mind. The only thing I could think > was using Service Worker as a kind of network simulation/HTTP proxy server that > intentionally creates network errors. I mean the spec does allow the developer > to use "preventDefault()" to create a network error. > > What might be better long term is to expose detailed SW network activity in > DevTools Network tab. I'd say for now these console messages are OK, as > currently these errors are pretty much impossible to debug. > > I'll also bring up changing the return value of respondWith() in a spec issue. Ah.. just realized respondWith()'s return value won't help in the case of preventDefault(), since it's never called. It was considered in a similar issue https://github.com/slightlyoff/ServiceWorker/issues/150 that "we may want to indicate something specific in devtools console / network panel".
this lgtm, longer term we probably want this in DevTools Network tab or somewhere rather than a console message but it's good for now. https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:29: void maybeOutputErrorMessage(ExecutionContext* context, WebServiceWorkerResponseError error, const KURL& requestURL) Shouldn't be "maybe" anymore since it always outputs? Might be cleaner now for this to just be "getMessageForResponseError()", and have responseWasRejected actually call addConsoleMessage(). https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:59: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, InfoMessageLevel, errorMessage)); on second thought Warning is probably more appropriate so they see warning icon in DevTools (kind of just guessing though). https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.h (right): https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.h:44: RespondWithObserver(ExecutionContext*, int eventID, const KURL&, WebURLRequest::FetchRequestMode, WebURLRequest::FrameType); probably should write requestURL here too
Looks fine by me - I don't think it generates too much noise.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:29: void maybeOutputErrorMessage(ExecutionContext* context, WebServiceWorkerResponseError error, const KURL& requestURL) On 2015/07/14 12:58:55, falken wrote: > Shouldn't be "maybe" anymore since it always outputs? Might be cleaner now for > this to just be "getMessageForResponseError()", and have responseWasRejected > actually call addConsoleMessage(). Done. https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.cpp:59: context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, InfoMessageLevel, errorMessage)); On 2015/07/14 12:58:55, falken wrote: > on second thought Warning is probably more appropriate so they see warning icon > in DevTools (kind of just guessing though). Done. https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... File Source/modules/serviceworkers/RespondWithObserver.h (right): https://codereview.chromium.org/1228233007/diff/60001/Source/modules/servicew... Source/modules/serviceworkers/RespondWithObserver.h:44: RespondWithObserver(ExecutionContext*, int eventID, const KURL&, WebURLRequest::FetchRequestMode, WebURLRequest::FrameType); On 2015/07/14 12:58:55, falken wrote: > probably should write requestURL here too Done.
mkwst@ As same as falken@, I don't think these messages will be shown in normal use case. Could you please give l-g-t-m?
On 2015/07/21 at 03:06:42, horo wrote: > mkwst@ > As same as falken@, I don't think these messages will be shown in normal use case. > Could you please give l-g-t-m? LGTM. *shrug* Let's see how it goes.
On 2015/07/22 06:05:49, Mike West wrote: > On 2015/07/21 at 03:06:42, horo wrote: > > mkwst@ > > As same as falken@, I don't think these messages will be shown in normal use > case. > > Could you please give l-g-t-m? > > LGTM. *shrug* Let's see how it goes. Thank you.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/1228233007/#ps100001 (title: "incorporated falken's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1228233007/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199287
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/1252443003/ by yhirano@chromium.org. The reason for reverting is: Causes content_browsertests failures: http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/45600. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
