Bug 44949 – Provide source-based properties for style declarations to CSSParser clients

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexander Pavlov (apavlov)]

In order to improve the Web Inspector CSS editing capabilities, it is desirable to be able to retrieve style properties as they were written by the user in the CSS style declaration rather than the way WebCore has stored them internally. Having the source ranges (start/end offsets) for properties is also necessary to enable model-based editing of CSS styles via setting the entire style text.

[Alexander Pavlov (apavlov)]

Created attachment 66370 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSPropertyDatum.cpp:68:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/css/CSSGrammar.y:1412:  Line contains tab character.  [whitespace/tab] [5]
WebCore/css/CSSParser.cpp:5696:  Missing space before ( in while(  [whitespace/parens] [5]
WebCore/css/CSSParser.cpp:5714:  Missing space before ( in while(  [whitespace/parens] [5]
WebCore/css/CSSParser.cpp:5727:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 5 in 20 files


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

[Alexander Pavlov (apavlov)]

Created attachment 66371 [details]
[PATCH] Style fixed

[Eric Seidel (no email)]

Attachment 66370 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3933066

[Eric Seidel (no email)]

Attachment 66371 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3942041

[Alexander Pavlov (apavlov)]

Created attachment 66376 [details]
[PATCH] Remove unused var

[Alexander Pavlov (apavlov)]

Created attachment 66492 [details]
[PATCH] Account for !important in property value strings + test

[Alexander Pavlov (apavlov)]

Created attachment 66625 [details]
[PATCH] Fix "important" keyword detection + test

[Alexander Pavlov (apavlov)]

Created attachment 66628 [details]
[PATCH] Rebase + fixed "important" detection

[Alexander Pavlov (apavlov)]

Created attachment 66629 [details]
[PATCH] Style fix

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:5804:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 20 files


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

[Alexander Pavlov (apavlov)]

Created attachment 66909 [details]
[PATCH] Rebased to follow the DocLoader rename

[Alexander Pavlov (apavlov)]

Joseph, could you please finish reviewing this patch? You seem to have started already :)

[Joseph Pecoraro]

(In reply to comment #13)
> Joseph, could you please finish reviewing this patch? You seem to have started already :)

Heh, I remember saying to give a good review on this it would take me
approx. and hour of studying the code. I know there are others who
are more familiar with the CSS Parser and parsing, I know it is difficult
to split this patch up, but that could be an approach that might help
this to get reviewed faster. As of right now I don't have that kind of
time to look at this in the detail it deserves. Pavel just mentioned he
could check it out when he gets back, and I know he has more experience
in this area.

[Dave Hyatt]

Comment on attachment 66909 [details]
[PATCH] Rebased to follow the DocLoader rename

In CSSMutableStyleDeclaration, Node* node() { return m_node; }

add a const to the method please.  Node* node() const { return m_node; }

Assuming this has no impact on the performance of parsing stylesheets in real-world usage, then r=me.

[Alexander Pavlov (apavlov)]

(In reply to comment #15)
> (From update of attachment 66909 [details])
> In CSSMutableStyleDeclaration, Node* node() { return m_node; }
> 
> add a const to the method please.  Node* node() const { return m_node; }

Sorry, forgot about it.

> Assuming this has no impact on the performance of parsing stylesheets in real-world usage, then r=me.

The added code for filling in the data works only when the parser is requested to do so (that is, non-NULL structure is passed in by the client).

=========

pfeldman@ had some more comments he forgot to send here and lost after a browser restart (but he had sent them to me over IM, so I'm reposting them with my replies inline):

> 1. SourceRange could be a struct with the "start" and "end" fields which improves readability and simplifies code

Done.

> 2. InspectorCSSStore::resourceStyleSheetText should be defined in InspectorController. That way you won't need to add more includes unrelated to the CSS domain. Let controller care about resource logic.

Since the core algorithm is not related to stylesheets only but instead to all resources, I'm extracting it as InspectorController::resourceContentForURL and delegating InspectorCSSStore::resourceStyleSheetText to it.

> 3. I was wondering if we could form the !important detection as a separate patch? Rationale: 1) It seems orthogonal to the rest of the logic and 2) I'd like to better understand the value we get from it. It seems to add a lot of complexity to the code.

This code is not so much orthogonal. It enables the correct detection of the "important" bit and, hence, the "value" field of CSSPropertyDatum for non-parsed properties, i.e. those ones whose "declaration" (from CSSGrammar.y) has an invalid structure (not "property ':' maybe_space expr prio")).
Still, I agree this code adds more overall complexity. I will remove the detection code, fix failing tests and file a separate bug once this patch is landed.

[Alexander Pavlov (apavlov)]

> > 3. I was wondering if we could form the !important detection as a separate patch? Rationale: 1) It seems orthogonal to the rest of the logic and 2) I'd like to better understand the value we get from it. It seems to add a lot of complexity to the code.
> 
> This code is not so much orthogonal. It enables the correct detection of the "important" bit and, hence, the "value" field of CSSPropertyDatum for non-parsed properties, i.e. those ones whose "declaration" (from CSSGrammar.y) has an invalid structure (not "property ':' maybe_space expr prio")).
> Still, I agree this code adds more overall complexity. I will remove the detection code, fix failing tests and file a separate bug once this patch is landed.

After a quick discussion with pfeldman, we decided to consider all the text after ':' as the property value, so stripping off "!important" should not happen.

[Alexander Pavlov (apavlov)]

Created attachment 67667 [details]
[PATCH] Review comments addressed

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/InspectorCSSStore.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 22 files


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

[Alexander Pavlov (apavlov)]

Created attachment 67668 [details]
[PATCH] Include order fixed (case-sensitive)

[Pavel Feldman]

Comment on attachment 67668 [details]
[PATCH] Include order fixed (case-sensitive)

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

> WebCore/css/CSSParser.cpp:228
> +void CSSParser::parseSheet(CSSStyleSheet* sheet, const String& string, int startLineNumber, StyleRuleRangeMap* ruleRangeMap)
I'd rather return CSSStylesheetSourceData.

> WebCore/css/CSSPropertyDatum.h:63
> +    bool important;
I thought this was no longer needed.

> WebCore/css/CSSPropertyDatum.h:64
> +    bool parsed;
parsedOk

> WebCore/css/CSSPropertyDatum.h:89
> +    Vector<std::pair<CSSPropertyDatum, SourceRange> > propertyData;
1) I would rename CSSPropertyDatum -> CSSPropertySourceData
2) I would not make CSSPropertySourceData a hashmap key since it adds complexity
3) I would place range within CSSPropertySourceData as you do wuth CSSStyleSourceData for consistency
4) I would add 'hash' method to CSSPropertySourceData for the sake of enable/disable resolution
5) For each *SourceData I would place a direct reference (refcounted) to corresponding model element

[Alexander Pavlov (apavlov)]

(In reply to comment #21)
> (From update of attachment 67668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67668&action=prettypatch
> 
> > WebCore/css/CSSParser.cpp:228
> > +void CSSParser::parseSheet(CSSStyleSheet* sheet, const String& string, int startLineNumber, StyleRuleRangeMap* ruleRangeMap)
> I'd rather return CSSStylesheetSourceData.
We can include it into the upcoming refactoring we have discussed. I will file a bug.

> > WebCore/css/CSSPropertyDatum.h:63
> > +    bool important;
> I thought this was no longer needed.
It is still needed for "parsedOk" properties so that the property overrides are computed correctly by StylesSidebarPane

> > WebCore/css/CSSPropertyDatum.h:64
> > +    bool parsed;
> parsedOk
Done

> > WebCore/css/CSSPropertyDatum.h:89
> > +    Vector<std::pair<CSSPropertyDatum, SourceRange> > propertyData;
> 1) I would rename CSSPropertyDatum -> CSSPropertySourceData
> 2) I would not make CSSPropertySourceData a hashmap key since it adds complexity
> 3) I would place range within CSSPropertySourceData as you do wuth CSSStyleSourceData for consistency
> 4) I would add 'hash' method to CSSPropertySourceData for the sake of enable/disable resolution
Fixed these four.

> 5) For each *SourceData I would place a direct reference (refcounted) to corresponding model element
Not implementing, since we don't need it at (rather, we need a reverse mapping), and this is related to the refactoring I mentioned above.

[Alexander Pavlov (apavlov)]

Created attachment 67688 [details]
[PATCH] pfeldman's comments addressed

[WebKit Review Bot]

http://trac.webkit.org/changeset/67704 might have broken Qt Linux Release minimal

[Alexander Pavlov (apavlov)]

Committed: http://trac.webkit.org/changeset/67704
Build fix: http://trac.webkit.org/changeset/67705

[Eric Seidel (no email)]

Comment on attachment 66909 [details]
[PATCH] Rebased to follow the DocLoader rename

Cleared David Hyatt's review+ from obsolete attachment 66909 [details] so that this bug does not appear in http://webkit.org/pending-commit.