Bug 41691

Summary: Web Inspector: provide line numbers for inline styles.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, hyatt, joepeck, keishi, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed change. joepeck: review+, joepeck: commit-queue-

Description Pavel Feldman 2010-07-06 06:03:11 PDT
Patch to follow.
Comment 1 Pavel Feldman 2010-07-06 07:51:51 PDT
Created attachment 60631 [details]
[PATCH] Proposed change.

We will probably need Dave's blessing for this one, but please review it before I add him to the CC list.
Comment 2 Pavel Feldman 2010-07-06 22:44:13 PDT
Comments for the patch: In order to provide rules of inline css with proper line numbers, I get current line number from the parser upon HTMLStyleElement creation (I store it in int there). Then I extend StyleElement's interface to accept the line number baseline that is used in the CSSParser when constructing rules.
Comment 3 Joseph Pecoraro 2010-07-06 22:44:18 PDT
Comment on attachment 60631 [details]
[PATCH] Proposed change.

> +        * inspector/styles-source-lines-expected.txt: Removed.
> +        * inspector/styles-source-lines.html:

This test is still here, but probably changed. Should there be
results checked in for it, or do you want those generated?


>  bool CSSStyleSheet::parseString(const String &string, bool strict)
>  {
> +    return parseStringAtLine(string, strict, 0);
> +}
...
>      virtual bool parseString(const String&, bool strict = true);

Can this be inlined? Are compilers smart enough to do it anyways?


> +    int m_startLine;

My comment would have been to make these counters "unsigned". But
there are already plenty of line counters as "int". This is fine then.

Looks good to me (with those updated test results)!
Comment 4 Pavel Feldman 2010-07-06 22:47:22 PDT
(In reply to comment #2)
> Comments for the patch: In order to provide rules of inline css with proper line numbers, I get current line number from the parser upon HTMLStyleElement creation (I store it in int there). Then I extend StyleElement's interface to accept the line number baseline that is used in the CSSParser when constructing rules.

Dave, could you take a look at it? It adds an argument to StyleElement and stores int in HTMLStyleElement.
Comment 5 Joseph Pecoraro 2010-07-06 23:03:44 PDT
Comment on attachment 60631 [details]
[PATCH] Proposed change.

> +++ b/WebCore/html/HTMLStyleElement.cpp
> @@ -37,7 +38,10 @@ inline HTMLStyleElement::HTMLStyleElement(const QualifiedName& tagName, Document
>      : HTMLElement(tagName, document)
>      , m_loading(false)
>      , m_createdByParser(createdByParser)
> +    , m_startLine(0)
>  {
> +    if (createdByParser && document && document->scriptableDocumentParser())
> +        m_startLine = document->scriptableDocumentParser()->lineNumber();
>      ASSERT(hasTagName(styleTag));
>  }

One last thing, move this after the ASSERT.
An extra int per style element should be fine.
r=me
Comment 6 Pavel Feldman 2010-07-07 00:41:49 PDT
Landed with review comments addressed. Also renamed startLine to startLineNumber:

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/http/tests/inspector/inspector-test.js
	M	LayoutTests/inspector/elements-tests.js
	A	LayoutTests/inspector/resources/styles-source-lines-inline-iframe.html
	M	LayoutTests/inspector/styles-iframe.html
	M	LayoutTests/inspector/styles-source-lines-expected.txt
	A	LayoutTests/inspector/styles-source-lines-inline-expected.txt
	A	LayoutTests/inspector/styles-source-lines-inline.html
	M	LayoutTests/inspector/styles-source-lines.html
	M	WebCore/ChangeLog
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSParser.h
	M	WebCore/css/CSSStyleSheet.cpp
	M	WebCore/css/CSSStyleSheet.h
	M	WebCore/dom/StyleElement.cpp
	M	WebCore/dom/StyleElement.h
	M	WebCore/html/HTMLStyleElement.cpp
	M	WebCore/html/HTMLStyleElement.h
	M	WebCore/inspector/InspectorCSSStore.cpp
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/front-end/DOMAgent.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/svg/SVGStyleElement.cpp
Committed r62630