Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18822004:
Extension Error Piping - Blink: WebKit Side (Closed)
|
Created:
7 years, 5 months ago by Devlin Modified:
7 years, 4 months ago CC:
blink-reviews, jamesr, eae+blinkwatch, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionPipe detailed information (including stack trace) about runtime JS errors caused by extensions from the WebKit side over to the Chrome side.
Goes with: https://codereview.chromium.org/18799006/
See Also: https://docs.google.com/a/google.com/document/d/1moGXfxo6BbulkoCWISlKq2aQR_buVYGeGhDj6_bqMXk/edit
BUG=21734
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156645
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Consolidate [did]AddMessageToConsole() and reportDetailedMessage() #
Total comments: 5
Patch Set 3 : Use v8 Contexts #
Total comments: 1
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Pass in plain text, per Pavel's suggestion #
Total comments: 5
Patch Set 6 : Extracted Formatting #
Total comments: 9
Patch Set 7 : Pavel's comments addressed #Patch Set 8 : #
Total comments: 5
Patch Set 9 : #
Total comments: 6
Patch Set 10 : Adams nits #Patch Set 11 : Latest master for cq #Patch Set 12 : #Patch Set 13 : #
Messages
Total messages: 40 (0 generated)
Yoyo and Matt - Could you take a look at this, and let me know if it looks like a good route to take for tapping into JS errors? (1/2)
This makes sense to me in general. I'm not a WebKit expert, so we'll have to find someone on WebKit to review it. What's the difference between Console and PageConsole? Why are there 2 places to add a console message? https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... File Source/WebKit/chromium/src/ChromeClientImpl.h (right): https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/ChromeClientImpl.h:104: const WTF::String& jsonStackTrace); I think it would be better to extend addMessageToConsole to include the optional stack trace.
I'm not quite sure what the differences between PageConsole and Console are. Best I can figure it, PageConsole reflects any errors directly related to the DOM, whereas Console is everything else. This is a SWAG based on the fact that a JS error in a content script is directed to PageConsole, whereas a JS error run in a background page is directed to Console (as is a console.log(), independent of the script location). In either case, we need both places, and I _think_ they might be the only two that will generate console errors. https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... File Source/WebKit/chromium/src/ChromeClientImpl.h (right): https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/ChromeClientImpl.h:104: const WTF::String& jsonStackTrace); On 2013/07/10 23:32:07, Matt Perry wrote: > I think it would be better to extend addMessageToConsole to include the optional > stack trace. Yeah, how much to merge addMessageToConsole() (and related) was my biggest question. Should we still keep didAddMessageToConsole() and reportDetailedMessage() separate (and just merge in addMessageToConsole()), d'you think?
https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... File Source/WebKit/chromium/src/ChromeClientImpl.h (right): https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... Source/WebKit/chromium/src/ChromeClientImpl.h:104: const WTF::String& jsonStackTrace); On 2013/07/11 01:15:44, D Cronin wrote: > On 2013/07/10 23:32:07, Matt Perry wrote: > > I think it would be better to extend addMessageToConsole to include the > optional > > stack trace. > > Yeah, how much to merge addMessageToConsole() (and related) was my biggest > question. Should we still keep didAddMessageToConsole() and > reportDetailedMessage() separate (and just merge in addMessageToConsole()), > d'you think? Yeah, I'd combine them all. This doesn't feel like a separate event - it's just extra data on the addMessageToConsole event.
On 2013/07/11 02:40:46, Matt Perry wrote: > https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... > File Source/WebKit/chromium/src/ChromeClientImpl.h (right): > > https://codereview.chromium.org/18822004/diff/8001/Source/WebKit/chromium/src... > Source/WebKit/chromium/src/ChromeClientImpl.h:104: const WTF::String& > jsonStackTrace); > On 2013/07/11 01:15:44, D Cronin wrote: > > On 2013/07/10 23:32:07, Matt Perry wrote: > > > I think it would be better to extend addMessageToConsole to include the > > optional > > > stack trace. > > > > Yeah, how much to merge addMessageToConsole() (and related) was my biggest > > question. Should we still keep didAddMessageToConsole() and > > reportDetailedMessage() separate (and just merge in addMessageToConsole()), > > d'you think? > > Yeah, I'd combine them all. This doesn't feel like a separate event - it's just > extra data on the addMessageToConsole event. Done.
+pfeldman for WebKit
lgtm
https://codereview.chromium.org/18822004/diff/5733953138851840/Source/WebKit/... File Source/WebKit/chromium/src/ChromeClientImpl.cpp (right): https://codereview.chromium.org/18822004/diff/5733953138851840/Source/WebKit/... Source/WebKit/chromium/src/ChromeClientImpl.cpp:379: return m_webView->client() && m_webView->client()->shouldReportDetailedMessage(source); You should decide based on the context. https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... Source/core/page/PageConsole.cpp:97: stackTrace = callStack->buildInspectorArray()->toJSONString(); You should instead format stack trace here, toJSONString that you are using might change its structure at any time, so you want to handle stack -> text conversion here (or come up with yet another class describing call stack in web API).
Thanks for the review =) https://codereview.chromium.org/18822004/diff/5733953138851840/Source/WebKit/... File Source/WebKit/chromium/src/ChromeClientImpl.cpp (right): https://codereview.chromium.org/18822004/diff/5733953138851840/Source/WebKit/... Source/WebKit/chromium/src/ChromeClientImpl.cpp:379: return m_webView->client() && m_webView->client()->shouldReportDetailedMessage(source); On 2013/07/17 09:09:38, pfeldman wrote: > You should decide based on the context. Deciding based on v8::Context, rather than URL, is good when it's possible. Unfortunately, sometimes we can't pass the context. This can happen in the case of a content script causing an error - the ScriptState* (from which we would get the V8 Context) passed in to PageConsole::addMessage() is NULL. In these cases, examining the URL can inform us whether or not the error came from an extension. That being said, if we use both the context and the URL, we are able restrict ourselves to chrome-extension:// urls, which addresses the concern you had with the method. https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... Source/core/page/PageConsole.cpp:97: stackTrace = callStack->buildInspectorArray()->toJSONString(); On 2013/07/17 09:09:38, pfeldman wrote: > You should instead format stack trace here, toJSONString that you are using > might change its structure at any time, so you want to handle stack -> text > conversion here (or come up with yet another class describing call stack in web > API). Done. https://codereview.chromium.org/18822004/diff/29001/Source/core/page/Chrome.cpp File Source/core/page/Chrome.cpp (right): https://codereview.chromium.org/18822004/diff/29001/Source/core/page/Chrome.c... Source/core/page/Chrome.cpp:446: static const char kLineNumberKey[] = "lineNumber"; I apologize in advance if the style is bad - I tried to followed http://www.webkit.org/coding/coding-style.html, but it doesn't address certain things (like static data and naming constants). Please let me know if there's anything I need to fix.
https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/5733953138851840/Source/core/pa... Source/core/page/PageConsole.cpp:97: stackTrace = callStack->buildInspectorArray()->toJSONString(); On 2013/07/17 09:09:38, pfeldman wrote: > You should instead format stack trace here, toJSONString that you are using > might change its structure at any time, so you want to handle stack -> text > conversion here (or come up with yet another class describing call stack in web > API). I'm starting to think that we should add a callstack class. Passing structured data in a string, and expecting both sides to agree on the format of the string, seems a bit strange. OTOH, it's a lot of plumbing to ferry across a new structured datatype (you'd need to transfer types at each boundary of WebCore->WebKit->browser).
https://codereview.chromium.org/18822004/diff/35001/Source/core/page/Chrome.cpp File Source/core/page/Chrome.cpp (right): https://codereview.chromium.org/18822004/diff/35001/Source/core/page/Chrome.c... Source/core/page/Chrome.cpp:462: String Chrome::generateConsoleMessageDetails(size_t lineNumber, size_t columnNumber, const String& sourceURL, const String& functionName, const String& executionContextURL) This isn't the right file for this code. Chrome.cpp has nothing to do with "Chrome" the browser.
I would like to understand better what you're trying to accomplish with this CL. I think we should back up and talk about the underlying problem you're trying to solve and then think through how best to solve it. I'm going to mark this CL not lgtm because it's going to require some more structural work. https://codereview.chromium.org/18822004/diff/35001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/18822004/diff/35001/public/web/WebViewClient.... public/web/WebViewClient.h:48: #include <v8.h> Hum... This is suspicious. We probably don't want to introduce V8 concepts into this interface.
On 2013/07/19 03:12:17, abarth wrote: > I would like to understand better what you're trying to accomplish with this CL. > I think we should back up and talk about the underlying problem you're trying > to solve and then think through how best to solve it. > > I'm going to mark this CL not lgtm because it's going to require some more > structural work. > > https://codereview.chromium.org/18822004/diff/35001/public/web/WebViewClient.h > File public/web/WebViewClient.h (right): > > https://codereview.chromium.org/18822004/diff/35001/public/web/WebViewClient.... > public/web/WebViewClient.h:48: #include <v8.h> > Hum... This is suspicious. We probably don't want to introduce V8 concepts > into this interface. Sorry for not being more clear, Adam. I've updated the CLs' descriptions to be slightly more informative, and linked the basic design doc. At a high-level, there are two main problems. The first is that extension errors are reported in too many places. The second, and the one we try to solve here, is that javascript errors caused by extensions are lost - they are reported to the console, but no one is explicitly made aware that the extension is misbehaving. What we are trying to do is extract the information about an extension error that normally goes to the developer tools and alert the user by placing it in a more centric location (e.g., chrome:extensions). In order to do this, we need to extract the error information, which is reported to WebKit (since it's treated as just a JS error). So, we need to: 1. Receive a JS error in WebKit 2. Determine if we want to pipe the details over to Chrome (i.e., is the error from an extension) 3. Pipe the details over to Chrome 1 is obviously done, and 2 is implemented with shouldReportDetailedMessage() (and related). 3 is the tricky one, since we're trying to move away from the blind-trust serialization we have here (hence the more structured approach in https://codereview.chromium.org/19722012/). Any advice? If you have any other questions, feel free to reply, email, or swing by in-person to chat. =)
On 2013/07/19 16:27:55, D Cronin wrote: > If you have any other questions, feel free to reply, email, or swing by > in-person to chat. =) Have you considered using the inspector protocol? The inspector has the ability to send console messages to a remote process to support remote inspecting (e.g., using your desktop computer to inspect a web page running on your phone). That seems very similar to what you're trying to do here (effectively remotely inspect the running extension from your centralized error console). The protocol is JSON based. I believe the scheme is defined in https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/pro...
On 2013/07/19 18:29:20, abarth wrote: > On 2013/07/19 16:27:55, D Cronin wrote: > > If you have any other questions, feel free to reply, email, or swing by > > in-person to chat. =) > > Have you considered using the inspector protocol? The inspector has the ability > to send console messages to a remote process to support remote inspecting (e.g., > using your desktop computer to inspect a web page running on your phone). That > seems very similar to what you're trying to do here (effectively remotely > inspect the running extension from your centralized error console). > > The protocol is JSON based. I believe the scheme is defined in > https://chromium.googlesource.com/chromium/blink/+/master/Source/devtools/pro... My understanding was that anything using the inspector (including the protocol) would only be available if the inspector was active. While we could always fully inspect each extension, this seems like much more overhead than we would want constantly running. Without triggering the inspection ourselves, we would only be notified when the user happens to have the inspector open. Is this correct?
You can attach to any page (that is not currently inspected) and query console state there. However, stacks are only collected when inspector is attached. But you don't want them collected at all times anyways - that would lead to memory bloat in the background. I was wondering if showing the number of total errors and warnings in chrome://extensions + existing ability to open devtools from there would suffice. Users will get cached error messages + will have ability to reload extension with stack collecting enabled.
On 2013/07/19 18:40:39, D Cronin wrote: > Is this correct? That's possible, but maybe we could change that so that the console part of the inspector protocol worked for the extension case without having to boot up the whole inspector. I guess my point is that addressing your use case is going to push you to re-invent the inspector protocol one piece at a time. Given that we already have the inspector protocol, perhaps we should integrate with it rather than growing a redundant system for remoting the console.
On 2013/07/19 18:46:22, abarth wrote: > I guess my point is that addressing your use case is going to push you to > re-invent the inspector protocol one piece at a time. Given that we already > have the inspector protocol, perhaps we should integrate with it rather than > growing a redundant system for remoting the console. We've talked about it here a bit more, and I'm not convinced that going through the protocol is the best course of action. In particular, it actually seems significantly heavier than what we need, especially since then we'd be running the inspector (even if just a fragmented version of it) on every page at all times. We're not trying to get any more information than is already reported to WebKit under normal circumstances (e.g., we're not generating full stack traces with context at every frame, etc) - as pfeldman pointed out, this would be far too cumbersome. Really, all we want to do is take a bit more of the information already available on the WebKit side and ferry it along to Chrome, expanding an existing call. If we wanted to get a great deal more information out of the console message (i.e., if we wanted the full-blown dev-tools version), I'd feel differently - but this just doesn't strike me as enough to add another layer to use the inspector. If you still think the inspector protocol's the way to go, is there a time on Monday we could chat in-person for a few minutes about how that might be done?
It seems like we could reuse some of the inspector's mechanisms without booting up a fully instance of the inspector. This approach is likely require some refactoring of existing code. I don't think we should glue on another JSON-based protocol for remoting console messages to another process. Feel free to schedule some time on calendar on Monday if you'd like to discuss verbally rather than via code review.
https://chromiumcodereview.appspot.com/18822004/diff/35001/Source/core/page/C... File Source/core/page/Chrome.cpp (right): https://chromiumcodereview.appspot.com/18822004/diff/35001/Source/core/page/C... Source/core/page/Chrome.cpp:465: stackFrame->setNumber(kLineNumberKey, lineNumber); Why another format? You should either pass stack traces as text (and linkify them later in your code) or introduce StackTrace structures in WebKit API. I'd vote for the plain text as a start. I.e. early version of your change with text instead of JSON.
On 2013/08/07 13:30:21, pfeldman wrote: > https://chromiumcodereview.appspot.com/18822004/diff/35001/Source/core/page/C... > File Source/core/page/Chrome.cpp (right): > > https://chromiumcodereview.appspot.com/18822004/diff/35001/Source/core/page/C... > Source/core/page/Chrome.cpp:465: stackFrame->setNumber(kLineNumberKey, > lineNumber); > Why another format? You should either pass stack traces as text (and linkify > them later in your code) or introduce StackTrace structures in WebKit API. I'd > vote for the plain text as a start. I.e. early version of your change with text > instead of JSON. Thanks for the reply :) > StackTrace structures in WebKit API This, I think, is too much work for what it is. It would require a StackTrace object in WebCore, then in WebKit, then at least one in the Chrome side. I think that, even though the conversions are fast, it's more work than it should be. So I'd agree that another solution would be better. > I'd vote for the plain text as a start. Just to confirm, by this you mean passing in a simple string which contains all the information and is later parsed on the Chrome side, so something like <file>,<line>,<function>;<file>,<line>,<function>;? Or were you thinking of something human-readable? ---- Adam earlier voiced his concerns that a method like this doesn't use the inspector protocol at all, which led to the more-recent CL at https://codereview.chromium.org/20191003/ . Is this older approach preferable to the one there, which tries to reuse the structures already in place?
> Just to confirm, by this you mean passing in a simple string which contains all > the information and is later parsed on the Chrome side, so something like > <file>,<line>,<function>;<file>,<line>,<function>;? Or were you thinking of > something human-readable? Human-readable. I.e. what "new Error().stack" returns if you paste it into console. You can linkify it in your UI later. > Adam earlier voiced his concerns that a method like this doesn't use the > inspector protocol at all, which led to the more-recent CL at > https://codereview.chromium.org/20191003/ . Is this older approach preferable > to the one there, which tries to reuse the structures already in place? I think Adam's concern was more of a "Can we reuse inspector infrastructure / protocol / types in order to avoid creating additional plumbing, types and formats". Unfortunately, inspector protocol is not of help here - you want inspector to co-exist with your error reporting. But on a positive note, I don't think you need anything other than formatted (human-readable) stack trace.
On 2013/08/07 21:12:35, pfeldman wrote: > > Just to confirm, by this you mean passing in a simple string which contains > all > > the information and is later parsed on the Chrome side, so something like > > <file>,<line>,<function>;<file>,<line>,<function>;? Or were you thinking of > > something human-readable? > > Human-readable. I.e. what "new Error().stack" returns if you paste it into > console. You can linkify it in your UI later. > > > Adam earlier voiced his concerns that a method like this doesn't use the > > inspector protocol at all, which led to the more-recent CL at > > https://codereview.chromium.org/20191003/ . Is this older approach preferable > > to the one there, which tries to reuse the structures already in place? > > I think Adam's concern was more of a "Can we reuse inspector infrastructure / > protocol / types in order to avoid creating additional plumbing, types and > formats". Unfortunately, inspector protocol is not of help here - you want > inspector to co-exist with your error reporting. But on a positive note, I don't > think you need anything other than formatted (human-readable) stack trace. Pavel -- Uploaded new patch set using just plaintext. Please take a look :)
https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.cpp File Source/core/page/Console.cpp (right): https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.... Source/core/page/Console.cpp:78: if (page->chrome().client()->shouldReportDetailedMessageForSource(callStack->at(0).sourceURL())) { Is it true that you are now collecting stack traces for all evals (empty URLs) twice unconditionally?
https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.cpp File Source/core/page/Console.cpp (right): https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.... Source/core/page/Console.cpp:78: if (page->chrome().client()->shouldReportDetailedMessageForSource(callStack->at(0).sourceURL())) { On 2013/08/08 17:13:52, pfeldman wrote: > Is it true that you are now collecting stack traces for all evals (empty URLs) > twice unconditionally? Short answer: Nope. Sorry, I didn't upload the corresponding chrome-side update - I was more worried about nailing down the fundamentals of the Blink half, since we've gone through so many iterations. shouldReportDetailedMessageForSource() now takes in a URL and returns true if and only if the url matches the extension scheme (i.e., anything with chrome-extension://). We get the stack trace of one frame in order to find the last caller (which we need in either case) to check this URL, and only if it's an extension URL do we go back and get the rest of the trace. As for evals, experimenting with it a bit has shown that it's not really viable to check for errors with them. The ScriptState (and as such, the context) is inaccessible in the case of a crippling error - the kind we're really interested in. So even though we could potentially report things like console.log, it's not useful enough to warrant the added complexity (since then you could also just to console.log(new Error().stack)). For things other than evals (like event/background pages, content scripts, browser actions/event callbacks, etc), we can determine whether or not it's an extension through the URL alone. Also, this addresses Adam's concern about exposing v8 to the WebViewClient interface.
Please extract the formatter and it is good to go! https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.cpp File Source/core/page/Console.cpp (right): https://codereview.chromium.org/18822004/diff/54001/Source/core/page/Console.... Source/core/page/Console.cpp:78: if (page->chrome().client()->shouldReportDetailedMessageForSource(callStack->at(0).sourceURL())) { On 2013/08/12 16:09:41, D Cronin wrote: > On 2013/08/08 17:13:52, pfeldman wrote: > > Is it true that you are now collecting stack traces for all evals (empty URLs) > > twice unconditionally? > > Short answer: Nope. > Ok! https://codereview.chromium.org/18822004/diff/54001/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/54001/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:97: for (size_t i = 0; i < callStack->size(); ++i) { Can you extract this formatter and use it from Console.cpp?
https://codereview.chromium.org/18822004/diff/54001/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/54001/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:97: for (size_t i = 0; i < callStack->size(); ++i) { On 2013/08/12 16:15:34, pfeldman wrote: > Can you extract this formatter and use it from Console.cpp? Done.
https://codereview.chromium.org/18822004/diff/66001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/core.gypi#new... Source/core/core.gypi:1164: 'page/FormatConsolemessage.h', Broken letter case. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... File Source/core/page/FormatConsoleMessage.cpp (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.cpp:44: return message.find("\n at ") != WTF::notFound; This looks like a very fragile heuristic. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.cpp:61: String formatConsoleMessage(const WTF::String& originalMessage, const WTF::String& url, unsigned lineNumber, unsigned columnNumber) Console message already has url, line number and column plumbed. Adding empty stack based on this information looks redundant. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... File Source/core/page/FormatConsoleMessage.h (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.h:43: WTF::String formatConsoleMessage(const WTF::String& originalMessage, PassRefPtr<ScriptCallStack>); It was fine to make it static on PageConsole. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:102: page->chrome().client()->addMessageToConsole(source, level, chromeMessage, lineNumber, url); Wait, I thought we agreed on an additional text field with the stack trace. Sorry for overseeing it at the first place.
https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... File Source/core/page/FormatConsoleMessage.cpp (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.cpp:44: return message.find("\n at ") != WTF::notFound; On 2013/08/14 09:53:00, pfeldman wrote: > This looks like a very fragile heuristic. It is fragile, but there's not really a solution (we have no knowledge of whether or not we tacked on a stack trace, since it's just part of the message string in the js implementation when we catch the error). But if we have the stack trace as a separate string, we'll be sending both stack traces along anyway, so we don't really need this. Removed. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.cpp:61: String formatConsoleMessage(const WTF::String& originalMessage, const WTF::String& url, unsigned lineNumber, unsigned columnNumber) On 2013/08/14 09:53:00, pfeldman wrote: > Console message already has url, line number and column plumbed. Adding empty > stack based on this information looks redundant. Mostly it was for completeness purposes - this way, we would know that we report some extra details if more details are requested. Also, ConsoleMessage doesn't have column - just line. But, assuming no one's using this on minified code (and they'd be crazy to do so, anyway), column's not really critical most of the time. Removed this variant. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... File Source/core/page/FormatConsoleMessage.h (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/FormatCo... Source/core/page/FormatConsoleMessage.h:43: WTF::String formatConsoleMessage(const WTF::String& originalMessage, PassRefPtr<ScriptCallStack>); On 2013/08/14 09:53:00, pfeldman wrote: > It was fine to make it static on PageConsole. Okay. Moved there. https://codereview.chromium.org/18822004/diff/66001/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/66001/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:102: page->chrome().client()->addMessageToConsole(source, level, chromeMessage, lineNumber, url); On 2013/08/14 09:53:00, pfeldman wrote: > Wait, I thought we agreed on an additional text field with the stack trace. > Sorry for overseeing it at the first place. Okay. Wasn't sure if plain-text should go with plain-text or not, and opted for the one that changed fewer interfaces. But having it as two arguments is good, too. Done.
lgtm https://codereview.chromium.org/18822004/diff/79001/Source/core/page/ChromeCl... File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/18822004/diff/79001/Source/core/page/ChromeCl... Source/core/page/ChromeClient.h:128: virtual bool shouldReportDetailedMessageForSource(const String&) = 0; Parameter name would not hurt. https://codereview.chromium.org/18822004/diff/79001/Source/core/page/PageCons... File Source/core/page/PageConsole.h (right): https://codereview.chromium.org/18822004/diff/79001/Source/core/page/PageCons... Source/core/page/PageConsole.h:53: static String getStackTraceString(const String& originalMessage, PassRefPtr<ScriptCallStack>); nit: formatStackTraceString https://codereview.chromium.org/18822004/diff/79001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/18822004/diff/79001/public/web/WebViewClient.... public/web/WebViewClient.h:132: virtual void didAddMessageToConsole( You'll need coordinated landing for this.
Adam, could you take another look, please? https://codereview.chromium.org/18822004/diff/79001/Source/core/page/ChromeCl... File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/18822004/diff/79001/Source/core/page/ChromeCl... Source/core/page/ChromeClient.h:128: virtual bool shouldReportDetailedMessageForSource(const String&) = 0; On 2013/08/15 18:36:23, pfeldman wrote: > Parameter name would not hurt. Done. https://codereview.chromium.org/18822004/diff/79001/Source/core/page/PageCons... File Source/core/page/PageConsole.h (right): https://codereview.chromium.org/18822004/diff/79001/Source/core/page/PageCons... Source/core/page/PageConsole.h:53: static String getStackTraceString(const String& originalMessage, PassRefPtr<ScriptCallStack>); On 2013/08/15 18:36:23, pfeldman wrote: > nit: formatStackTraceString Done.
abarth - friendly ping?
LGTM modulo the nits below. Sorry for not seeing your earlier message. https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:101: // static Please remove these comments. We don't use them in Blink. https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:104: String stackTrace; Please don't ever call String::append. It's horribly slow. Instead, this should be: StringBuilder stackTrace; and the last line of the function should be: return stackTrace.toString(); https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:107: stackTrace.append("\n at " + (frame.functionName().length() ? frame.functionName() : String("(anonymous function)")) + " (" + frame.sourceURL() + ":" + String::number(frame.lineNumber()) + ":" + String::number(frame.columnNumber()) + ")"); Rather than creating all these temporary strings, please just call StringBuilder::append a bunch of times. As written, this code is very inefficient.
https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... File Source/core/page/PageConsole.cpp (right): https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:101: // static On 2013/08/19 19:54:18, abarth wrote: > Please remove these comments. We don't use them in Blink. Done. https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:104: String stackTrace; On 2013/08/19 19:54:18, abarth wrote: > Please don't ever call String::append. It's horribly slow. Instead, this > should be: > > StringBuilder stackTrace; > > and the last line of the function should be: > > return stackTrace.toString(); Done. https://codereview.chromium.org/18822004/diff/85002/Source/core/page/PageCons... Source/core/page/PageConsole.cpp:107: stackTrace.append("\n at " + (frame.functionName().length() ? frame.functionName() : String("(anonymous function)")) + " (" + frame.sourceURL() + ":" + String::number(frame.lineNumber()) + ":" + String::number(frame.columnNumber()) + ")"); On 2013/08/19 19:54:18, abarth wrote: > Rather than creating all these temporary strings, please just call > StringBuilder::append a bunch of times. As written, this code is very > inefficient. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/18822004/1...
Failed to apply patch for Source/core/page/PageConsole.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/page/PageConsole.h
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file
Source/core/page/PageConsole.h.rej
Patch: Source/core/page/PageConsole.h
Index: Source/core/page/PageConsole.h
diff --git a/Source/core/page/PageConsole.h b/Source/core/page/PageConsole.h
index
622ecf16b96384614913831df55b6d98bee1bf12..7bb8d6425e3edc3426d37294c079af486c108bb5
100644
--- a/Source/core/page/PageConsole.h
+++ b/Source/core/page/PageConsole.h
@@ -50,6 +50,7 @@ public:
void addMessage(MessageSource, MessageLevel, const String& message, const
String& sourceURL, unsigned lineNumber, unsigned columnNumber = 0,
PassRefPtr<ScriptCallStack> = 0, ScriptState* = 0, unsigned long
requestIdentifier = 0);
void addMessage(MessageSource, MessageLevel, const String& message,
PassRefPtr<ScriptCallStack>);
void addMessage(MessageSource, MessageLevel, const String& message,
unsigned long requestIdentifier = 0, Document* = 0);
+ static String formatStackTraceString(const String& originalMessage,
PassRefPtr<ScriptCallStack>);
static void mute();
static void unmute();
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/18822004/7...
Failed to apply patch for Source/core/page/PageConsole.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/page/PageConsole.h
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file
Source/core/page/PageConsole.h.rej
Patch: Source/core/page/PageConsole.h
Index: Source/core/page/PageConsole.h
diff --git a/Source/core/page/PageConsole.h b/Source/core/page/PageConsole.h
index
622ecf16b96384614913831df55b6d98bee1bf12..7bb8d6425e3edc3426d37294c079af486c108bb5
100644
--- a/Source/core/page/PageConsole.h
+++ b/Source/core/page/PageConsole.h
@@ -50,6 +50,7 @@ public:
void addMessage(MessageSource, MessageLevel, const String& message, const
String& sourceURL, unsigned lineNumber, unsigned columnNumber = 0,
PassRefPtr<ScriptCallStack> = 0, ScriptState* = 0, unsigned long
requestIdentifier = 0);
void addMessage(MessageSource, MessageLevel, const String& message,
PassRefPtr<ScriptCallStack>);
void addMessage(MessageSource, MessageLevel, const String& message,
unsigned long requestIdentifier = 0, Document* = 0);
+ static String formatStackTraceString(const String& originalMessage,
PassRefPtr<ScriptCallStack>);
static void mute();
static void unmute();
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/18822004/1...
Message was sent while issue was closed.
Change committed as 156645 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
