Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917323003:
Move AddProperty to a helper class. (Closed)
DescriptionMove AddProperty to a helper class.
This cl paves the way for parsing shorthand properties. AddProperty is
currently a private function within CSSPropertyParser. We now move it
to a helper class so that it can be accessed by property APIs when a
property parses its values and adds parsed properties to a set of all
parsed properties for further processing.
BUG=668012
Review-Url: https://codereview.chromium.org/2917323003
Cr-Commit-Position: refs/heads/master@{#477261}
Committed: https://chromium.googlesource.com/chromium/src/+/97d2eb259484cb04b331ac05367ff2bb1c31ed5a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change parameter name to conform to the style guide. #
Total comments: 8
Patch Set 3 : Changed the wrapper function name to AddParsedProperty. #
Messages
Total messages: 27 (13 generated)
jiameng@chromium.org changed reviewers: + bugsnash@chromium.org
The CQ bit was checked by jiameng@chromium.org 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/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void CSSPropertyParser::AddParsedProperties(CSSPropertyID property, is there a reason to keep this method here? I would just call CSSPropertyParserHelpers::AddProperty directly. This method won't be accessible by the APIs right? https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID property, Passing CSSPropertyID makes me uncomfortable. I think it's necessary here though :( do you have a plan for how the APIs will know which CSSPropertyIDs to pass through here? https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:117: CSSPropertyID, given that there are 2 of these add names for readability
Thanks! PTAL. https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void CSSPropertyParser::AddParsedProperties(CSSPropertyID property, On 2017/06/05 05:45:58, Bugs Nash wrote: > is there a reason to keep this method here? I would just call > CSSPropertyParserHelpers::AddProperty directly. This method won't be accessible > by the APIs right? I would like to keep this "wrapper" function to avoid repeated code: all subsequent calls in this file share the same "parsed_properties_" and they all leave out parameter "implicit". https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID property, On 2017/06/05 05:45:58, Bugs Nash wrote: > Passing CSSPropertyID makes me uncomfortable. I think it's necessary here though > :( > > do you have a plan for how the APIs will know which CSSPropertyIDs to pass > through here? This helper function will be called by individual property class, so the property itself will know what its property ID is and also whether it's a shorthand. These property info will then be passed to this function. https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:117: CSSPropertyID, On 2017/06/05 05:45:58, Bugs Nash wrote: > given that there are 2 of these add names for readability Done.
The CQ bit was checked by jiameng@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/05 at 06:15:09, jiameng wrote: > Thanks! PTAL. > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void CSSPropertyParser::AddParsedProperties(CSSPropertyID property, > On 2017/06/05 05:45:58, Bugs Nash wrote: > > is there a reason to keep this method here? I would just call > > CSSPropertyParserHelpers::AddProperty directly. This method won't be accessible > > by the APIs right? > > I would like to keep this "wrapper" function to avoid repeated code: all subsequent calls in this file share the same "parsed_properties_" and they all leave out parameter "implicit". NB as per offline convo, when the code is migrated to the APIs they will not call this wrapper function, they will call AddProperty directly. > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID property, > On 2017/06/05 05:45:58, Bugs Nash wrote: > > Passing CSSPropertyID makes me uncomfortable. I think it's necessary here though > > :( > > > > do you have a plan for how the APIs will know which CSSPropertyIDs to pass > > through here? > > This helper function will be called by individual property class, so the property itself will know what its property ID is and also whether it's a shorthand. These property info will then be passed to this function. > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:117: CSSPropertyID, > On 2017/06/05 05:45:58, Bugs Nash wrote: > > given that there are 2 of these add names for readability > > Done. lgtm with nits
On 2017/06/06 04:27:44, Bugs Nash wrote: > On 2017/06/05 at 06:15:09, jiameng wrote: > > Thanks! PTAL. > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void > CSSPropertyParser::AddParsedProperties(CSSPropertyID property, > > On 2017/06/05 05:45:58, Bugs Nash wrote: > > > is there a reason to keep this method here? I would just call > > > CSSPropertyParserHelpers::AddProperty directly. This method won't be > accessible > > > by the APIs right? > > > > I would like to keep this "wrapper" function to avoid repeated code: all > subsequent calls in this file share the same "parsed_properties_" and they all > leave out parameter "implicit". > > NB as per offline convo, when the code is migrated to the APIs they will not > call this wrapper function, they will call AddProperty directly. > > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp > (right): > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: > void AddProperty(CSSPropertyID property, > > On 2017/06/05 05:45:58, Bugs Nash wrote: > > > Passing CSSPropertyID makes me uncomfortable. I think it's necessary here > though > > > :( > > > > > > do you have a plan for how the APIs will know which CSSPropertyIDs to pass > > > through here? > > > > This helper function will be called by individual property class, so the > property itself will know what its property ID is and also whether it's a > shorthand. These property info will then be passed to this function. > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h > (right): > > > > > https://codereview.chromium.org/2917323003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:117: > CSSPropertyID, > > On 2017/06/05 05:45:58, Bugs Nash wrote: > > > given that there are 2 of these add names for readability > > > > Done. > > lgtm with nits Thanks. That's right, we can remove the wrapper function AddProperties in CSSPropertyParser.cpp after we complete ribbonizing all properties. We will also need to change longhand property API so that in addition to parsing the values, it'll add parsed values to the parsed_properties_ set.
jiameng@chromium.org changed reviewers: + meade@chromium.org
meade@chromium.org: Please review changes in
https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void CSSPropertyParser::AddParsedProperties(CSSPropertyID property, nit: this name isn't accurate. this method only adds a single property (not properties) AddParsedProperty is better https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID resolved_property, optional: AddPropertyToVector might be a better name? since you're passing in the vector as well. If you do rename this to AddPropertyToVector then the wrapper function can remain as AddProperty. up to you https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1467: HeapVector<CSSProperty, 256>& properties) { nit: I think this HeapVector argument should be before the bools in the argument list as it's more important than the bools
Thanks Bugs, PTAL. https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:71: void CSSPropertyParser::AddParsedProperties(CSSPropertyID property, On 2017/06/06 04:37:05, Bugs Nash wrote: > nit: this name isn't accurate. this method only adds a single property (not > properties) AddParsedProperty is better Done. https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID resolved_property, On 2017/06/06 04:37:05, Bugs Nash wrote: > optional: AddPropertyToVector might be a better name? since you're passing in > the vector as well. If you do rename this to AddPropertyToVector then the > wrapper function can remain as AddProperty. up to you I'd like to leave the "vector" part out of the name, as it's too implementation-specific. :) https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1467: HeapVector<CSSProperty, 256>& properties) { On 2017/06/06 04:37:05, Bugs Nash wrote: > nit: I think this HeapVector argument should be before the bools in the argument > list as it's more important than the bools We usually put input params before the output params. Do we have a different style guide here?
https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: void AddProperty(CSSPropertyID resolved_property, On 2017/06/06 at 05:00:47, Jia wrote: > On 2017/06/06 04:37:05, Bugs Nash wrote: > > optional: AddPropertyToVector might be a better name? since you're passing in > > the vector as well. If you do rename this to AddPropertyToVector then the > > wrapper function can remain as AddProperty. up to you > > I'd like to leave the "vector" part out of the name, as it's too implementation-specific. :) True. AddPropertyToSet? At the moment the 2 names are AddParsedProperty and AddProperty and they mean exactly the same thing, they're both adding parsed properties and they could both be called simply AddProperty. The difference is that one passes in the set to be added to and one doesn't. Can you think of a better pair of names that gets this across? https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1467: HeapVector<CSSProperty, 256>& properties) { On 2017/06/06 at 05:00:47, Jia wrote: > On 2017/06/06 04:37:05, Bugs Nash wrote: > > nit: I think this HeapVector argument should be before the bools in the argument > > list as it's more important than the bools > > We usually put input params before the output params. Do we have a different style guide here? No there doesn't seem to be, you are correct
On 2017/06/06 05:14:12, Bugs Nash wrote: > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp > (right): > > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: > void AddProperty(CSSPropertyID resolved_property, > On 2017/06/06 at 05:00:47, Jia wrote: > > On 2017/06/06 04:37:05, Bugs Nash wrote: > > > optional: AddPropertyToVector might be a better name? since you're passing > in > > > the vector as well. If you do rename this to AddPropertyToVector then the > > > wrapper function can remain as AddProperty. up to you > > > > I'd like to leave the "vector" part out of the name, as it's too > implementation-specific. :) > > True. AddPropertyToSet? At the moment the 2 names are AddParsedProperty and > AddProperty and they mean exactly the same thing, they're both adding parsed > properties and they could both be called simply AddProperty. The difference is > that one passes in the set to be added to and one doesn't. Can you think of a > better pair of names that gets this across? > > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1467: > HeapVector<CSSProperty, 256>& properties) { > On 2017/06/06 at 05:00:47, Jia wrote: > > On 2017/06/06 04:37:05, Bugs Nash wrote: > > > nit: I think this HeapVector argument should be before the bools in the > argument > > > list as it's more important than the bools > > > > We usually put input params before the output params. Do we have a different > style guide here? > > No there doesn't seem to be, you are correct Re the name above. It's true the two methods have similar names, but since the wrapper version is really temporary, I'd like to keep a better sounding name for the external version. As for the internal version, again, as it's temporary, I really don't think it matters. :)
On 2017/06/06 at 05:33:37, jiameng wrote: > On 2017/06/06 05:14:12, Bugs Nash wrote: > > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp > > (right): > > > > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1462: > > void AddProperty(CSSPropertyID resolved_property, > > On 2017/06/06 at 05:00:47, Jia wrote: > > > On 2017/06/06 04:37:05, Bugs Nash wrote: > > > > optional: AddPropertyToVector might be a better name? since you're passing > > in > > > > the vector as well. If you do rename this to AddPropertyToVector then the > > > > wrapper function can remain as AddProperty. up to you > > > > > > I'd like to leave the "vector" part out of the name, as it's too > > implementation-specific. :) > > > > True. AddPropertyToSet? At the moment the 2 names are AddParsedProperty and > > AddProperty and they mean exactly the same thing, they're both adding parsed > > properties and they could both be called simply AddProperty. The difference is > > that one passes in the set to be added to and one doesn't. Can you think of a > > better pair of names that gets this across? > > > > https://codereview.chromium.org/2917323003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1467: > > HeapVector<CSSProperty, 256>& properties) { > > On 2017/06/06 at 05:00:47, Jia wrote: > > > On 2017/06/06 04:37:05, Bugs Nash wrote: > > > > nit: I think this HeapVector argument should be before the bools in the > > argument > > > > list as it's more important than the bools > > > > > > We usually put input params before the output params. Do we have a different > > style guide here? > > > > No there doesn't seem to be, you are correct > > Re the name above. It's true the two methods have similar names, but since the wrapper version is really temporary, I'd like to keep a better sounding name for the external version. As for the internal version, again, as it's temporary, I really don't think it matters. :) k
lgtm
The CQ bit was checked by jiameng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bugsnash@chromium.org Link to the patchset: https://codereview.chromium.org/2917323003/#ps40001 (title: "Changed the wrapper function name to AddParsedProperty.")
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": 1496743102331870,
"parent_rev": "10a266981d83baf81c3eb24873c8732699ed087e", "commit_rev":
"ad728106916414170d30b93471942e59115b7468"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496743102331870,
"parent_rev": "915f99ef26a1a1fe4d3dca0321f7509b3996db59", "commit_rev":
"97d2eb259484cb04b331ac05367ff2bb1c31ed5a"}
Message was sent while issue was closed.
Description was changed from ========== Move AddProperty to a helper class. This cl paves the way for parsing shorthand properties. AddProperty is currently a private function within CSSPropertyParser. We now move it to a helper class so that it can be accessed by property APIs when a property parses its values and adds parsed properties to a set of all parsed properties for further processing. BUG=668012 ========== to ========== Move AddProperty to a helper class. This cl paves the way for parsing shorthand properties. AddProperty is currently a private function within CSSPropertyParser. We now move it to a helper class so that it can be accessed by property APIs when a property parses its values and adds parsed properties to a set of all parsed properties for further processing. BUG=668012 Review-Url: https://codereview.chromium.org/2917323003 Cr-Commit-Position: refs/heads/master@{#477261} Committed: https://chromium.googlesource.com/chromium/src/+/97d2eb259484cb04b331ac05367f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/97d2eb259484cb04b331ac05367f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
