Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533673002:
Added CSSPropertyAPI and CSS padding properties which implement this API (Closed)
DescriptionAdded CSSPropertyAPI and CSS padding properties which implement this API
Added the CSSPropertyAPI for a single method parseSingleValue. Made
CSSPaddingProperties which implements this API for a few css
properties. Added CSS property descriptor which uses a struct to store
function pointers for the property's functions.
BUG=668012
Committed: https://crrev.com/13f6e06ed6b80d003e7298b501e57a58695ca951
Cr-Commit-Position: refs/heads/master@{#438057}
Patch Set 1 #Patch Set 2 : fix branch #
Total comments: 14
Patch Set 3 : Added comments and made code neater #
Total comments: 1
Patch Set 4 : Fixed some formatting #
Total comments: 32
Patch Set 5 : Made code neater, moved get descriptor code to a macro #Patch Set 6 : removed unnecessary line #Patch Set 7 : review comments #
Total comments: 45
Patch Set 8 : Renamed CSSPaddingProperties, added comments and style #Patch Set 9 : Polishing code comments #
Total comments: 10
Patch Set 10 : Fixed comments and added is_pod static assert for descriptor #
Total comments: 4
Patch Set 11 : Fixed comments #
Total comments: 5
Patch Set 12 : fixed comments #
Total comments: 6
Patch Set 13 : Removed id variable in class #
Total comments: 2
Patch Set 14 : added TODO #Patch Set 15 : renamed isValid to temporaryCanReadValue #
Total comments: 4
Patch Set 16 : Removed subclasses of WebkitPadding #Patch Set 17 : format #Patch Set 18 : format #Dependent Patchsets: Messages
Total messages: 78 (38 generated)
aazzam@google.com changed reviewers: + sashab@chromium.org
The CQ bit was checked by aazzam@google.com
The CQ bit was unchecked by aazzam@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2521173002 Patch 40001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by aazzam@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal :) https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:15: class CSSPropertyAPI { Look at other public APIs -- twitter, facebook, google+, thetvdb put a comment here https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:19: NOTREACHED(); Try not implement these https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:18: static CSSPropertyDescriptor cssPropertyDescriptors[800]; CSSPropertyNames.h -- kNumberOfProperties, MAX_PROPERTY_ID https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:20: static void registerDescriptors() { TODO(aazzam): generate this function https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:21: static bool isCalled = false; comments explaining this and the loop underneath https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:40: remove newline https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:45: cssPropertyDescriptors[id] = descriptor; DCHECK(id > 0) DCHECK(id <= kNumberOfProperties) https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:46: } add newline https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:12: const CSSValue* (*parseSingleValue)(CSSParserTokenRange&, Comments about matching the API methods https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:15: bool isValidProperty; _isValidProperty - shows its private comment explaining its separate and not to be used https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:18: const CSSPropertyDescriptor& get(CSSPropertyID); Move to be static methocs on CSSPropertyDescriptor CSSPropertyDescriptor::get(WidthValue) https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:19: CSSPropertyDescriptor getInvalidDescriptor(); Move to static method in cpp file OR ideally Anonymous namespace namespace blink{ namespace { void foo() { ..} } f;rijeglierjgire foo() } https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: void add(CSSPropertyDescriptor, CSSPropertyID); Move to static CSSPropertyDescriptor::add() https://codereview.chromium.org/2533673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:25: a.parseSingleValue = &T::parseSingleValue; Add ASSERT(a.parse != CSSPropertyAPI::parse)
nainar@chromium.org changed reviewers: + alancutter@chromium.org, nainar@chromium.org
Hi Anna, The patch lg*m so far. Other than one issue and I have commented on that inline. Please do run the trybots on this patch. Running git cl try on your local patch should suffice. Or you can click on the CQ Dry Run button in the UI here. One more thing - while I understand your patch has no behaviour change, would adding Unit Tests be something worth considering here? Sasha can maybe provide more context here. Also adding Alan as a reviewer while Sasha is out. :) https://codereview.chromium.org/2533673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:19: bool isValidProperty; should this maybe be m_isValidProperty?
The CQ bit was checked by aazzam@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3433: parse Avoid manual line wrapping, let the formatting tool do it for you. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3436: those properties will be taken out of the switch statement. */ Style guide says to use /**/ or // consistently. These should use // for consistency. Ditto this for all other parts of the patch. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:36: } I don't think this class is necessary. Without it the "subclasses" will hit compile errors for any methods they don't implement saving you the need to DCHECK anything. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:15: CSSPropertyDescriptor a; Avoid names like "a" as mentioned before. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:19: } End braces of namespaces should have a comment with the namespace they belong to and have a blank line before them. See existing source files. Ditto for other parts of this patch. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:23: static void registerDescriptors() { Rename to ensureDescriptorsRegistered(). https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:25: // once Blink style for comments is for sentences to end in a full stop. Ditto for all other parts of this patch. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:32: // which haven't been implemented yet I don't think this comment is necessary. Comments should say *why* code exists rather than *what* it does. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:37: // TODO(aazzam): Generate this function I'd extend the TODO to be: Generate this function using compile time literals instead of add() and getDescriptor(). Initialising the array can be done at compile time with zero runtime overhead. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:6: #include "core/css/CSSValue.h" CSSValue can just be forward declared. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:17: CSSPropertyID (*getID)(); Why is this a getter and not just "CSSPropertyID id"? https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:19: bool m_isValidProperty; "m_" is for private class data members. For simple structs like this the "m_" is not required. I would rename this to isValid as well because it's about the descriptor rather than the property. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:21: static void add(CSSPropertyDescriptor, CSSPropertyID); Does this need to be exposed? https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:26: CSSPropertyDescriptor a; Avoid using names like "a" for objects with specific purposes/types. Name it "descriptor". https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:35: } Does this need to be in the header file?
https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:43: #include "core/css/properties/CSSPaddingProperties.h" This include is not necessary. The core parser logic is being separated from knowing about individual properties' implementations.
To add to nainar's comment about unit tests: I would be okay for these changes to come without new tests. Fundamentally they are just moving code from one file to another so it's hard to think of what could be tested here (assuming the properties affected already have sufficient test coverage).
The CQ bit was checked by aazzam@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal :) https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:43: #include "core/css/properties/CSSPaddingProperties.h" On 2016/11/29 at 06:54:04, alancutter wrote: > This include is not necessary. The core parser logic is being separated from knowing about individual properties' implementations. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3433: parse On 2016/11/29 at 06:51:49, alancutter wrote: > Avoid manual line wrapping, let the formatting tool do it for you. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3436: those properties will be taken out of the switch statement. */ On 2016/11/29 at 06:51:49, alancutter wrote: > Style guide says to use /**/ or // consistently. These should use // for consistency. > Ditto this for all other parts of the patch. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:36: } On 2016/11/29 at 06:51:49, alancutter wrote: > I don't think this class is necessary. Without it the "subclasses" will hit compile errors for any methods they don't implement saving you the need to DCHECK anything. I think we should keep the class since it's a way of seeing all the methods that our API has. It's also needed in the future where we have methods which aren't implemented by every property, and we'll want to have a default implementation. I've changed the code now so that we don't need to use NOTREACHED() on our not implemented functions. https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:15: CSSPropertyDescriptor a; On 2016/11/29 at 06:51:49, alancutter wrote: > Avoid names like "a" as mentioned before. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:19: } On 2016/11/29 at 06:51:50, alancutter wrote: > End braces of namespaces should have a comment with the namespace they belong to and have a blank line before them. See existing source files. > Ditto for other parts of this patch. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:23: static void registerDescriptors() { On 2016/11/29 at 06:51:49, alancutter wrote: > Rename to ensureDescriptorsRegistered(). done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:25: // once On 2016/11/29 at 06:51:49, alancutter wrote: > Blink style for comments is for sentences to end in a full stop. > Ditto for all other parts of this patch. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:32: // which haven't been implemented yet On 2016/11/29 at 06:51:49, alancutter wrote: > I don't think this comment is necessary. > Comments should say *why* code exists rather than *what* it does. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:37: // TODO(aazzam): Generate this function On 2016/11/29 at 06:51:49, alancutter wrote: > I'd extend the TODO to be: Generate this function using compile time literals instead of add() and getDescriptor(). > > Initialising the array can be done at compile time with zero runtime overhead. done - added the macros https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:6: #include "core/css/CSSValue.h" On 2016/11/29 at 06:51:50, alancutter wrote: > CSSValue can just be forward declared. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:17: CSSPropertyID (*getID)(); On 2016/11/29 at 06:51:50, alancutter wrote: > Why is this a getter and not just "CSSPropertyID id"? done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:19: bool m_isValidProperty; On 2016/11/29 at 06:51:50, alancutter wrote: > "m_" is for private class data members. For simple structs like this the "m_" is not required. > > I would rename this to isValid as well because it's about the descriptor rather than the property. done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:21: static void add(CSSPropertyDescriptor, CSSPropertyID); On 2016/11/29 at 06:51:50, alancutter wrote: > Does this need to be exposed? get() is used in CSSPropertyParser so it needs to be exposed - i put a comment in the code explaining this. I've moved add() outside of the struct :) https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:26: CSSPropertyDescriptor a; On 2016/11/29 at 06:51:50, alancutter wrote: > Avoid using names like "a" for objects with specific purposes/types. Name it "descriptor". done https://codereview.chromium.org/2533673002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:35: } On 2016/11/29 at 06:51:50, alancutter wrote: > Does this need to be in the header file? as far as I know, templates do need to be in the header fine
https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.cpp (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.cpp:41: } These implementations are identical, they should inherit from the one single implementation. Note that code deduplication can have instruction cache performance benefits. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:15: // An API for CSS properties which allows you to call functions on properties "Allows you to call property specific functions" https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:17: // properties, provide a default implementation below. The last sentence is questionable. It would be implicit if you stated that the specific properties were subclasses of CSSPropertyAPI. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:13: { X::parseSingleValue, X::id, Y } Is Y ever not true? https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:12: // This struct should contain function pointers matching those declared in s/This struct should contain/contains/ https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:14: // template below. This sentence is out of date. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:22: bool isValid; What "internal" means is very unclear here. Use private and friend keywords to make access restrictions explicit. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:25: // in the parser This is implicit in being a public method, no need for this comment.
A few small naming and comments comments Other than that lg*m :D https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Rename to CSSPropertyAPIPadding.h Ctrl+Shift+F - global find + replace https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:6: #define CSSPaddingProperties_h Change these after rename https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:9: #include "core/css/parser/CSSPropertyParserHelpers.h" Try forward-declare CSSParserContext as well and remove this include https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:11: #include "platform/Length.h" Might not be needed anymore https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:11: #include "platform/Length.h" Might not be needed anymore https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:18: STATIC_ONLY(CSSPropertyAPIWebkitPaddingStart); Try put this on CSSPropertyAPI and remove it from here To check if it works, try add a non-static method - you should get a compile error void foo() {}; https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:18: Remove newline below Try reword to express that we will use this API to represent ALL property-specific logic inside the blink style engine, with the eventual goal of having no property logic outside of these classes Add more instructions for using it: - make a class that inherits from this class and implements all the static methods (see existing child classes as examples) - add new methods and static variables here (with optional default implementations) if you want them implemented for all properties AND update getDescriptor GET_DESCRIPTOR etc https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:21: static const CSSPropertyID id; // A unique ID for the property. // TODO(aazzam): Remove this once all callsites have been removed. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:23: // Parses the CSS property and returns a CSS Value. or null if it could not be parsed. https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:7: #include "core/CSSPropertyNames.h" Remove https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:12: #define GET_DESCRIPTOR(X, Y) \ comment and the one below instructions on how to update when a new method is added https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:20: GET_DESCRIPTOR(CSSPropertyAPIWebkitPaddingEnd, true), Remove 'true' argumetn now :D https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:27: switch (id) { TODO(azzam): we are using hard-coded indexes for the moment because. .. later generate this switch statement or return cssPropertyDescriptors[id] https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:6: #include "core/css/properties/CSSPropertyAPI.h" Not needed yayyyyy https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:16: const CSSValue* (*parseSingleValue)(CSSParserTokenRange&, // These methods match the declarations in rnefn file https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:25: // in the parser full stop You can call this function to get the descriptor for a property, and then call methods.... also not parser specific :)
The CQ bit was checked by aazzam@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aazzam@google.com changed reviewers: + rune@opera.com - alancutter@chromium.org, nainar@chromium.org, sashab@chromium.org
https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.cpp (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.cpp:41: } On 2016/11/30 at 23:46:54, alancutter wrote: > These implementations are identical, they should inherit from the one single implementation. > Note that code deduplication can have instruction cache performance benefits. done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/30 at 23:48:39, sashab wrote: > Rename to CSSPropertyAPIPadding.h > > Ctrl+Shift+F - global find + replace done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:6: #define CSSPaddingProperties_h On 2016/11/30 at 23:48:39, sashab wrote: > Change these after rename done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:9: #include "core/css/parser/CSSPropertyParserHelpers.h" On 2016/11/30 at 23:48:39, sashab wrote: > Try forward-declare CSSParserContext as well and remove this include done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:11: #include "platform/Length.h" On 2016/11/30 at 23:48:39, sashab wrote: > Might not be needed anymore done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPaddingProperties.h:18: STATIC_ONLY(CSSPropertyAPIWebkitPaddingStart); On 2016/11/30 at 23:48:39, sashab wrote: > Try put this on CSSPropertyAPI and remove it from here > > To check if it works, try add a non-static method - you should get a compile error > > void foo() {}; done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:15: // An API for CSS properties which allows you to call functions on properties On 2016/11/30 at 23:46:54, alancutter wrote: > "Allows you to call property specific functions" done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:17: // properties, provide a default implementation below. On 2016/11/30 at 23:46:54, alancutter wrote: > The last sentence is questionable. It would be implicit if you stated that the specific properties were subclasses of CSSPropertyAPI. done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:18: On 2016/11/30 at 23:48:39, sashab wrote: > Remove newline below > > Try reword to express that we will use this API to represent ALL property-specific logic inside the blink style engine, with the eventual goal of having no property logic outside of these classes > > Add more instructions for using it: > - make a class that inherits from this class and implements all the static methods (see existing child classes as examples) > - add new methods and static variables here (with optional default implementations) if you want them implemented for all properties AND update getDescriptor GET_DESCRIPTOR etc done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:21: static const CSSPropertyID id; On 2016/11/30 at 23:48:39, sashab wrote: > // A unique ID for the property. > // TODO(aazzam): Remove this once all callsites have been removed. done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:23: // Parses the CSS property and returns a CSS Value. On 2016/11/30 at 23:48:39, sashab wrote: > or null if it could not be parsed. done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:7: #include "core/CSSPropertyNames.h" On 2016/11/30 at 23:48:39, sashab wrote: > Remove done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:12: #define GET_DESCRIPTOR(X, Y) \ On 2016/11/30 at 23:48:39, sashab wrote: > comment and the one below > instructions on how to update when a new method is added done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:13: { X::parseSingleValue, X::id, Y } On 2016/11/30 at 23:46:54, alancutter wrote: > Is Y ever not true? good point :) https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:20: GET_DESCRIPTOR(CSSPropertyAPIWebkitPaddingEnd, true), On 2016/11/30 at 23:48:39, sashab wrote: > Remove 'true' argumetn now :D done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:27: switch (id) { On 2016/11/30 at 23:48:39, sashab wrote: > TODO(azzam): we are using hard-coded indexes for the moment because. .. later generate this switch statement or return cssPropertyDescriptors[id] done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:6: #include "core/css/properties/CSSPropertyAPI.h" On 2016/11/30 at 23:48:39, sashab wrote: > Not needed yayyyyy done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:12: // This struct should contain function pointers matching those declared in On 2016/11/30 at 23:46:54, alancutter wrote: > s/This struct should contain/contains/ done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:14: // template below. On 2016/11/30 at 23:46:54, alancutter wrote: > This sentence is out of date. done https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:16: const CSSValue* (*parseSingleValue)(CSSParserTokenRange&, On 2016/11/30 at 23:48:39, sashab wrote: > // These methods match the declarations in rnefn file I think i already am saying this in the comment above? https://codereview.chromium.org/2533673002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:25: // in the parser On 2016/11/30 at 23:48:39, sashab wrote: > full stop > > You can call this function to get the descriptor for a property, and then call methods.... > also not parser specific :) ^ see alan's comment
Description was changed from ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for two of the methods - parseSingle Value and getID. Made CSSPaddingProperties which implement this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 ========== to ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 ==========
aazzam@google.com changed reviewers: + ojan@chromium.org
aazzam@google.com changed reviewers: + sashab@chromium.org - rune@opera.com
Love the comments!! Looking super polished. Just a couple of nits, happy for ojan to look at this though :) https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3434: // statement. Awesome https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:30: // CSSPropertyDescriptor.h. Awesome awesome awesome! \o/ https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:36: // TODO(aazzam): Remove this once all callsites have been removed Nit - full stop at the end :) https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:14: { X::parseSingleValue, X::id, true } Add a static_assert for is_pod to CSSPropertyDescriptor to make sure this always works :) Love the comments in this file https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:36: // hold invalid descriptors for methods which haven't been implemented yet. Fix newline :) https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:22: static const CSSPropertyDescriptor& get(CSSPropertyID); get() method needs a comment since we want people to call it :)
Description was changed from ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 ========== to ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 ==========
aazzam@google.com changed reviewers: - sashab@chromium.org
Thanks Sasha, I fixed those things :) would like it if you could review for sign-off ojan :) https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:36: // TODO(aazzam): Remove this once all callsites have been removed On 2016/12/05 at 23:58:12, sashab wrote: > Nit - full stop at the end :) done https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:14: { X::parseSingleValue, X::id, true } On 2016/12/05 at 23:58:12, sashab wrote: > Add a static_assert for is_pod to CSSPropertyDescriptor to make sure this always works :) > > Love the comments in this file done https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:36: // hold invalid descriptors for methods which haven't been implemented yet. On 2016/12/05 at 23:58:12, sashab wrote: > Fix newline :) done https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:22: static const CSSPropertyDescriptor& get(CSSPropertyID); On 2016/12/05 at 23:58:12, sashab wrote: > get() method needs a comment since we want people to call it :) done
sashab@chromium.org changed reviewers: + sashab@chromium.org
Some more tiny stuff sorry :) But looking great so far https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: // only. This isn't exactly for internal use only anymore :) So maybe put something like // Stores whether or not this descriptor is for a valid property. Do not access the contents of this descriptor unless this value is true. https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:22: // Returns a CSSPropertyDescriptor given a CSSPropertyID. Nit - add a newline before this comment, showing its a separate method :) Also maybe expand this comment a little, e.g.: Returns the corresponding CSSPropertyDescriptor for a given CSSPropertyID. Use this function to access the API for a property. Returns a descriptor with isValid set to false if no descriptor exists for this ID.
https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: // only. On 2016/12/06 at 01:46:55, sashab wrote: > This isn't exactly for internal use only anymore :) So maybe put something like > > // Stores whether or not this descriptor is for a valid property. Do not access the contents of this descriptor unless this value is true. done https://codereview.chromium.org/2533673002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:22: // Returns a CSSPropertyDescriptor given a CSSPropertyID. On 2016/12/06 at 01:46:55, sashab wrote: > Nit - add a newline before this comment, showing its a separate method :) Also maybe expand this comment a little, e.g.: > > Returns the corresponding CSSPropertyDescriptor for a given CSSPropertyID. Use this function to access the API for a property. Returns a descriptor with isValid set to false if no descriptor exists for this ID. done
One more tiny thing :) https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:23: "CSSPropertyDescriptor should be POD."); Maybe change to "CSSPropertyDescriptor must be a POD to support using initializer lists." to make it explicit its for the list :)
https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:21: bool isValid; Is this just a temporary thing we need while we're transitioning? Will this go away when we have transitioned all properties over?
https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:21: bool isValid; On 2016/12/07 at 00:48:57, ojan wrote: > Is this just a temporary thing we need while we're transitioning? Will this go away when we have transitioned all properties over? In particular, I'm trying to understand the end state of the design. Will we always need the branch for checking isValid or is that something we can remove eventually?
PTAL :) https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp (right): https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.cpp:23: "CSSPropertyDescriptor should be POD."); On 2016/12/06 at 23:04:28, sashab wrote: > Maybe change to > > "CSSPropertyDescriptor must be a POD to support using initializer lists." > > to make it explicit its for the list :) done :) https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:21: bool isValid; On 2016/12/07 at 00:49:41, ojan wrote: > On 2016/12/07 at 00:48:57, ojan wrote: > > Is this just a temporary thing we need while we're transitioning? Will this go away when we have transitioned all properties over? > > In particular, I'm trying to understand the end state of the design. Will we always need the branch for checking isValid or is that something we can remove eventually? yep this will be removed eventually! and once the switch statement in CSSPropertyParser.cpp is removed, we won't need the if statement anymore. I added a TODO, does this look better?
aazzam@google.com changed reviewers: - sashab@chromium.org
ojan@google.com changed reviewers: + ojan@google.com
Almost there. Sorry for the delays. The TODOs are good (and should stay), but comments are much less effective at avoiding misuse of code than scary naming. https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:37: static const CSSPropertyID id; Can we give this a less inviting name so that only people who know what they're doing use this? In particular, say what the id is for. temporaryIDForXXX or something like that. I'm not quite following though, when will we be able to stop storing IDs? But, actually, I don't see anything that reads the id. Am I missing it? https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:23: bool isValid; Same here. If this is temporary, let's make it clear in the name so that other code is less likely to depend on it. Also, it can be less general. Something like, temporaryCanReadValue. I'm intentionally going with something that's a little confusing so that people won't be tempted to use it for anything. :)
PTAL ojan :) https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPI.h:37: static const CSSPropertyID id; On 2016/12/08 at 01:00:18, Z_DONOTUSE wrote: > Can we give this a less inviting name so that only people who know what they're doing use this? In particular, say what the id is for. temporaryIDForXXX or something like that. I'm not quite following though, when will we be able to stop storing IDs? > > But, actually, I don't see anything that reads the id. Am I missing it? you're right, ID isn't read anywhere, I got rid of it :) https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:23: bool isValid; On 2016/12/08 at 01:00:18, Z_DONOTUSE wrote: > Same here. If this is temporary, let's make it clear in the name so that other code is less likely to depend on it. Also, it can be less general. > > Something like, temporaryCanReadValue. I'm intentionally going with something that's a little confusing so that people won't be tempted to use it for anything. :) On second thoughts this isn't that temporary - eventually we will implement most properties, however we still will may want to not implement some properties and will need this variable to check. I've removed the TODO which was there before :)
aazzam@google.com changed reviewers: - ojan@google.com
ojan@google.com changed reviewers: + ojan@google.com
https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:23: bool isValid; On 2016/12/08 at 02:27:10, aazzam wrote: > On 2016/12/08 at 01:00:18, Z_DONOTUSE wrote: > > Same here. If this is temporary, let's make it clear in the name so that other code is less likely to depend on it. Also, it can be less general. > > > > Something like, temporaryCanReadValue. I'm intentionally going with something that's a little confusing so that people won't be tempted to use it for anything. :) > > On second thoughts this isn't that temporary - eventually we will implement most properties, however we still will may want to not implement some properties and will need this variable to check. I've removed the TODO which was there before :) What are cases where we'd want to keep this? Sorry for all the questions. We've just had a history of overly generic names getting misused. I don't know what makes a descriptor valid or invalid, so other people probably won't either. I'm trying to understand the end design we're aiming for. Also, there's an extra branch for the isValid check that we don't necessarily need.
https://codereview.chromium.org/2533673002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPIPadding.h (right): https://codereview.chromium.org/2533673002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPIPadding.h:22: class CSSPropertyAPIWebkitPaddingStart : public CSSPropertyAPIWebkitPadding {}; Nit: It's really strange to see empty classes like this. Maybe add a TODO to autogenerate this file to make it more clear that this is an intermediary state?
PTAL ojan :) https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:23: bool isValid; On 2016/12/08 at 03:18:00, Z_DONOTUSE wrote: > On 2016/12/08 at 02:27:10, aazzam wrote: > > On 2016/12/08 at 01:00:18, Z_DONOTUSE wrote: > > > Same here. If this is temporary, let's make it clear in the name so that other code is less likely to depend on it. Also, it can be less general. > > > > > > Something like, temporaryCanReadValue. I'm intentionally going with something that's a little confusing so that people won't be tempted to use it for anything. :) > > > > On second thoughts this isn't that temporary - eventually we will implement most properties, however we still will may want to not implement some properties and will need this variable to check. I've removed the TODO which was there before :) > > What are cases where we'd want to keep this? Sorry for all the questions. We've just had a history of overly generic names getting misused. I don't know what makes a descriptor valid or invalid, so other people probably won't either. I'm trying to understand the end design we're aiming for. > > Also, there's an extra branch for the isValid check that we don't necessarily need. Okay - for now, since we have properties which we intend to still implement, we need to keep isValid as a temporary variable. I've renamed it (as suggested) to temporaryCanReadValue :) In the future (once we've implemented all properties), I will make a new struct CSSInvalidPropertyDescriptor which implements the methods with NOTREACHED() for invalid properties where we want a runtime error to occur. For properties which aren't invalid, but are not handled in the switch statement (the ones which go to the default: return nullptr;) I will implement them with implementation return nullptr. So our final design of CSSPropertyParser won't have the branching for isValid check, and will also have the entire switch statement removed. https://codereview.chromium.org/2533673002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyAPIPadding.h (right): https://codereview.chromium.org/2533673002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyAPIPadding.h:22: class CSSPropertyAPIWebkitPaddingStart : public CSSPropertyAPIWebkitPadding {}; On 2016/12/08 at 03:20:05, Z_DONOTUSE wrote: > Nit: It's really strange to see empty classes like this. Maybe add a TODO to autogenerate this file to make it more clear that this is an intermediary state? Yep - it's already being autogenerated, i'm just submitting that CL after this one :) added the TODO for the interim
alancutter@chromium.org changed reviewers: + alancutter@chromium.org
https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: bool temporaryCanReadValue; TODO: Remove this once the switch in CSSPropertyParser::parseSingleValue() has been completely replaced by CSSPropertyDescriptors.
https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: bool temporaryCanReadValue; On 2016/12/11 at 23:31:15, alancutter wrote: > TODO: Remove this once the switch in CSSPropertyParser::parseSingleValue() has been completely replaced by CSSPropertyDescriptors. Alternate idea: Make get() return a const CSSPropertyDescriptor*. Personally I'd do this but I'll leave the decision up to you.
https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: bool temporaryCanReadValue; On 2016/12/11 at 23:31:15, alancutter wrote: > TODO: Remove this once the switch in CSSPropertyParser::parseSingleValue() has been completely replaced by CSSPropertyDescriptors. I've kept it as returning a reference rather than a pointer for performance. I considered changing it to a pointer temporarily, rather than having a temporary bool, but removing the bool temporaryCanReadValue is a simpler transition for after we've implemented all the properties.
lgtm https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h (right): https://codereview.chromium.org/2533673002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/properties/CSSPropertyDescriptor.h:20: bool temporaryCanReadValue; On 2016/12/12 at 03:37:32, aazzam wrote: > On 2016/12/11 at 23:31:15, alancutter wrote: > > TODO: Remove this once the switch in CSSPropertyParser::parseSingleValue() has been completely replaced by CSSPropertyDescriptors. > > I've kept it as returning a reference rather than a pointer for performance. I considered changing it to a pointer temporarily, rather than having a temporary bool, but removing the bool temporaryCanReadValue is a simpler transition for after we've implemented all the properties. I think Alan meant to add the TODO as a comment. Which, I agree with.
sashab@chromium.org changed reviewers: + sashab@chromium.org
LGTM. Well done Anna! :)
The CQ bit was checked by aazzam@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
aazzam@google.com changed reviewers: - ojan@chromium.org, ojan@google.com, sashab@chromium.org
PTAL Alan :)
lgtm
The CQ bit was checked by aazzam@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, ojan@google.com Link to the patchset: https://codereview.chromium.org/2533673002/#ps320001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by aazzam@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, ojan@google.com, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2533673002/#ps340001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by aazzam@google.com
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": 340001, "attempt_start_ts": 1481597467824720,
"parent_rev": "82bc1616acea6a7111e10ff92dfa3ece84cb0def", "commit_rev":
"5f86e313436ed20d90fd273fc387a66e37dd5509"}
Message was sent while issue was closed.
Description was changed from ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 ========== to ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 Review-Url: https://codereview.chromium.org/2533673002 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 Review-Url: https://codereview.chromium.org/2533673002 ========== to ========== Added CSSPropertyAPI and CSS padding properties which implement this API Added the CSSPropertyAPI for a single method parseSingleValue. Made CSSPaddingProperties which implements this API for a few css properties. Added CSS property descriptor which uses a struct to store function pointers for the property's functions. BUG=668012 Committed: https://crrev.com/13f6e06ed6b80d003e7298b501e57a58695ca951 Cr-Commit-Position: refs/heads/master@{#438057} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/13f6e06ed6b80d003e7298b501e57a58695ca951 Cr-Commit-Position: refs/heads/master@{#438057} |
