Bug 111334 – Web Inspector: Plumbing CSS warnings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Sergey Ryazanov]

Adding a framework for delivering CSS parsing messages to Inspector console.

[Sergey Ryazanov]

Created attachment 191253 [details]
Patch

[Pavel Feldman]

Comment on attachment 191253 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:61
> +#include "DOMWindow.h"

I don't think CSSParser should depend on DOMWindow.

> Source/WebCore/css/CSSParser.cpp:11126
> +            builder.appendLiteral("\\n");

I still think CSSParser should not bother with formatting. It should pass as much info as possible into console.

> Source/WebCore/css/CSSParser.cpp:11717
> +CSSParser::Location CSSParser::currentLocation()

This seems to be unused.

> Source/WebCore/css/CSSParser.h:382
> +    Console* m_console;

You should not store console pointer in parser.

[Sergey Ryazanov]

Created attachment 191498 [details]
Patch

[Sergey Ryazanov]

Comment on attachment 191253 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:61
>> +#include "DOMWindow.h"
> 
> I don't think CSSParser should depend on DOMWindow.

DOMWindow owns the console don the dependency is inevitable. Initially I added a helper method to Document hiding that dependency but you suggested to use console directly (https://bugs.webkit.org/attachment.cgi?id=190686&action=review).

Document doesn't provide over way to plumb a message. addMessage is private, addMessageToConsole doesn't allow to pass location information.

So I only can leave it as is or return logCSSWarningToConsole. I'd prefer second approach. It looks similar to that Document alreasy has (I mean logExceptionToConsole).

>> Source/WebCore/css/CSSParser.cpp:11126
>> +            builder.appendLiteral("\\n");
> 
> I still think CSSParser should not bother with formatting. It should pass as much info as possible into console.

Removed.

>> Source/WebCore/css/CSSParser.cpp:11717
>> +CSSParser::Location CSSParser::currentLocation()
> 
> This seems to be unused.

Removed for now.

>> Source/WebCore/css/CSSParser.h:382
>> +    Console* m_console;
> 
> You should not store console pointer in parser.

done

[Sergey Ryazanov]

Comment on attachment 191253 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:61
>> +#include "DOMWindow.h"
> 
> I don't think CSSParser should depend on DOMWindow.

DOMWindow owns the console don the dependency is inevitable. Initially I added a helper method to Document hiding that dependency but you suggested to use console directly (https://bugs.webkit.org/attachment.cgi?id=190686&action=review).

Document doesn't provide over way to plumb a message. addMessage is private, addMessageToConsole doesn't allow to pass location information.

So I only can leave it as is or return logCSSWarningToConsole. I'd prefer second approach. It looks similar to that Document alreasy has (I mean logExceptionToConsole).

>> Source/WebCore/css/CSSParser.cpp:11126
>> +            builder.appendLiteral("\\n");
> 
> I still think CSSParser should not bother with formatting. It should pass as much info as possible into console.

Removed.

>> Source/WebCore/css/CSSParser.cpp:11717
>> +CSSParser::Location CSSParser::currentLocation()
> 
> This seems to be unused.

Removed for now.

>> Source/WebCore/css/CSSParser.h:382
>> +    Console* m_console;
> 
> You should not store console pointer in parser.

done

[Sergey Ryazanov]

Comment on attachment 191253 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:61
>> +#include "DOMWindow.h"
> 
> I don't think CSSParser should depend on DOMWindow.

DOMWindow owns the console don the dependency is inevitable. Initially I added a helper method to Document hiding that dependency but you suggested to use console directly (https://bugs.webkit.org/attachment.cgi?id=190686&action=review).

Document doesn't provide over way to plumb a message. addMessage is private, addMessageToConsole doesn't allow to pass location information.

So I only can leave it as is or return logCSSWarningToConsole. I'd prefer second approach. It looks similar to that Document alreasy has (I mean logExceptionToConsole).

>> Source/WebCore/css/CSSParser.cpp:11126
>> +            builder.appendLiteral("\\n");
> 
> I still think CSSParser should not bother with formatting. It should pass as much info as possible into console.

Removed.

>> Source/WebCore/css/CSSParser.cpp:11717
>> +CSSParser::Location CSSParser::currentLocation()
> 
> This seems to be unused.

Removed for now.

>> Source/WebCore/css/CSSParser.h:382
>> +    Console* m_console;
> 
> You should not store console pointer in parser.

done

[Sergey Ryazanov]

Created attachment 191730 [details]
Patch

[Pavel Feldman]

Comment on attachment 191730 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4780
> +void Document::addConsoleMessage(MessageSource source, MessageLevel level, const String& message, const KURL& sourceURL, int lineNumber)

As I mentioned, I don't think piping console messages through DOMWindow is the way to go.

[Sergey Ryazanov]

Created attachment 193544 [details]
Patch

[Pavel Feldman]

Comment on attachment 193544 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:10553
> +    m_tokenStartLineNumber = m_lineNumber;

This field seems to be unused.

> Source/WebCore/css/CSSParser.cpp:11239
> +    m_styleSheet->singleOwnerDocument()->page()->console()->addMessage(CSSMessageSource, WarningMessageLevel, message, m_styleSheet->baseURL().string(), lineNumber + 1);

Intermediate variable would not hurt.

[Sergey Ryazanov]

Created attachment 193871 [details]
Patch

[Timothy Hatcher]

Comment on attachment 193871 [details]
Patch

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

> Source/WebCore/page/PageConsole.cpp:173
> +    if (source == CSSMessageSource)
> +        return;

Why not allow CSS messages to print to stdout too? I use this to ease development of the Web Inspector, so I don't always need to inspect the Inspector to see messages. Please remove this unless there is a compelling reason.

[Pavel Feldman]

Comment on attachment 193871 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:11219
> +    if (!isLoggingErrors())

Check for m_logErrors to speed things up.

>> Source/WebCore/page/PageConsole.cpp:173
>> +        return;
> 
> Why not allow CSS messages to print to stdout too? I use this to ease development of the Web Inspector, so I don't always need to inspect the Inspector to see messages. Please remove this unless there is a compelling reason.

It would need a ton of rebaselines. We'll do that when the dust settles.

[Pavel Feldman]

Comment on attachment 193871 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:11219
>> +    if (!isLoggingErrors())
> 
> Check for m_logErrors to speed things up.

Check for m_logErrors to speed things up.

>>> Source/WebCore/page/PageConsole.cpp:173
>>> +        return;
>> 
>> Why not allow CSS messages to print to stdout too? I use this to ease development of the Web Inspector, so I don't always need to inspect the Inspector to see messages. Please remove this unless there is a compelling reason.
> 
> It would need a ton of rebaselines. We'll do that when the dust settles.

It would need a ton of rebaselines. We'll do that when the dust settles.

[WebKit Review Bot]

Comment on attachment 193871 [details]
Patch

Rejecting attachment 193871 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 193871, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13971 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
55>At revision 13971.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://webkit-commit-queue.appspot.com/results/17200476

[Sergey Ryazanov]

Created attachment 194068 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194068 [details]
Patch

Clearing flags on attachment: 194068

Committed r146353: <http://trac.webkit.org/changeset/146353>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Glenn Adams]

Comment on attachment 194068 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:474
> +    m_logErrors = false;

why is m_logErrors set twice?

> Source/WebCore/css/CSSParser.cpp:11226
> +        builder.append(location.token.characters16(), location.token.length());

why not just use builder.append(location.token)?

[Sergey Ryazanov]

Comment on attachment 194068 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:474
>> +    m_logErrors = false;
> 
> why is m_logErrors set twice?

It's used inside cssyyparse call. Almost each member abound follows the same pattern.

>> Source/WebCore/css/CSSParser.cpp:11226
>> +        builder.append(location.token.characters16(), location.token.length());
> 
> why not just use builder.append(location.token)?

In order to avoid implicit conversion to WTF::String (CSSParserString::operator String() const would be applied).