Bug 63622 – Web Inspector: Adding CSS properties results in messy style rules

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

[Alexander Pavlov (apavlov)]

1. Add a property to an existing style rule via the Elements tab.
2. Go to the stylesheet in the Resources tab to see how its source has been modified.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=83532

[Alexander Pavlov (apavlov)]

Created attachment 99102 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorStyleSheet.h:146:  The parameter name "property" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


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

[Alexander Pavlov (apavlov)]

Created attachment 99103 [details]
[PATCH] Style fixed

[WebKit Review Bot]

Comment on attachment 99103 [details]
[PATCH] Style fixed

Attachment 99103 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8965087

[WebKit Review Bot]

Comment on attachment 99103 [details]
[PATCH] Style fixed

Attachment 99103 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8960220

[WebKit Review Bot]

Comment on attachment 99103 [details]
[PATCH] Style fixed

Attachment 99103 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8965090

[Alexander Pavlov (apavlov)]

Created attachment 99281 [details]
[PATCH] Chromium build fix

[WebKit Review Bot]

Comment on attachment 99281 [details]
[PATCH] Chromium build fix

Attachment 99281 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8970053

[Alexander Pavlov (apavlov)]

Created attachment 99467 [details]
[PATCH] Test and more heuristics added

[WebKit Review Bot]

Comment on attachment 99467 [details]
[PATCH] Test and more heuristics added

Attachment 99467 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8970460

[Early Warning System Bot]

Comment on attachment 99467 [details]
[PATCH] Test and more heuristics added

Attachment 99467 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8963852

[WebKit Review Bot]

Comment on attachment 99467 [details]
[PATCH] Test and more heuristics added

Attachment 99467 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8960937

[Alexander Pavlov (apavlov)]

Created attachment 99471 [details]
[PATCH] Build fix attempt

[WebKit Review Bot]

Comment on attachment 99471 [details]
[PATCH] Build fix attempt

Attachment 99471 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8965861

New failing tests:
inspector/styles/styles-new-API.html

[WebKit Review Bot]

Created attachment 99477 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 99471 [details]
[PATCH] Build fix attempt

The logic seems Ok, but I think we need a solid number of tests in order to land this. Could you please add tests both for average and edge cases?

[Alexander Pavlov (apavlov)]

Created attachment 99703 [details]
[PATCH] Test augmented, more heuristics

[WebKit Review Bot]

Comment on attachment 99703 [details]
[PATCH] Test augmented, more heuristics

Attachment 99703 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8984408

[Early Warning System Bot]

Comment on attachment 99703 [details]
[PATCH] Test augmented, more heuristics

Attachment 99703 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8990123

[WebKit Review Bot]

Comment on attachment 99703 [details]
[PATCH] Test augmented, more heuristics

Attachment 99703 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8988314

[WebKit Review Bot]

Comment on attachment 99703 [details]
[PATCH] Test augmented, more heuristics

Attachment 99703 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8982756

[Alexander Pavlov (apavlov)]

Created attachment 99815 [details]
[PATCH] Qt build fix

[WebKit Review Bot]

Comment on attachment 99815 [details]
[PATCH] Qt build fix

Attachment 99815 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8985698

[WebKit Review Bot]

Comment on attachment 99815 [details]
[PATCH] Qt build fix

Attachment 99815 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8987657

[WebKit Review Bot]

Comment on attachment 99815 [details]
[PATCH] Qt build fix

Attachment 99815 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8986734

[Alexander Pavlov (apavlov)]

Created attachment 99818 [details]
[PATCH] Chromium build fix

[Pavel Feldman]

Comment on attachment 99818 [details]
[PATCH] Chromium build fix

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:352
> +void InspectorStyle::ensureFormatPropertyPrefix(Vector<InspectorStyleProperty>& allProperties)

I think detection should be based upon CSSRuleSourceData instead. I.e. you get CSS markup and detect the indentation.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:697
> +    if (!newTextLength && fullPrefixLength) {

This is way too much math in a single method. Can it be split?

[Alexander Pavlov (apavlov)]

Created attachment 99992 [details]
[PATCH] InspectorStyleTextEditor factored out; made use of CSSPropertySourceData instead of InspectorStyleProperty

[Alexander Pavlov (apavlov)]

Created attachment 99993 [details]
[PATCH] Removed parameter name

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorStyleSheet.h:138:  The parameter name "property" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 10 files


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

[Pavel Feldman]

Comment on attachment 99993 [details]
[PATCH] Removed parameter name

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

> Source/WebCore/inspector/InspectorStyleSheet.cpp:56
> +static inline bool isWhitespace(UChar ch)

Let the compiler decide.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:156
> +InspectorStyleTextEditor::InspectorStyleTextEditor(Vector<InspectorStyleProperty>* disabledProperties, const std::pair<String, String>& format)

According to its size, it deserves its own file.

typedef the pair.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:477
> +    ensureTextEditor(&allProperties, text);

I'd allocate editor on stack and use it once.

[Alexander Pavlov (apavlov)]

Created attachment 100011 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Committed r90619: <http://trac.webkit.org/changeset/90619>