Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746793002:
Added core/css/properties/README.md. (Closed)
DescriptionAdded core/css/properties/README.md.
Added core/css/properties/README.md, detailing the purpose and status of
CSS property APIs as well as the steps involved in adding a new
property API.
BUG=545324
Review-Url: https://codereview.chromium.org/2746793002
Cr-Commit-Position: refs/heads/master@{#457305}
Committed: https://chromium.googlesource.com/chromium/src/+/780472d59ff68a6222b6a36cc145e11ea6ba1a71
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed reviewer comments. #Patch Set 3 : Addressed reviewer comments. #Messages
Total messages: 14 (6 generated)
bugsnash@chromium.org changed reviewers: + rvera@chromium.org
lgtm
bugsnash@chromium.org changed reviewers: + alancutter@chromium.org
+alancutter@ for OWNERS
https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/properties/README.md (right): https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:16: Status: Eventually, all logic pertaining to a single property should be found only We should have a timestamp attached to the status so it'll be obvious when this doc gets out of date. https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:24: ## How to add a new property API Is there an example CL we can link to that follows all these steps? https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:36: for details) This doc ought to mention the api_class flag as a requirement for using the CSSPropertyAPI.
https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/properties/README.md (right): https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:16: Status: Eventually, all logic pertaining to a single property should be found only On 2017/03/13 at 04:53:01, alancutter wrote: > We should have a timestamp attached to the status so it'll be obvious when this doc gets out of date. Done. https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:24: ## How to add a new property API On 2017/03/13 at 04:53:01, alancutter wrote: > Is there an example CL we can link to that follows all these steps? Hm. All the CLs we've written are to create APIs for existing properties (and move some parsing logic out of the parser to the new API). These steps describe how to add a new property that was not previously implemented, which hasn't happened in the new world yet. Do you think it would be useful to link one of the CLs that move an existing property to the new API? https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:36: for details) On 2017/03/13 at 04:53:01, alancutter wrote: > This doc ought to mention the api_class flag as a requirement for using the CSSPropertyAPI. I have added a mention that api_class and api_methods are required (but not explained how to add them). This is because it seems like the requirements of these flags might change, so I want to minimize the information that might go out of date in this doc. Given that the specifics are described in CSSProperties.json5 I figure it doesn't need to be repeated here.
lgtm https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/properties/README.md (right): https://codereview.chromium.org/2746793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/properties/README.md:24: ## How to add a new property API On 2017/03/15 at 22:37:28, Bugs Nash wrote: > On 2017/03/13 at 04:53:01, alancutter wrote: > > Is there an example CL we can link to that follows all these steps? > > Hm. All the CLs we've written are to create APIs for existing properties (and move some parsing logic out of the parser to the new API). These steps describe how to add a new property that was not previously implemented, which hasn't happened in the new world yet. Do you think it would be useful to link one of the CLs that move an existing property to the new API? Yes, I think a concrete CL showing places in the code is quite valuable. Definitely make it clear it's migrating a property when linking to it rather than adding a new one. For someone familiar with the old system it would probably serve well as a Rosetta stone.
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rvera@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2746793002/#ps40001 (title: "Addressed reviewer comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489625835611730,
"parent_rev": "524166ab8948e4348526fda14e5a834b45d730fe", "commit_rev":
"780472d59ff68a6222b6a36cc145e11ea6ba1a71"}
Message was sent while issue was closed.
Description was changed from ========== Added core/css/properties/README.md. Added core/css/properties/README.md, detailing the purpose and status of CSS property APIs as well as the steps involved in adding a new property API. BUG=545324 ========== to ========== Added core/css/properties/README.md. Added core/css/properties/README.md, detailing the purpose and status of CSS property APIs as well as the steps involved in adding a new property API. BUG=545324 Review-Url: https://codereview.chromium.org/2746793002 Cr-Commit-Position: refs/heads/master@{#457305} Committed: https://chromium.googlesource.com/chromium/src/+/780472d59ff68a6222b6a36cc145... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/780472d59ff68a6222b6a36cc145... |
