Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897833004:
Refactored out need to pass property ID in background list parsing. (Closed)
DescriptionRefactored out need to pass property ID in background list parsing.
This patch
- Moved AddBackgroundValue to CSSPropertyParserHelpers and renamed
it to AddListValueOptimised, for use in ConsumeCommaSeparatedList
helper
- Added a memory optimisation option to ConsumeCommaSeparatedList,
allowing background component lists to use this method without
changing any behavior. The optimisation option will not wrap a single
value in a list, but will return a single CSSValue. Use of this
optimisation can be more easily changed in future.
- Added DCHECKs to ensure that call sites that expect a list returned
from ConsumeCommaSeparatedList do receive a list (as the return type
has changed to the more general CSSValue)
- Deleted ConsumeCommaSeparatedBackgroundComponent, instead using
the more general ConsumeCommaSeparatedList template
- Split property specific parsing logic for background components
into the case statements in CSSPropertyParser::ParseSingleValue,
ready to be moved to property APIs. Logic for this was copied from
ConsumeBackgroundComponent. Note that ConsumeBackgroundComponent is
still currently used in ConsumeBackgroundShorthand and so cannot yet
be deleted.
BUG=668012
Patch Set 1 #Patch Set 2 : removed whitespace #Patch Set 3 : Replaced AddBackgroundValue with generic AddListValueOptimized #Patch Set 4 : changed bool to enum as per style guide #
Total comments: 8
Patch Set 5 : changed enum to enum class and addressed comments #
Total comments: 4
Messages
Total messages: 30 (16 generated)
The CQ bit was checked by bugsnash@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...
removed whitespace
Description was changed from ========== Refactored out need to pass property ID in background list parsing. This patch - Deleted ConsumeCommaSeparatedBackgroundComponent, instead using the more general ConsumeCommaSeparatedList template - Added a memory optimisation option to ConsumeCommaSeparatedList, allowing background component lists to use this method without changing any behavior. The optimisation option will not wrap a single value in a list. Use of this optimisation can be more easily changed in future. - Added DCHECKs to ensure that call sites that expect a list returned from ConsumeCommaSeparatedList do receive a list - Moved property specific parsing logic for background conponenents from ConsumeBackgroundComponent into the case statements in CSSPropertyParser::ParseSingleValue, ready to be moved to property APIs. Note that ConsumeBackgroundComponent is still currently used in ConsumeBackgroundShorthand and so cannot yet be deleted. BUG=668012 ========== to ========== Refactored out need to pass property ID in background list parsing. This patch - Deleted ConsumeCommaSeparatedBackgroundComponent, instead using the more general ConsumeCommaSeparatedList template - Added a memory optimisation option to ConsumeCommaSeparatedList, allowing background component lists to use this method without changing any behavior. The optimisation option will not wrap a single value in a list. Use of this optimisation can be more easily changed in future. - Added DCHECKs to ensure that call sites that expect a list returned from ConsumeCommaSeparatedList do receive a list - Moved property specific parsing logic for background componenents from ConsumeBackgroundComponent into the case statements in CSSPropertyParser::ParseSingleValue, ready to be moved to property APIs. Note that ConsumeBackgroundComponent is still currently used in ConsumeBackgroundShorthand and so cannot yet be deleted. BUG=668012 ==========
bugsnash@chromium.org changed reviewers: + jiameng@chromium.org
bugsnash@chromium.org changed reviewers: - jiameng@chromium.org
The CQ bit was checked by bugsnash@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.
bugsnash@chromium.org changed reviewers: + jiameng@chromium.org
The CQ bit was checked by bugsnash@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...
Description was changed from ========== Refactored out need to pass property ID in background list parsing. This patch - Deleted ConsumeCommaSeparatedBackgroundComponent, instead using the more general ConsumeCommaSeparatedList template - Added a memory optimisation option to ConsumeCommaSeparatedList, allowing background component lists to use this method without changing any behavior. The optimisation option will not wrap a single value in a list. Use of this optimisation can be more easily changed in future. - Added DCHECKs to ensure that call sites that expect a list returned from ConsumeCommaSeparatedList do receive a list - Moved property specific parsing logic for background componenents from ConsumeBackgroundComponent into the case statements in CSSPropertyParser::ParseSingleValue, ready to be moved to property APIs. Note that ConsumeBackgroundComponent is still currently used in ConsumeBackgroundShorthand and so cannot yet be deleted. BUG=668012 ========== to ========== Refactored out need to pass property ID in background list parsing. This patch - Moved AddBackgroundValue to CSSPropertyParserHelpers and renamed it to AddListValueOptimised, for use in ConsumeCommaSeparatedList helper - Added a memory optimisation option to ConsumeCommaSeparatedList, allowing background component lists to use this method without changing any behavior. The optimisation option will not wrap a single value in a list, but will return a single CSSValue. Use of this optimisation can be more easily changed in future. - Added DCHECKs to ensure that call sites that expect a list returned from ConsumeCommaSeparatedList do receive a list (as the return type has changed to the more general CSSValue) - Deleted ConsumeCommaSeparatedBackgroundComponent, instead using the more general ConsumeCommaSeparatedList template - Split property specific parsing logic for background components into the case statements in CSSPropertyParser::ParseSingleValue, ready to be moved to property APIs. Logic for this was copied from ConsumeBackgroundComponent. Note that ConsumeBackgroundComponent is still currently used in ConsumeBackgroundShorthand and so cannot yet be deleted. BUG=668012 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1633: return ConsumeCommaSeparatedList(kNoOptimisation, ConsumeTime, range_, Please consider add some description as to why some properties should use optimization while other don't. https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:30: enum Optimisation { kNoOptimisation, kOptimisation }; I see you have comment below at the function defn that says optimisation param is a bool, but please consider add a comment here as well, so that no other values will be added in the future. Also, it would be clearer to explicitly set kNoOptimisation to 0 and kOptimisation to 1. https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:130: // to optimise the list for memory, such that a single value is returned as is Nit: I think your comment meant to say ".. a single value is return as *it* is to.."? https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:147: optimise ? AddListValueOptimized(result, value) I understand that if "optimize" is true, then the function should return a CSSValue rather than a CSSValueList. But the code seems to do the opposite: 1). If "optimise" is false, then "result" will be a nullptr (from code above). Then "ToCSSValueList" will take a nullptr, I think this will fail. 2). If "optimise" is true, then "result" will be a ptr to CSSValueList. When AddListValueOptimized is called, the input "list" will be non-null, so the 1st "if" branch will be taken and it'll wrap the value into the list. Did you mean to set "result" to "CSSValueList::CreateCommaSeparated();" if "optimise" is false?
changed enum to enum class and addressed comments
sorry about the delay here, ptal https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1633: return ConsumeCommaSeparatedList(kNoOptimisation, ConsumeTime, range_, On 2017/05/23 at 23:56:14, Jia wrote: > Please consider add some description as to why some properties should use optimization while other don't. have added comment to ComsumeCommaSepartedList about how using the optimization may worsen code health. https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:30: enum Optimisation { kNoOptimisation, kOptimisation }; On 2017/05/23 at 23:56:14, Jia wrote: > I see you have comment below at the function defn that says optimisation param is a bool, but please consider add a comment here as well, so that no other values will be added in the future. Also, it would be clearer to explicitly set kNoOptimisation to 0 and kOptimisation to 1. i changed this to an enum class and am no longer treating it as a bool https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:130: // to optimise the list for memory, such that a single value is returned as is On 2017/05/23 at 23:56:14, Jia wrote: > Nit: I think your comment meant to say ".. a single value is return as *it* is to.."? fixed https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:147: optimise ? AddListValueOptimized(result, value) On 2017/05/23 at 23:56:14, Jia wrote: > I understand that if "optimize" is true, then the function should return a CSSValue rather than a CSSValueList. But the code seems to do the opposite: > > 1). If "optimise" is false, then "result" will be a nullptr (from code above). Then "ToCSSValueList" will take a nullptr, I think this will fail. > 2). If "optimise" is true, then "result" will be a ptr to CSSValueList. When AddListValueOptimized is called, the input "list" will be non-null, so the 1st "if" branch will be taken and it'll wrap the value into the list. > > Did you mean to set "result" to "CSSValueList::CreateCommaSeparated();" if "optimise" is false? yes I did, good catch!
On 2017/06/02 05:42:19, Bugs Nash wrote: > sorry about the delay here, ptal > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1633: return > ConsumeCommaSeparatedList(kNoOptimisation, ConsumeTime, range_, > On 2017/05/23 at 23:56:14, Jia wrote: > > Please consider add some description as to why some properties should use > optimization while other don't. > > have added comment to ComsumeCommaSepartedList about how using the optimization > may worsen code health. > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h > (right): > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:30: enum > Optimisation { kNoOptimisation, kOptimisation }; > On 2017/05/23 at 23:56:14, Jia wrote: > > I see you have comment below at the function defn that says optimisation param > is a bool, but please consider add a comment here as well, so that no other > values will be added in the future. Also, it would be clearer to explicitly set > kNoOptimisation to 0 and kOptimisation to 1. > > i changed this to an enum class and am no longer treating it as a bool > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:130: // to > optimise the list for memory, such that a single value is returned as is > On 2017/05/23 at 23:56:14, Jia wrote: > > Nit: I think your comment meant to say ".. a single value is return as *it* is > to.."? > > fixed > > https://codereview.chromium.org/2897833004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:147: > optimise ? AddListValueOptimized(result, value) > On 2017/05/23 at 23:56:14, Jia wrote: > > I understand that if "optimize" is true, then the function should return a > CSSValue rather than a CSSValueList. But the code seems to do the opposite: > > > > 1). If "optimise" is false, then "result" will be a nullptr (from code above). > Then "ToCSSValueList" will take a nullptr, I think this will fail. > > 2). If "optimise" is true, then "result" will be a ptr to CSSValueList. When > AddListValueOptimized is called, the input "list" will be non-null, so the 1st > "if" branch will be taken and it'll wrap the value into the list. > > > > Did you mean to set "result" to "CSSValueList::CreateCommaSeparated();" if > "optimise" is false? > > yes I did, good catch! LGTM
bugsnash@chromium.org changed reviewers: + suzyh@chromium.org
+suzyh for owners
Hrmm... I'm undecided how I feel about this one. It's a gnarly problem, and this is a decent solution... I'm just not sure what the best approach is going to be. So, the first question is whether we definitely need the optimization. I'm sorry if you already answered this and I forgot... If we do want to keep the optimization, how do you feel about a separate method ConsumeSingleValueOrCommaSeparatedList(*) that does the optimized version, and having ConsumeCommaSeparatedList call that and wrap the result in a list if it is just a single item? That way the call site knows whether it's calling the optimized version or is definitely getting a list, without the enum and extra DCHECKs? (*) Naming is hard. https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1461: void AddListValueOptimized(CSSValue*& list, CSSValue* value) { No action required, since this is not new to your patch. Just felt a need to say 'ew'. I didn't realise you could have a reference to a pointer >.< https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:140: // of being wrapped in a list. Using the optimisation may worsen code health Do we have data to indicate that the optimisation does in fact have a performance benefit?
On 2017/06/05 at 00:49:20, suzyh wrote: > Hrmm... I'm undecided how I feel about this one. It's a gnarly problem, and this is a decent solution... I'm just not sure what the best approach is going to be. > > So, the first question is whether we definitely need the optimization. I'm sorry if you already answered this and I forgot... See inline comment > > If we do want to keep the optimization, how do you feel about a separate method ConsumeSingleValueOrCommaSeparatedList(*) that does the optimized version, and having ConsumeCommaSeparatedList call that and wrap the result in a list if it is just a single item? That way the call site knows whether it's calling the optimized version or is definitely getting a list, without the enum and extra DCHECKs? > (*) Naming is hard. Hmm not sure I would want the non optimized method to call through to the optimised method, as the optimised method requires more work, branching and changing the type of value etc. without the optimization that's not necessary. Perhaps split them out into separate methods with some duplicate logic? Perhaps have the optimised version call through to the non optimised version and remove the list wrapping if there's only 1 item? none of the options are ideal really. what are your thoughts? > > https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): > > https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1461: void AddListValueOptimized(CSSValue*& list, CSSValue* value) { > No action required, since this is not new to your patch. Just felt a need to say 'ew'. I didn't realise you could have a reference to a pointer >.< > > https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): > > https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:140: // of being wrapped in a list. Using the optimisation may worsen code health > Do we have data to indicate that the optimisation does in fact have a performance benefit?
https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp (right): https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:1461: void AddListValueOptimized(CSSValue*& list, CSSValue* value) { On 2017/06/05 at 00:49:20, suzyh_UTC10 wrote: > No action required, since this is not new to your patch. Just felt a need to say 'ew'. I didn't realise you could have a reference to a pointer >.< this is required because we need to change what the pointer points to (see line 1465) and this needs to be reflected in the call site https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h (right): https://codereview.chromium.org/2897833004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.h:140: // of being wrapped in a list. Using the optimisation may worsen code health On 2017/06/05 at 00:49:20, suzyh_UTC10 wrote: > Do we have data to indicate that the optimisation does in fact have a performance benefit? currently working on that. it seems like it does have a performance benefit but further investigation required. happy to hold off on this patch until that happens
> > If we do want to keep the optimization, how do you feel about a separate method ConsumeSingleValueOrCommaSeparatedList(*) that does the optimized version, and having ConsumeCommaSeparatedList call that and wrap the result in a list if it is just a single item? That way the call site knows whether it's calling the optimized version or is definitely getting a list, without the enum and extra DCHECKs? > > (*) Naming is hard. > > Hmm not sure I would want the non optimized method to call through to the optimised method, as the optimised method requires more work, branching and changing the type of value etc. without the optimization that's not necessary. Perhaps split them out into separate methods with some duplicate logic? Perhaps have the optimised version call through to the non optimised version and remove the list wrapping if there's only 1 item? none of the options are ideal really. what are your thoughts? LOL at the idea that the "optimised" version does more work and it is therefore cheaper for the "optimised" version to call the "non-optimised" version. If that's really the case, then I think we need to be clearer that the optimisation is for the caller, not within the function itself. As discussed offline, let's first see what the performance hit is from making all callsites use the non-optimised version. Then if that doesn't fly, we can decide whether to do "optimised version then wrap in list" or "non-optimised version then fetch first and only element".
On 2017/06/05 at 01:43:59, suzyh wrote: > > > If we do want to keep the optimization, how do you feel about a separate method ConsumeSingleValueOrCommaSeparatedList(*) that does the optimized version, and having ConsumeCommaSeparatedList call that and wrap the result in a list if it is just a single item? That way the call site knows whether it's calling the optimized version or is definitely getting a list, without the enum and extra DCHECKs? > > > (*) Naming is hard. > > > > Hmm not sure I would want the non optimized method to call through to the optimised method, as the optimised method requires more work, branching and changing the type of value etc. without the optimization that's not necessary. Perhaps split them out into separate methods with some duplicate logic? Perhaps have the optimised version call through to the non optimised version and remove the list wrapping if there's only 1 item? none of the options are ideal really. what are your thoughts? > > LOL at the idea that the "optimised" version does more work and it is therefore cheaper for the "optimised" version to call the "non-optimised" version. If that's really the case, then I think we need to be clearer that the optimisation is for the caller, not within the function itself. yeah it's a memory optimization, not a performance optimization. the method comment covers that, is that enough? > > As discussed offline, let's first see what the performance hit is from making all callsites use the non-optimised version. Then if that doesn't fly, we can decide whether to do "optimised version then wrap in list" or "non-optimised version then fetch first and only element". sgtm
Message was sent while issue was closed.
On 2017/06/05 at 01:56:39, Bugs Nash wrote: > On 2017/06/05 at 01:43:59, suzyh wrote: > > > > If we do want to keep the optimization, how do you feel about a separate method ConsumeSingleValueOrCommaSeparatedList(*) that does the optimized version, and having ConsumeCommaSeparatedList call that and wrap the result in a list if it is just a single item? That way the call site knows whether it's calling the optimized version or is definitely getting a list, without the enum and extra DCHECKs? > > > > (*) Naming is hard. > > > > > > Hmm not sure I would want the non optimized method to call through to the optimised method, as the optimised method requires more work, branching and changing the type of value etc. without the optimization that's not necessary. Perhaps split them out into separate methods with some duplicate logic? Perhaps have the optimised version call through to the non optimised version and remove the list wrapping if there's only 1 item? none of the options are ideal really. what are your thoughts? > > > > LOL at the idea that the "optimised" version does more work and it is therefore cheaper for the "optimised" version to call the "non-optimised" version. If that's really the case, then I think we need to be clearer that the optimisation is for the caller, not within the function itself. > > yeah it's a memory optimization, not a performance optimization. the method comment covers that, is that enough? > > > > > As discussed offline, let's first see what the performance hit is from making all callsites use the non-optimised version. Then if that doesn't fly, we can decide whether to do "optimised version then wrap in list" or "non-optimised version then fetch first and only element". > > sgtm Closed as this has been rewritten at https://chromium-review.googlesource.com/c/575253/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
