Bug 114929

Summary: Web Inspector: ConsoleMessage should include line and column number where possible
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web Inspector (Deprecated)Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, graouts, joepeck, rniwa, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Add column number alongside line number
oliver: review-, joepeck: commit-queue-
[PATCH] Column Numbers and Stack Traces in Console API messages
timothy: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 none

Description Joseph Pecoraro 2013-04-21 18:04:59 PDT 
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.
Comment 1 Joseph Pecoraro 2013-04-21 18:22:07 PDT 
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.
Comment 2 WebKit Commit Bot 2013-04-21 18:23:22 PDT 
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.
Comment 3 Oliver Hunt 2013-04-21 19:42:24 PDT 
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
Comment 4 Timothy Hatcher 2013-04-22 04:32:08 PDT 
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.
Comment 5 Timothy Hatcher 2013-04-22 04:40:13 PDT 
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.
Comment 6 Timothy Hatcher 2013-04-22 05:23:58 PDT 
So Joe needed to do it this way to allow columns to work when the Inspector isn't open.
Comment 7 Timothy Hatcher 2013-04-22 06:09:05 PDT 
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?
Comment 8 Joseph Pecoraro 2013-04-22 11:09:03 PDT 
(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.
Comment 9 Joseph Pecoraro 2013-04-22 15:40:52 PDT 
(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.
Comment 10 Joseph Pecoraro 2013-04-22 15:45:53 PDT 
Created attachment 199120 [details]
[PATCH] Column Numbers and Stack Traces in Console API messages

I'll start working on exceptions now.
Comment 11 WebKit Commit Bot 2013-04-22 15:47:34 PDT 
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.
Comment 12 Build Bot 2013-04-22 20:47:18 PDT 
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
Comment 13 Build Bot 2013-04-22 20:47:20 PDT 
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
Comment 14 Build Bot 2013-04-22 23:34:20 PDT 
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
Comment 15 Build Bot 2013-04-22 23:34:22 PDT 
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
Comment 16 Joseph Pecoraro 2013-04-24 23:48:43 PDT 
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.
Comment 17 Joseph Pecoraro 2013-04-25 11:33:52 PDT 
Committed r149125: <http://trac.webkit.org/changeset/149125>
Comment 18 Joseph Pecoraro 2013-04-25 12:51:20 PDT 
(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.