Web Inspector: ConsoleMessage should include line and column number where possible

[Joseph Pecoraro]

Currently ConsoleMessage only includes line number information. However, if the original source code is reformatted in the frontend (e.g. pretty printed), then having just the original line number is not enough information to know where the original source code moved too. To know that the frontend would need to know both the line and column number in the original source.

ConsoleMessages should have a column number.

[Joseph Pecoraro]

Created attachment 198979 [details]
[PATCH] Add column number alongside line number

This patch:

  - adds column number (almost) everywhere where line number is available. (Follow up will come for exceptions)
  - exposes column to ChromeClient alongside line number
  - adds column to ConsoleMessages in the Inspector Protocol.
  - provides a column for console.* function calls (log, error, etc.)

Lets see how the bot does on other ports.

[WebKit Commit Bot]

Attachment 198979 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/inspector/console/console-url-and-line-expected.txt', u'LayoutTests/inspector/console/console-url-and-line.html', u'LayoutTests/inspector/console/console-url-line-column-expected.txt', u'LayoutTests/inspector/console/console-url-line-column.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptExecutionContext.h', u'Source/WebCore/inspector/ConsoleMessage.cpp', u'Source/WebCore/inspector/ConsoleMessage.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorConsoleAgent.cpp', u'Source/WebCore/inspector/InspectorConsoleAgent.h', u'Source/WebCore/inspector/InspectorConsoleInstrumentation.h', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.h', u'Source/WebCore/inspector/InspectorProfilerAgent.cpp', u'Source/WebCore/inspector/InspectorProfilerAgent.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/ChromeClient.h', u'Source/WebCore/page/Console.cpp', u'Source/WebCore/page/ContentSecurityPolicy.cpp', u'Source/WebCore/page/PageConsole.cpp', u'Source/WebCore/page/PageConsole.h', u'Source/WebCore/workers/DefaultSharedWorkerRepository.cpp', u'Source/WebCore/workers/SharedWorkerContext.cpp', u'Source/WebCore/workers/WorkerContext.cpp', u'Source/WebCore/workers/WorkerContext.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerMessagingProxy.h', u'Source/WebCore/workers/WorkerReportingProxy.h', u'Source/WebCore/xml/XSLTProcessorLibxslt.cpp', u'Source/WebCore/xml/XSLTProcessorQt.cpp', u'Source/WebKit/blackberry/Api/DumpRenderTreeClient.h', u'Source/WebKit/blackberry/Api/WebPageClient.h', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp', u'Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebChromeClient.h', u'Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebChromeClient.h', u'Source/WebKit/wince/ChangeLog', u'Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.cpp', u'Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.h', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp', u'Source/WebKit/wx/WebKitSupport/ChromeClientWx.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h']" exit_code: 1
Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.h:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.h:87:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:87:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:277:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 7 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 198979 [details]
[PATCH] Add column number alongside line number

View in context: https://bugs.webkit.org/attachment.cgi?id=198979&action=review

This basically looks fine to me, but i'm going to r- it anyway because i'm annoying.  We now _always_ generate a stack trace when we throw an exception, we may as well just show it in the console (either in line, or via a drop down, i'm easy with either, but i also wouldn't want me designing UI for anything so I'm going to leave the decision with you)

>>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:277
>>> +                                          unsigned int lineNumber, unsigned columnNumber, const String& sourceID)
>> 
>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Omit int when using unsigned  [runtime/unsigned] [1]

You're changing this line, you may as well fix the style foible

>>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:87
>>> +                                     unsigned int lineNumber, unsigned columnNumber, const String& sourceID);
>> 
>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Omit int when using unsigned  [runtime/unsigned] [1]

ditto

[Timothy Hatcher]

We do show the stack trace and have for a while. It is the disclosure triangle at the beginning of the error message, if it is available. Not the best or most discoverable UI, but we might change it sometime soon to something better.

[Timothy Hatcher]

Note: we only capture stack traces if the Inspector is open. And I think it is limited to 5 call frames. Those rules were established by Google, and can be subject to change.

[Timothy Hatcher]

So Joe needed to do it this way to allow columns to work when the Inspector isn't open.

[Timothy Hatcher]

With that said, I still don't see the stack trace showing up in the Inspector for exceptions. Maybe we are only getting it for normal console messages?

[Joseph Pecoraro]

(In reply to comment #7)
> With that said, I still don't see the stack trace showing up in the Inspector for exceptions. Maybe we are only getting it for normal console messages?

With Oliver's improvements can now get this information for exceptions. However, I haven't modified the exception path with this patch. That will come in the next set of patches.

I wanted to try and take smaller steps after getting this large step out of the way, but let me take another look at making things more consistent now if needed.

[Joseph Pecoraro]

(In reply to comment #3)
> (From update of attachment 198979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198979&action=review
> 
> This basically looks fine to me, but i'm going to r- it anyway because i'm annoying.  We now _always_ generate a stack trace when we throw an exception, we may as well just show it in the console (either in line, or via a drop down, i'm easy with either, but i also wouldn't want me designing UI for anything so I'm going to leave the decision with you)

Yep, fixes "exceptions" will be in a follow up bug later (sometime this week). This starts with just the console API messages. I'll add a test to this that console API messages all have stacktraces if it makes sense.

[Joseph Pecoraro]

Created attachment 199120 [details]
[PATCH] Column Numbers and Stack Traces in Console API messages

I'll start working on exceptions now.

[WebKit Commit Bot]

Attachment 199120 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/inspector/console/console-messages-stack-traces-expected.txt', u'LayoutTests/inspector/console/console-messages-stack-traces.html', u'LayoutTests/inspector/console/console-url-and-line-expected.txt', u'LayoutTests/inspector/console/console-url-and-line.html', u'LayoutTests/inspector/console/console-url-line-column-expected.txt', u'LayoutTests/inspector/console/console-url-line-column.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptExecutionContext.h', u'Source/WebCore/inspector/ConsoleMessage.cpp', u'Source/WebCore/inspector/ConsoleMessage.h', u'Source/WebCore/inspector/Inspector.json', u'Source/WebCore/inspector/InspectorConsoleAgent.cpp', u'Source/WebCore/inspector/InspectorConsoleAgent.h', u'Source/WebCore/inspector/InspectorConsoleInstrumentation.h', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.h', u'Source/WebCore/inspector/InspectorProfilerAgent.cpp', u'Source/WebCore/inspector/InspectorProfilerAgent.h', u'Source/WebCore/loader/EmptyClients.h', u'Source/WebCore/page/ChromeClient.h', u'Source/WebCore/page/Console.cpp', u'Source/WebCore/page/ContentSecurityPolicy.cpp', u'Source/WebCore/page/PageConsole.cpp', u'Source/WebCore/page/PageConsole.h', u'Source/WebCore/workers/DefaultSharedWorkerRepository.cpp', u'Source/WebCore/workers/SharedWorkerContext.cpp', u'Source/WebCore/workers/WorkerContext.cpp', u'Source/WebCore/workers/WorkerContext.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerMessagingProxy.h', u'Source/WebCore/workers/WorkerReportingProxy.h', u'Source/WebCore/xml/XSLTProcessorLibxslt.cpp', u'Source/WebCore/xml/XSLTProcessorQt.cpp', u'Source/WebKit/blackberry/Api/DumpRenderTreeClient.h', u'Source/WebKit/blackberry/Api/WebPageClient.h', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp', u'Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebChromeClient.h', u'Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebChromeClient.h', u'Source/WebKit/wince/ChangeLog', u'Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.cpp', u'Source/WebKit/wince/WebCoreSupport/ChromeClientWinCE.h', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp', u'Source/WebKit/wx/WebKitSupport/ChromeClientWx.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h']" exit_code: 1
Source/WebKit/wx/WebKitSupport/ChromeClientWx.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/wx/WebKitSupport/ChromeClientWx.h:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 199120 [details]
[PATCH] Column Numbers and Stack Traces in Console API messages

Attachment 199120 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/157636

New failing tests:
inspector/console/console-messages-stack-traces.html

[Build Bot]

Created attachment 199141 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 199120 [details]
[PATCH] Column Numbers and Stack Traces in Console API messages

Attachment 199120 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/110756

New failing tests:
inspector/console/console-messages-stack-traces.html

[Build Bot]

Created attachment 199154 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Joseph Pecoraro]

The different layout-test-results was just me forgetting to git add the new expected results to the patch attached. I fixed it locally. I'll land this tomorrow when I can watch the bots.

[Joseph Pecoraro]

Committed r149125: <http://trac.webkit.org/changeset/149125>

[Joseph Pecoraro]

(In reply to comment #17)
> Committed r149125: <http://trac.webkit.org/changeset/149125>

Apparently the bot has a few failures:

<http://build.webkit.org/results/Apple%20Lion%20Release%20WK2%20(Tests)/r149128%20(10144)/results.html>

  - inspector/console/console-log-syntax-error.html
  Seems fine, but isn't happening locally for me. Need to investigate.

  - inspector/extensions/extensions-panel.html
  - inspector/extensions/extensions-resources.html
  No idea what would be going on here.