Bug 189694 - Implement CSS Custom Properties and Values Skeleton
Summary: Implement CSS Custom Properties and Values Skeleton
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 189729
Blocks: 189692
  Show dependency treegraph
 
Reported: 2018-09-18 00:05 PDT by Justin Michaud
Modified: 2018-09-20 11:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (34.06 KB, patch)
2018-09-18 00:49 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (8.24 MB, application/zip)
2018-09-18 02:44 PDT, Build Bot
no flags Details
Patch (36.88 KB, patch)
2018-09-18 18:58 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (37.99 KB, patch)
2018-09-18 20:07 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (38.23 KB, patch)
2018-09-19 08:13 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (38.05 KB, patch)
2018-09-19 08:45 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (36.62 KB, patch)
2018-09-19 12:18 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (37.85 KB, patch)
2018-09-19 15:36 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (37.72 KB, patch)
2018-09-20 10:47 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2018-09-18 00:05:30 PDT
Implement a feature flag and DOM bindings for the CSS Custom Properties and Values API
Comment 1 Justin Michaud 2018-09-18 00:49:13 PDT
Created attachment 350009 [details]
Patch
Comment 2 Justin Michaud 2018-09-18 00:51:28 PDT
For reviewer: The bindings code is particularly scary/messy/likely to break something, I would appreciate some help with it. Also, is Document the right place to store the list of registered properties? I was planning on adding code to StyleResolver::applyProperty to handle inherits and initialValue after this patch is sorted out, which does have access to a Document, but I wonder if there is a cleaner way.
Comment 3 Build Bot 2018-09-18 00:53:14 PDT
Comment on attachment 350009 [details]
Patch

Attachment 350009 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9253990

New failing tests:
(JS) JSTestEnabledBySetting.cpp
(JS) JSTestGenerateIsReachable.cpp
(JS) JSTestInterface.cpp
(JS) JSTestNode.cpp
(JS) JSTestObj.cpp
Comment 4 Build Bot 2018-09-18 02:44:52 PDT
Comment on attachment 350009 [details]
Patch

Attachment 350009 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9254260

New failing tests:
storage/indexeddb/keyrange.html
storage/indexeddb/keyrange-private.html
css3/flexbox/align-absolute-child.html
fast/dom/DOMURL/check-instanceof-domurl-functions.html
Comment 5 Build Bot 2018-09-18 02:44:54 PDT
Created attachment 350011 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 6 Sam Weinig 2018-09-18 11:37:03 PDT
Comment on attachment 350009 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        * bindings/scripts/CodeGeneratorJS.pm:

Please add bindings tests for this new functionality.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3100
> +        #next if ($operation->isStatic);

Please don't check in commented out code.

> Source/WebCore/css/DOMCSSNamespace.idl:37
> +    [CallWith=Document,EnabledAtRuntime=CSSCustomPropertiesAndValues] static void registerProperty(DOMCSSPropertyDescriptor descriptor);

This should probably be in it's own IDL file using a partial interface.

> Source/WebCore/dom/Document.cpp:8224
> +    if (m_CSSRegisteredPropertySet.contains(prop.name))
> +        return false;
> +    m_CSSRegisteredPropertySet.add(prop.name, prop);
> +    return true;

This should use HashMap::ensure to avoid the double hash lookup. Also, is the return value necessary? It doesn't seem like the caller uses it.

> Source/WebCore/dom/Document.h:1496
> +    bool registerCSSProperty(const CSSRegisteredProperty&);

This could probably take an r-value CSSRegisteredProperty.

> Source/WebKit/Shared/WebPreferences.yaml:1358
> +  humanReadableName: "CSS Custom Properties and Values Api Level 1"
> +  humanReadableDescription: "Enable CSS Properties and Values Api Level 1"

"Api" -> "API". Also, the "Level" 1 is probably not necessary.
Comment 7 Justin Michaud 2018-09-18 18:58:24 PDT
Created attachment 350085 [details]
Patch
Comment 8 Justin Michaud 2018-09-18 19:00:09 PDT
I responded to the feedback about the patch. I also moved the code generator changes out to a new patch, which I will upload shortly. The feature flag will not work properly until the code generator changes land.
Comment 9 Justin Michaud 2018-09-18 20:07:56 PDT
Created attachment 350089 [details]
Patch
Comment 10 Justin Michaud 2018-09-18 20:12:10 PDT
https://bugs.webkit.org/show_bug.cgi?id=189729 is the bug for the bindings
Comment 11 Justin Michaud 2018-09-19 08:13:02 PDT
Created attachment 350118 [details]
Patch
Comment 12 Justin Michaud 2018-09-19 08:45:22 PDT
Created attachment 350120 [details]
Patch
Comment 13 Simon Fraser (smfr) 2018-09-19 10:37:31 PDT
We should import https://github.com/web-platform-tests/wpt/tree/master/css/css-properties-values-api at some point.
Comment 14 Simon Fraser (smfr) 2018-09-19 10:50:09 PDT
Comment on attachment 350120 [details]
Patch

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

> Source/WebCore/css/DOMCSSCustomPropertyDescriptor.h:15
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following disclaimer in
> + *    the documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Motorola Mobility Inc. nor the names of its
> + *    contributors may be used to endorse or promote products derived from this
> + *    software without specific prior written permission.

Wrong license.

> Source/WebCore/css/DOMCSSCustomPropertyDescriptor.idl:15
> +* 1. Redistributions of source code must retain the above copyright
> +*    notice, this list of conditions and the following disclaimer.
> +* 2. Redistributions in binary form must reproduce the above
> +*    copyright notice, this list of conditions and the following disclaimer in
> +*    the documentation and/or other materials provided with the distribution.
> +* 3. Neither the name of Motorola Mobility Inc. nor the names of its
> +*    contributors may be used to endorse or promote products derived from this
> +*    software without specific prior written permission.

Wrong license.

> Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp:15
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following disclaimer in
> + *    the documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of Motorola Mobility Inc. nor the names of its
> + *    contributors may be used to endorse or promote products derived from this
> + *    software without specific prior written permission.

Wrong license.

> Source/WebCore/css/DOMCSSRegisterCustomProperty.h:2
> + * Copyright (C) 2012 Motorola Mobility Inc. All rights reserved.

Apple

> Source/WebCore/dom/Document.h:2029
> +    HashMap<String, CSSRegisteredCustomProperty> m_CSSRegisteredPropertySet;

Probably want a HashMap<String, std::unique_ptr<CSSRegisteredCustomProperty>> at some point.
Comment 15 Justin Michaud 2018-09-19 12:18:17 PDT
Created attachment 350141 [details]
Patch
Comment 16 Justin Michaud 2018-09-19 13:40:02 PDT
(In reply to Simon Fraser (smfr) from comment #13)
> We should import
> https://github.com/web-platform-tests/wpt/tree/master/css/css-properties-
> values-api at some point.

https://bugs.webkit.org/show_bug.cgi?id=189764
Comment 17 Justin Michaud 2018-09-19 15:36:23 PDT
Created attachment 350153 [details]
Patch
Comment 18 Justin Michaud 2018-09-20 10:47:30 PDT
Created attachment 350231 [details]
Patch
Comment 19 WebKit Commit Bot 2018-09-20 11:23:29 PDT
Comment on attachment 350231 [details]
Patch

Rejecting attachment 350231 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 350231, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=350231&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=189694&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 350231 from bug 189694.
Fetching: https://bugs.webkit.org/attachment.cgi?id=350231
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/css-custom-properties-api/registerProperty-expected.txt
	A	LayoutTests/css-custom-properties-api/registerProperty.html
	A	Source/WebCore/css/CSSRegisteredCustomProperty.h
	A	Source/WebCore/css/DOMCSSCustomPropertyDescriptor.h
	A	Source/WebCore/css/DOMCSSCustomPropertyDescriptor.idl
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.h
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.idl
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: b9d443dd00ccfedd6fcb922f344e2ff85415d675 and refs/remotes/origin/master differ, using rebase:
:040000 040000 d046daf9cd393c65af5c4ae80c7db1d7c3c7650e 664ea62087e4092795ab709d5b041447814487fe M	LayoutTests
:040000 040000 d2c09978b4d1d5ccdd0f6ae18813226993bfde8f d64d3e018d269ac1c35ce279e79f5f52f5547f36 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	LayoutTests/css-custom-properties-api/registerProperty-expected.txt
	A	LayoutTests/css-custom-properties-api/registerProperty.html
	A	Source/WebCore/css/CSSRegisteredCustomProperty.h
	A	Source/WebCore/css/DOMCSSCustomPropertyDescriptor.h
	A	Source/WebCore/css/DOMCSSCustomPropertyDescriptor.idl
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.h
	A	Source/WebCore/css/DOMCSSRegisterCustomProperty.idl
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: b9d443dd00ccfedd6fcb922f344e2ff85415d675 and refs/remotes/origin/master differ, using rebase:
:040000 040000 d046daf9cd393c65af5c4ae80c7db1d7c3c7650e 664ea62087e4092795ab709d5b041447814487fe M	LayoutTests
:040000 040000 d2c09978b4d1d5ccdd0f6ae18813226993bfde8f d64d3e018d269ac1c35ce279e79f5f52f5547f36 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   a92813138ca..db7de6d33eb  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 236266 = a92813138ca971a1efd5654eb374a1f51a74baa7
r236269 = 6a5b1c4cd780c104178671fc3cede538d570259b
r236270 = ff1ca6c5e9acaba2eb6d522d1cb06a9acf0c46a0
r236271 = db7de6d33eb03a60876c1e45a5f0b3cac88b76b7
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Total errors found: 0 in 2 files

Full output: https://webkit-queues.webkit.org/results/9284354
Comment 20 WebKit Commit Bot 2018-09-20 11:58:04 PDT
Comment on attachment 350231 [details]
Patch

Clearing flags on attachment: 350231

Committed r236273: <https://trac.webkit.org/changeset/236273>
Comment 21 WebKit Commit Bot 2018-09-20 11:58:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2018-09-20 11:59:27 PDT
<rdar://problem/44649888>