Bug 63622 - Web Inspector: Adding CSS properties results in messy style rules
Summary: Web Inspector: Adding CSS properties results in messy style rules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-29 05:55 PDT by Alexander Pavlov (apavlov)
Modified: 2011-07-08 04:35 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Suggested solution (15.65 KB, patch)
2011-06-29 09:34 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Style fixed (15.65 KB, patch)
2011-06-29 09:37 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Chromium build fix (16.80 KB, patch)
2011-06-30 06:07 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Test and more heuristics added (25.94 KB, patch)
2011-07-01 06:51 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Build fix attempt (20.49 KB, patch)
2011-07-01 07:35 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.23 MB, application/zip)
2011-07-01 08:54 PDT, WebKit Review Bot
no flags Details
[PATCH] Test augmented, more heuristics (37.79 KB, patch)
2011-07-05 05:06 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Qt build fix (26.30 KB, patch)
2011-07-06 04:27 PDT, Alexander Pavlov (apavlov)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Chromium build fix (26.29 KB, patch)
2011-07-06 05:15 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] InspectorStyleTextEditor factored out; made use of CSSPropertySourceData instead of InspectorStyleProperty (50.58 KB, patch)
2011-07-07 09:19 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Removed parameter name (50.57 KB, patch)
2011-07-07 09:21 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed (60.80 KB, patch)
2011-07-07 11:30 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-06-29 05:55:44 PDT
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
Comment 1 Alexander Pavlov (apavlov) 2011-06-29 09:34:05 PDT
Created attachment 99102 [details]
[PATCH] Suggested solution
Comment 2 WebKit Review Bot 2011-06-29 09:35:17 PDT
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.
Comment 3 Alexander Pavlov (apavlov) 2011-06-29 09:37:00 PDT
Created attachment 99103 [details]
[PATCH] Style fixed
Comment 4 WebKit Review Bot 2011-06-29 09:40:40 PDT
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
Comment 5 WebKit Review Bot 2011-06-29 09:52:06 PDT
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
Comment 6 WebKit Review Bot 2011-06-29 09:55:43 PDT
Comment on attachment 99103 [details]
[PATCH] Style fixed

Attachment 99103 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8965090
Comment 7 Alexander Pavlov (apavlov) 2011-06-30 06:07:50 PDT
Created attachment 99281 [details]
[PATCH] Chromium build fix
Comment 8 WebKit Review Bot 2011-06-30 06:28:43 PDT
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
Comment 9 Alexander Pavlov (apavlov) 2011-07-01 06:51:10 PDT
Created attachment 99467 [details]
[PATCH] Test and more heuristics added
Comment 10 WebKit Review Bot 2011-07-01 06:55:52 PDT
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
Comment 11 Early Warning System Bot 2011-07-01 06:59:18 PDT
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
Comment 12 WebKit Review Bot 2011-07-01 07:05:30 PDT
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
Comment 13 Alexander Pavlov (apavlov) 2011-07-01 07:35:52 PDT
Created attachment 99471 [details]
[PATCH] Build fix attempt
Comment 14 WebKit Review Bot 2011-07-01 08:54:48 PDT
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
Comment 15 WebKit Review Bot 2011-07-01 08:54:53 PDT
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
Comment 16 Pavel Feldman 2011-07-04 03:24:36 PDT
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?
Comment 17 Alexander Pavlov (apavlov) 2011-07-05 05:06:25 PDT
Created attachment 99703 [details]
[PATCH] Test augmented, more heuristics
Comment 18 WebKit Review Bot 2011-07-05 05:15:08 PDT
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
Comment 19 Early Warning System Bot 2011-07-05 05:17:13 PDT
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
Comment 20 WebKit Review Bot 2011-07-05 05:17:37 PDT
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
Comment 21 WebKit Review Bot 2011-07-05 05:21:59 PDT
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
Comment 22 Alexander Pavlov (apavlov) 2011-07-06 04:27:39 PDT
Created attachment 99815 [details]
[PATCH] Qt build fix
Comment 23 WebKit Review Bot 2011-07-06 04:32:40 PDT
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
Comment 24 WebKit Review Bot 2011-07-06 04:36:12 PDT
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
Comment 25 WebKit Review Bot 2011-07-06 04:44:34 PDT
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
Comment 26 Alexander Pavlov (apavlov) 2011-07-06 05:15:19 PDT
Created attachment 99818 [details]
[PATCH] Chromium build fix
Comment 27 Pavel Feldman 2011-07-07 00:09:27 PDT
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?
Comment 28 Alexander Pavlov (apavlov) 2011-07-07 09:19:02 PDT
Created attachment 99992 [details]
[PATCH] InspectorStyleTextEditor factored out; made use of CSSPropertySourceData instead of InspectorStyleProperty
Comment 29 Alexander Pavlov (apavlov) 2011-07-07 09:21:50 PDT
Created attachment 99993 [details]
[PATCH] Removed parameter name
Comment 30 WebKit Review Bot 2011-07-07 09:22:03 PDT
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.
Comment 31 Pavel Feldman 2011-07-07 09:28:14 PDT
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.
Comment 32 Alexander Pavlov (apavlov) 2011-07-07 11:30:08 PDT
Created attachment 100011 [details]
[PATCH] Comments addressed
Comment 33 Alexander Pavlov (apavlov) 2011-07-08 04:35:20 PDT
Committed r90619: <http://trac.webkit.org/changeset/90619>