Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22518002:
Incremental changes in error reporting (Closed)
|
Created:
7 years, 4 months ago by vadimt Modified:
7 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIncremental changes in error reporting.
Unifying error reporting.
Now sending all errors to the server, including ones that should be detected early by developer (because sometimes this doesn't happen).
Supporting all formats of frames in error stacks (now supporting anonymous top frame).
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217287
Patch Set 1 #Patch Set 2 : Cosmetics #
Total comments: 1
Patch Set 3 : Better telling runtime errors from non-runtime ones. #
Total comments: 13
Patch Set 4 : More notes #Patch Set 5 : Cosmetics #
Total comments: 4
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/22518002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:187: var topFrameLineMatch = filteredStack.match(/\n at .*\n/); seems like a good place for a test :D
Newer version.
https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { buildNonRuntimeError? buildCustomError? This is more than just an error, so this should be more than buildError. If you have something better than my naming, good. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:217: errorLocation = topFrame; Have you seen this happen? I don't know if attempting to set file and line are worth it in this case. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:226: file = topFrameElements[topFrameElements.length - 3]; Seems weird to just drop the chrome-extension.
https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { On 2013/08/08 20:14:01, rgustafson wrote: > buildNonRuntimeError? buildCustomError? This is more than just an error, so this > should be more than buildError. If you have something better than my naming, > good. You cannot build a runtime error. And 'custom' is overloaded. Manual? Built? Probably, we can leave it as is. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:217: errorLocation = topFrame; On 2013/08/08 20:14:01, rgustafson wrote: > Have you seen this happen? I don't know if attempting to set file and line are > worth it in this case. Sure, this example (2.) with anonymous top frame. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:226: file = topFrameElements[topFrameElements.length - 3]; On 2013/08/08 20:14:01, rgustafson wrote: > Seems weird to just drop the chrome-extension. That's not a big deal, but this way, we support all 4 kinds of a frame. Probably, not worth added complexity to add chrome-extension://
https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { On 2013/08/08 20:25:28, vadimt wrote: > On 2013/08/08 20:14:01, rgustafson wrote: > > buildNonRuntimeError? buildCustomError? This is more than just an error, so > this > > should be more than buildError. If you have something better than my naming, > > good. > > You cannot build a runtime error. And 'custom' is overloaded. Manual? Built? > Probably, we can leave it as is. agree it might be nice to differentiate somehow; 'buildError' doesn't seem to indicate the side effect that the notFromRuntime expando will have. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:195: var filteredStack = error.notFromRuntime ? comment here could help future readers.
https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { On 2013/08/09 23:02:18, Travis Skare wrote: > On 2013/08/08 20:25:28, vadimt wrote: > > On 2013/08/08 20:14:01, rgustafson wrote: > > > buildNonRuntimeError? buildCustomError? This is more than just an error, so > > this > > > should be more than buildError. If you have something better than my naming, > > > good. > > > > You cannot build a runtime error. And 'custom' is overloaded. Manual? Built? > > Probably, we can leave it as is. > > agree it might be nice to differentiate somehow; 'buildError' doesn't seem to > indicate the side effect that the notFromRuntime expando will have. Yeah, I still think this needs a modifier. I don't think build has the wide meaning that you think it does because I don't think that differentiates enough. Also, now that I'm looking at it again, the run time error terminology is a bit confusing too. afaik technically everything that isn't a parse error is a run time error in js.. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:226: file = topFrameElements[topFrameElements.length - 3]; On 2013/08/08 20:25:28, vadimt wrote: > On 2013/08/08 20:14:01, rgustafson wrote: > > Seems weird to just drop the chrome-extension. > > That's not a big deal, but this way, we support all 4 kinds of a frame. > Probably, not worth added complexity to add chrome-extension:// Okay, that's fine.
https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { On 2013/08/09 23:08:22, rgustafson wrote: > On 2013/08/09 23:02:18, Travis Skare wrote: > > On 2013/08/08 20:25:28, vadimt wrote: > > > On 2013/08/08 20:14:01, rgustafson wrote: > > > > buildNonRuntimeError? buildCustomError? This is more than just an error, > so > > > this > > > > should be more than buildError. If you have something better than my > naming, > > > > good. > > > > > > You cannot build a runtime error. And 'custom' is overloaded. Manual? Built? > > > Probably, we can leave it as is. > > > > agree it might be nice to differentiate somehow; 'buildError' doesn't seem to > > indicate the side effect that the notFromRuntime expando will have. > > Yeah, I still think this needs a modifier. I don't think build has the wide > meaning that you think it does because I don't think that differentiates enough. > Also, now that I'm looking at it again, the run time error terminology is a bit > confusing too. afaik technically everything that isn't a parse error is a run > time error in js.. Done. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:27: function buildError(message) { On 2013/08/09 23:02:18, Travis Skare wrote: > On 2013/08/08 20:25:28, vadimt wrote: > > On 2013/08/08 20:14:01, rgustafson wrote: > > > buildNonRuntimeError? buildCustomError? This is more than just an error, so > > this > > > should be more than buildError. If you have something better than my naming, > > > good. > > > > You cannot build a runtime error. And 'custom' is overloaded. Manual? Built? > > Probably, we can leave it as is. > > agree it might be nice to differentiate somehow; 'buildError' doesn't seem to > indicate the side effect that the notFromRuntime expando will have. Done. https://codereview.chromium.org/22518002/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:195: var filteredStack = error.notFromRuntime ? On 2013/08/09 23:02:18, Travis Skare wrote: > comment here could help future readers. New name is self-explaining.
https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:260: if (isEnabled) just checking, isEnabled == false and DEBUG_MODE == true is an expected-possible case, correct?
lgtm
https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:260: if (isEnabled) On 2013/08/12 20:47:14, Travis Skare wrote: > just checking, isEnabled == false and DEBUG_MODE == true is an expected-possible > case, correct? Correct.
lgtm
arv@, please provide OWNER's approval
LGTM https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:256: console.error(message); console.error(error) also prints the stack. If you want to add extra info you can do: console.error('Critical error', error)
https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22518002/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:256: console.error(message); On 2013/08/12 21:31:30, arv wrote: > console.error(error) > > also prints the stack. If you want to add extra info you can do: > > console.error('Critical error', error) > Couple of reasons why this would be not ideal: 1. console.error('Critical error', error) puts 'error' as an expandable object in the output. In some cases, you cannot see the whole object unless you click at it to expand. This is not good when, say, a tester copy-pastes console output into the bug. 2. var message is used below for the alert, so we'd have code duplication.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/22518002/20001
Retried try job too often on linux_rel for step(s) check_deps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/22518002/20001
Retried try job too often on linux_rel for step(s) check_deps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/22518002/20001
Message was sent while issue was closed.
Change committed as 217287 |
