Bug 81737 - cssText should use shorthand notations
Summary: cssText should use shorthand notations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 82012 82028 82040 185953
Blocks: 12319 73967 82364
  Show dependency treegraph
 
Reported: 2012-03-20 21:33 PDT by Ryosuke Niwa
Modified: 2018-09-13 10:13 PDT (History)
11 users (show)

See Also:


Attachments
work in progress (11.42 KB, patch)
2012-03-21 01:14 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (16.11 KB, patch)
2012-03-21 19:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 3 (18.90 KB, patch)
2012-03-22 01:32 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 4 (9.67 KB, patch)
2012-03-22 13:41 PDT, Ryosuke Niwa
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
work in progress 5 (44.14 KB, patch)
2012-03-23 03:49 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (32.02 KB, patch)
2012-03-23 17:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added the missing BitVector change (33.20 KB, patch)
2012-03-23 17:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Another build fix (33.21 KB, patch)
2012-03-23 17:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed win build (34.29 KB, patch)
2012-03-26 15:41 PDT, Ryosuke Niwa
enrica: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-03-20 21:33:11 PDT
Takes an example from 12319. Copying some text from TextEdit in the rich format mode results in verbose inline styles such as:
<p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font-family: Helvetica; font-size: 12px; ">this is line one</p>

It could be

<p style="margin: 0px; font-family: Helvetica; font-size: 12px; ">this is line one</p>

instead.
Comment 1 Ryosuke Niwa 2012-03-21 01:14:10 PDT
Created attachment 132987 [details]
work in progress
Comment 2 Ryosuke Niwa 2012-03-21 19:25:23 PDT
Created attachment 133169 [details]
work in progress 2

There appear to be some regressions in copy & paste.
Comment 3 Ryosuke Niwa 2012-03-22 01:32:04 PDT
Created attachment 133205 [details]
work in progress 3

I just need a test now.
Comment 4 WebKit Review Bot 2012-03-22 01:34:47 PDT
Attachment 133205 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/editing/pasteboard/paste-and-s..." exit_code: 1
Source/WebCore/css/StylePropertySet.h:103:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Ryosuke Niwa 2012-03-22 11:49:12 PDT
It appears that we can do this for all cases. e.g. Firefox uses shorthand notations for cssText as well.
Comment 6 Ryosuke Niwa 2012-03-22 13:41:23 PDT
Created attachment 133337 [details]
work in progress 4

Enable shorthands for all cases.
Comment 7 WebKit Review Bot 2012-03-22 15:24:27 PDT
Comment on attachment 133337 [details]
work in progress 4

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

New failing tests:
editing/pasteboard/paste-and-sanitize.html
inspector/styles/styles-update-from-js.html
fast/css/remove-shorthand.html
editing/style/non-inheritable-styles.html
fast/css/uri-token-parsing.html
printing/page-rule-css-text.html
Comment 8 Ryosuke Niwa 2012-03-23 03:49:54 PDT
Created attachment 133453 [details]
work in progress 5
Comment 9 Ryosuke Niwa 2012-03-23 17:17:20 PDT
Created attachment 133590 [details]
Patch
Comment 10 Early Warning System Bot 2012-03-23 17:20:13 PDT
Comment on attachment 133590 [details]
Patch

Attachment 133590 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12117885
Comment 11 Early Warning System Bot 2012-03-23 17:21:22 PDT
Comment on attachment 133590 [details]
Patch

Attachment 133590 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12117887
Comment 12 Gyuyoung Kim 2012-03-23 17:22:14 PDT
Comment on attachment 133590 [details]
Patch

Attachment 133590 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12122879
Comment 13 Build Bot 2012-03-23 17:24:08 PDT
Comment on attachment 133590 [details]
Patch

Attachment 133590 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12123847
Comment 14 Build Bot 2012-03-23 17:30:58 PDT
Comment on attachment 133590 [details]
Patch

Attachment 133590 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12116920
Comment 15 Ryosuke Niwa 2012-03-23 17:36:25 PDT
Created attachment 133596 [details]
Added the missing BitVector change
Comment 16 WebKit Review Bot 2012-03-23 17:43:50 PDT
Comment on attachment 133596 [details]
Added the missing BitVector change

Attachment 133596 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12117902
Comment 17 Early Warning System Bot 2012-03-23 17:51:28 PDT
Comment on attachment 133596 [details]
Added the missing BitVector change

Attachment 133596 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12119007
Comment 18 Early Warning System Bot 2012-03-23 17:56:15 PDT
Comment on attachment 133596 [details]
Added the missing BitVector change

Attachment 133596 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12097058
Comment 19 Ryosuke Niwa 2012-03-23 17:58:07 PDT
Created attachment 133601 [details]
Another build fix
Comment 20 WebKit Review Bot 2012-03-23 18:13:26 PDT
Comment on attachment 133601 [details]
Another build fix

Attachment 133601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12122905
Comment 21 Build Bot 2012-03-23 18:30:10 PDT
Comment on attachment 133601 [details]
Another build fix

Attachment 133601 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12122912
Comment 22 Eric Seidel (no email) 2012-03-26 15:00:57 PDT
Comment on attachment 133601 [details]
Another build fix

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

Do we know what the perf effect of this change is?

> Source/WebCore/css/StylePropertySet.cpp:730
> +            // Don't use CSSPropertyFont because old UAs can't recognize them but are important for editing.

Is this really still the case? :)
Comment 23 Ryosuke Niwa 2012-03-26 15:03:44 PDT
(In reply to comment #22)
> (From update of attachment 133601 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133601&action=review
> 
> Do we know what the perf effect of this change is?

I don't think there's any perf. implications because we didn't even use StringBuilder before my patch for https://bugs.webkit.org/show_bug.cgi?id=82028 was landed.

> > Source/WebCore/css/StylePropertySet.cpp:730
> > +            // Don't use CSSPropertyFont because old UAs can't recognize them but are important for editing.
> 
> Is this really still the case? :)

Yes. Unfortunately, some old versions of mail clients support as little CSS as MSIE4 did :(  We even used to generate font elements for a very old mail client that didn't support CSS at all.
Comment 24 Enrica Casucci 2012-03-26 15:29:53 PDT
Comment on attachment 133601 [details]
Another build fix

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

The patch looks good to me. Thanks for doing this :-)

> Source/WTF/wtf/BitVector.h:170
> +

Unnecessary empty line.
Comment 25 Enrica Casucci 2012-03-26 15:33:28 PDT
Comment on attachment 133601 [details]
Another build fix

Please fix the build breaks. The rest of the patch looks good.
Comment 26 Ryosuke Niwa 2012-03-26 15:41:35 PDT
Created attachment 133909 [details]
Fixed win build
Comment 27 Enrica Casucci 2012-03-26 15:53:13 PDT
Comment on attachment 133909 [details]
Fixed win build

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

r+ with the Linux build fix and the ChangeLog entry.

> Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:302
>      ?retrieveCallerFromVMCode@Interpreter@JSC@@QBE?AVJSValue@2@PAVExecState@2@PAVJSFunction@2@@Z

You need a ChangeLog entry for this change.
Comment 28 WebKit Review Bot 2012-03-26 16:38:35 PDT
Comment on attachment 133909 [details]
Fixed win build

Attachment 133909 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12145156
Comment 29 Ryosuke Niwa 2012-03-26 16:50:42 PDT
Comment on attachment 133909 [details]
Fixed win build

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

> Source/WebCore/css/StylePropertySet.cpp:777
> +        case CSSPropertyWebkitMaskPositionX:
> +        case CSSPropertyWebkitMaskPositionY:
> +            shorthandPropertyID = CSSPropertyWebkitMaskPosition;
> +            break;
> +        case CSSPropertyWebkitMaskRepeatX:
> +        case CSSPropertyWebkitMaskRepeatY:
> +            shorthandPropertyID = CSSPropertyWebkitMaskRepeat;

Huh, these properties need to be merged with the following properties for CSSPropertyWebkitMask.
It's causing LayoutTests/fast/css/remove-shorthand.html to fail now.
It seems like something has changed since last week :\
Comment 30 Ryosuke Niwa 2012-03-26 16:59:37 PDT
Committed r112177: <http://trac.webkit.org/changeset/112177>
Comment 31 mitz 2012-03-26 18:22:48 PDT
(In reply to comment #30)
> Committed r112177: <http://trac.webkit.org/changeset/112177>

Is this what caused fast/dom/css-dom-read-2.html to fail?
Comment 32 Ryosuke Niwa 2012-03-26 18:27:41 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > Committed r112177: <http://trac.webkit.org/changeset/112177>
> 
> Is this what caused fast/dom/css-dom-read-2.html to fail?

So it seems. Will rebaseline. It's odd that I didn't see this failure locally last week :\
Comment 33 Ryosuke Niwa 2012-03-26 18:32:46 PDT
Chromium build fix landed in http://trac.webkit.org/changeset/112185, and rebaselined the test in http://trac.webkit.org/changeset/112188.