Bug 36414 – Web Inspector: display CSS selector source line in the styles sidebar pane.

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

[Pavel Feldman]

Patch to follow shortly.

Source lines are provided relative to the start of the stylesheet definition. This works fine for linked styles just fine, but is obviously not what user would expect in case of inline ones (it has offset from the <style> tag instead of the html file line number). In order to fix this, we need to pull line number from the HTMLTokenizer to the CSS parser call. Or alternatively we could put line number in HTMLStyle tag and compute exact rule line later. Anyways, for now, I only show line numbers for external css files, inline ones' caption did not change.

[Pavel Feldman]

Created attachment 51223 [details]
[IMAGE] Screenshot while running with patch.

[Pavel Feldman]

Created attachment 51224 [details]
[PATCH] Proposed change.

[Joseph Pecoraro]

Nice!!

There was an open bug with a number of interested people CC'd (and some discussion):
https://bugs.webkit.org/show_bug.cgi?id=21268

How much does this affect CSS Parse times?

Have you considered adding `sourceLine` to the CSSStyleRule.idl to expose the value to developers? It would be non-standard but it might be nice to have.

[Pavel Feldman]

> How much does this affect CSS Parse times?

An equal check and an increment should not affect performance. At least I would not expect it to. Converting comment tokens from ignored to whitespace means that I added some processing time for the css files that have a lot of comments. I can provide some stats if necessary, but I expect performance not to regress at all.
In case I am wrong, we can always slit isCSSWhitespace into two methods one of them will be incrementing line numbers. That way regression is guaranteed to be 0 by the comments modulus. I will probably do that just in case.

> 
> Have you considered adding `sourceLine` to the CSSStyleRule.idl to expose the
> value to developers? It would be non-standard but it might be nice to have.

No, I am not sure if that is important. We can now implement this feature without exposing line numbers, so that line numbers exposure becomes a subject of a different request.

[Pavel Feldman]

> In case I am wrong, we can always slit isCSSWhitespace into two methods one of
> them will be incrementing line numbers. That way regression is guaranteed to be
> 0 by the comments modulus. I will probably do that just in case.

This is wrong, isCSSWhitespace is not the thing that might affect performance. Main guys are the check and increment I mentioned earlier. Still, I don't think that we can talk about regression here, but I can do some stats!

[Pavel Feldman]

Created attachment 51330 [details]
[PATCH] Proposed change that does not regress performance.

There was an 8% performance regression in the original patch caused by the bison's token location handling code. As soon as I started using @1.first_line, whole yylloc thing was starting to make things slower. I went back the the location-less parsing, but inserted updateLocation calls into the selector rules. At the moment of their action triggering, lexer is at the '{' token which is exactly where we want to end up upon navigation. Updated patch does not regress performance, I was testing it on a 4Mb concatenated CSS taken from chromium page cyclers.

[Pavel Feldman]

Created attachment 51333 [details]
[PATCH] Oops - wrong patch squeezed in last time.

[Dave Hyatt]

Comment on attachment 51333 [details]
[PATCH] Oops - wrong patch squeezed in last time.

r=me

[Dave Hyatt]

Comment on attachment 51333 [details]
[PATCH] Oops - wrong patch squeezed in last time.

Comments issue brought up in IRC.

[Joseph Pecoraro]

(In reply to comment #6)
> Created an attachment (id=51330) [details]
> There was an 8% performance regression in the original patch caused by the
> bison's token location handling code. As soon as I started using @1.first_line,
> whole yylloc thing was starting to make things slower. I went back the the
> location-less parsing, but inserted updateLocation calls into the selector
> rules. At the moment of their action triggering, lexer is at the '{' token
> which is exactly where we want to end up upon navigation. Updated patch does
> not regress performance, I was testing it on a 4Mb concatenated CSS taken from
> chromium page cyclers.

Excellent! Thanks for checking on this and improving it!

> @@ -4702,6 +4704,10 @@ int CSSParser::lex(void* yylvalWithoutType)
>      case SGML_CD:
>      case INCLUDES:
>      case DASHMATCH:
> +        for (UChar* current = t; current < t + length; ++current) {
> +            if (*current == '\n')
> +                ++m_line;
> +        }
>          break;

Any particular reason this is here and not just after the WHITESPACE token (above SGML_CD)? Can `t` (the lexed text) contain whitespace in the other 3 cases (SGML_CD, INCLUDES, DASHMATCH), and if so could it contain whitespace in any of the other cases too? For example, I figured if there was whitespace before a "<!--" it would have been lexed as a WHITESPACE token already, but I could be wrong.


> -\/\*[^*]*\*+([^/*][^*]*\*+)*\/  /* ignore comments */
> +\/\*[^*]*\*+([^/*][^*]*\*+)*\/  {yyTok = WHITESPACE; return yyTok;}

I think it would be nice to make a COMMENT token, or change WHITESPACE to mean WHITESPACE_OR_COMMENT. Or, at the very least there could be a source comment explaining that the WHITESPACE token is returned so that newlines in the comments can be counted, and not for any semantic reason. This is just a personal nit, since most WebKit code tries to be as clear as possible and this is a little clever.

Another personal nit would be that the ChangeLog is pretty bare!


> -        this.subtitle = subtitle;
> +        if (subtitle)
> +            this.subtitle = subtitle;

Does this actually change anything? Still, I like that its clearer now.

[Joseph Pecoraro]

Apparently I was about 20 minutes behind some IRC conversation. Doh!

[Pavel Feldman]

Created attachment 51338 [details]
[PATCH] Proposed change. Review comments addressed, comments are back to being ignored.

[Pavel Feldman]

> Any particular reason this is here and not just after the WHITESPACE token
> (above SGML_CD)? Can `t` (the lexed text) contain whitespace in the other 3
> cases (SGML_CD, INCLUDES, DASHMATCH), and if so could it contain 
whitespace in
> any of the other cases too? For example, I figured if there was whitespace
> before a "<!--" it would have been lexed as a WHITESPACE token already, but 

You are right, I got confused with SGML, for a moment I was thinking it could contain whitespace. Now it is under WHITESPACE.

> 
> > -\/\*[^*]*\*+([^/*][^*]*\*+)*\/  /* ignore comments */
> > +\/\*[^*]*\*+([^/*][^*]*\*+)*\/  {yyTok = WHITESPACE; return yyTok;}

This is actually something that was wrong as discussed on IRC.

 
> Another personal nit would be that the ChangeLog is pretty bare!

Added few words there.

> 
> > -        this.subtitle = subtitle;
> > +        if (subtitle)
> > +            this.subtitle = subtitle;
> 

Yes, subtitle is not being defined in the first case above (we are assigning linkified node instead).

[Dave Hyatt]

Comment on attachment 51338 [details]
[PATCH] Proposed change. Review comments addressed, comments are back to being ignored.

r=me

[Pavel Feldman]

Created attachment 51351 [details]
[PATCH] Variation of patch I'm going to land.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/inspector/styles-source-lines-expected.txt
	A	LayoutTests/inspector/styles-source-lines.html
	M	WebCore/ChangeLog
	M	WebCore/css/CSSGrammar.y
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSParser.h
	M	WebCore/css/CSSStyleRule.cpp
	M	WebCore/css/CSSStyleRule.h
	M	WebCore/css/tokenizer.flex
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
Committed r56383

[David Kilzer (:ddkilzer)]

http://trac.webkit.org/changeset/56383