Bug 63622

Summary:

Web Inspector: Adding CSS properties results in messy style rules

Product:

WebKit

Reporter:

Alexander Pavlov (apavlov) <apavlov>

Component:

Web Inspector (Deprecated)

Assignee:

Alexander Pavlov (apavlov) <apavlov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Suggested solution	none
[PATCH] Style fixed	webkit.review.bot: commit-queue-
[PATCH] Chromium build fix	webkit.review.bot: commit-queue-
[PATCH] Test and more heuristics added	webkit.review.bot: commit-queue-
[PATCH] Build fix attempt	pfeldman: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01	none
[PATCH] Test augmented, more heuristics	webkit.review.bot: commit-queue-
[PATCH] Qt build fix	webkit.review.bot: commit-queue-
[PATCH] Chromium build fix	pfeldman: review-
[PATCH] InspectorStyleTextEditor factored out; made use of CSSPropertySourceData instead of InspectorStyleProperty	none
[PATCH] Removed parameter name	pfeldman: review-
[PATCH] Comments addressed	pfeldman: review+

Alexander Pavlov (apavlov)

Reported 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

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
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Alexander Pavlov (apavlov)

Comment 1 2011-06-29 09:34:05 PDT

Created attachment 99102 [details] [PATCH] Suggested solution

WebKit Review Bot

Comment 2 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.

Alexander Pavlov (apavlov)

Comment 3 2011-06-29 09:37:00 PDT

Created attachment 99103 [details] [PATCH] Style fixed

WebKit Review Bot

Comment 4 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

WebKit Review Bot

Comment 5 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

WebKit Review Bot

Comment 6 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

Alexander Pavlov (apavlov)

Comment 7 2011-06-30 06:07:50 PDT

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

WebKit Review Bot

Comment 8 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

Alexander Pavlov (apavlov)

Comment 9 2011-07-01 06:51:10 PDT

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

WebKit Review Bot

Comment 10 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

Early Warning System Bot

Comment 11 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

WebKit Review Bot

Comment 12 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

Alexander Pavlov (apavlov)

Comment 13 2011-07-01 07:35:52 PDT

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

WebKit Review Bot

Comment 14 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

WebKit Review Bot

Comment 15 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

Pavel Feldman

Comment 16 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?

Alexander Pavlov (apavlov)

Comment 17 2011-07-05 05:06:25 PDT

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

WebKit Review Bot

Comment 18 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

Early Warning System Bot

Comment 19 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

WebKit Review Bot

Comment 20 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

WebKit Review Bot

Comment 21 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

Alexander Pavlov (apavlov)

Comment 22 2011-07-06 04:27:39 PDT

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

WebKit Review Bot

Comment 23 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

WebKit Review Bot

Comment 24 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

WebKit Review Bot

Comment 25 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

Alexander Pavlov (apavlov)

Comment 26 2011-07-06 05:15:19 PDT

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

Pavel Feldman

Comment 27 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?

Alexander Pavlov (apavlov)

Comment 28 2011-07-07 09:19:02 PDT

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

Alexander Pavlov (apavlov)

Comment 29 2011-07-07 09:21:50 PDT

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

WebKit Review Bot

Comment 30 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.

Pavel Feldman

Comment 31 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.

Alexander Pavlov (apavlov)

Comment 32 2011-07-07 11:30:08 PDT

Created attachment 100011 [details] [PATCH] Comments addressed

Alexander Pavlov (apavlov)

Comment 33 2011-07-08 04:35:20 PDT

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

Note You need to log in before you can comment on or make changes to this bug.