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.
Created attachment 66370 [details] [PATCH] Suggested solution
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.
Created attachment 66371 [details] [PATCH] Style fixed
Attachment 66370 [details] did not build on mac: Build output: http://queues.webkit.org/results/3933066
Attachment 66371 [details] did not build on mac: Build output: http://queues.webkit.org/results/3942041
Created attachment 66376 [details] [PATCH] Remove unused var
Created attachment 66492 [details] [PATCH] Account for !important in property value strings + test
Created attachment 66625 [details] [PATCH] Fix "important" keyword detection + test
Created attachment 66628 [details] [PATCH] Rebase + fixed "important" detection
Created attachment 66629 [details] [PATCH] Style fix
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.
Created attachment 66909 [details] [PATCH] Rebased to follow the DocLoader rename
Joseph, could you please finish reviewing this patch? You seem to have started already :)
(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.
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.
(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.
> > 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.
Created attachment 67667 [details] [PATCH] Review comments addressed
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.
Created attachment 67668 [details] [PATCH] Include order fixed (case-sensitive)
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
(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.
Created attachment 67688 [details] [PATCH] pfeldman's comments addressed
http://trac.webkit.org/changeset/67704 might have broken Qt Linux Release minimal
Committed: http://trac.webkit.org/changeset/67704 Build fix: http://trac.webkit.org/changeset/67705
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.