Bug 150874 – Use modern for-loops in WebCore/css.

• Component: WebCore Misc.
• Status: RESOLVED LATER

[Hunseop Jeong]

Refactor the for-loops using the ranged-for loops and auto keyword in WebCore/css.

[Hunseop Jeong]

Created attachment 264766 [details]
Patch

[Hunseop Jeong]

Created attachment 264785 [details]
Patch

[Darin Adler]

Comment on attachment 264785 [details]
Patch

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

> Source/WebCore/css/CSSBasicShapes.cpp:290
> +        points.append(value->cssText());

Since we do reserveInitialCapacity above, this can and should be uncheckedAppend rather than append.

> Source/WebCore/css/CSSBasicShapes.cpp:361
> +            for (auto& radii : horizontalRadii) {

The word “radii” is plural. The singular would be “radius”, so this should be “radius”.

> Source/WebCore/css/CSSBasicShapes.cpp:371
> +                for (auto& radii : verticalRadii) {

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1775
> +        short number = propertyID == CSSPropertyCounterIncrement ? entry.value.incrementValue() : entry.value.resetValue();
>          list->append(cssValuePool.createValue((double)number, CSSPrimitiveValue::CSS_NUMBER));

Not new to the patch, but:

Doesn’t seem safe to convert the number to a short. The incrementValue and resetValue functions return an int. Better would be to use the type “int” for the local variable, or still better, to use “auto” so it the return type of incrementValue and resetValue functions determine the type.

The typecast to (double) should be unneeded. Other call site call createValue on values of type int without any typecast. I think the use of short here is the only reason that cast was needed before.

Someone should add a test that shows this old code is incorrect, with a value outside the range of a short.

> Source/WebCore/css/CSSGroupingRule.cpp:57
> +    for (auto& childRule : m_childRuleCSSOMWrappers) {
> +        if (childRule)
> +            childRule->setParentRule(0);
>      }

The name childRule here is OK. I would probably use the name “wrapper” or “rule” instead. Generally speaking I like these very local names in these small loops to be short single words, since they don’t have to be unambiguous in a broader scope.

The argument to setParentRule should be nullptr, not 0.

> Source/WebCore/css/CSSKeyframesRule.cpp:99
> +    for (auto& childRule : m_childRuleCSSOMWrappers) {
> +        if (childRule)
> +            childRule->setParentRule(nullptr);
>      }

Ditto.

Really unpleasant that this code has to be repeated. Maybe there’s a way to refactor at some point so CSSGroupingRule and CSSKeyframeRule can share code.

> Source/WebCore/css/CSSParser.cpp:5146
> +        String cur = string.stripWhiteSpace();

Comments that have nothing to do with this patch:

The local variable name “cur” is not right for WebKit coding style. I think I already have a patch in progress that addresses this.

The use of stripWhitespace here is likely a bug. It should probably be stripLeadingAndTrailingHTMLSpaces. I think I already have a patch in progress that addresses this.

Someone should redo this code to use StringView instead of String; we don’t want to allocate all these strings just so we can convert them to floating point numbers and then throw them away. Might have to add some things to StringView to do that efficiently. I think I already have a patch in progress that addresses this.

> Source/WebCore/css/CSSSelectorList.cpp:58
> +    for (auto& current : selectorVector) {

It’s usually better if a variable name is a noun, not an adjective. I do appreciate your use of a single word here, though, so I’m not going to suggest a better name. The obvious one is selector, but that’s used for the inner loop. Maybe some vague word like “item” (one of the items in the vector) or “head” (head of a linked list where tagHistory is the “next” link) would be better, but I am not sure.

> Source/WebCore/css/CSSSelectorList.cpp:59
> +        for (CSSParserSelector* selector = current.get(); selector; selector = selector->tagHistory())

Could use auto* here.

> Source/WebCore/css/CSSStyleSheet.cpp:128
> +    for (auto& childRule : m_childRuleCSSOMWrappers) {
> +        if (childRule)
> +            childRule->setParentStyleSheet(0);
>      }

Same comments as above. Use nullptr, maybe a single word name “wrapper”?

> Source/WebCore/css/CSSValueList.cpp:75
> +        if (value.get().equals(*val))

Not sure if I prefer “.get().” over “->”, which we could also use in places like this. Would love to hear Andreas Kling’s take on that. Maybe we should remove the -> operator and always use .get(). for this.

> Source/WebCore/css/CSSValueList.cpp:161
> +        value.get().addSubresourceStyleURLs(urls, styleSheet);

Ditto.

> Source/WebCore/css/CSSValueList.cpp:167
> +        if (value.get().traverseSubresources(handler))

Ditto.

> Source/WebCore/css/CSSValueList.cpp:179
> +        m_values.uncheckedAppend(*value->cloneForCSSOM());

An example of where we use -> but we could use .get(). instead.

Instead of *value->cloneForCSSOM() this should just say value->cloneForCSSOM() (or value.get().cloneForCSSOM(), but no "*"). I believe the "*" actually causes one cycle of unnecessary reference count churn since it prevents us from moving into m_values vector instead of copying and then destroying the temporary.

> Source/WebCore/css/CSSValueList.cpp:253
> +        switch (value->classType()) {

Another example of where we use -> but we could use .get(). instead.

> Source/WebCore/css/CSSValueList.cpp:255
> +            if (!downcast<CSSFunctionValue>(*value.ptr()).buildParserValueSubstitutingVariables(&result, customProperties))

Instead of *value.ptr() this should use value.get(). No idea why the existing code was doing it that roundabout way.

> Source/WebCore/css/CSSValueList.cpp:260
> +            if (!downcast<CSSValueList>(*value.ptr()).buildParserValueSubstitutingVariables(&result, customProperties))

Ditto.

> Source/WebCore/css/CSSValueList.cpp:265
> +            if (!downcast<CSSVariableValue>(*value.ptr()).buildParserValueListSubstitutingVariables(parserList, customProperties))

Ditto.

> Source/WebCore/css/CSSValueList.cpp:271
> +            if (downcast<CSSPrimitiveValue>(*value.ptr()).buildParserValue(&result))

Ditto.

> Source/WebCore/css/ElementRuleCollector.cpp:152
> +    for (auto& pair : matchRequest.ruleSet->regionSelectorsAndRuleSets()) {

Could even consider naming this “region” instead of “pair”. It’s not *exactly* a region, but close enough. Because region.selector is the selector for the region, and region.ruleSet is the rule set for the region.

> Source/WebCore/css/ElementRuleCollector.cpp:153
> +        const CSSSelector* regionSelector = pair.selector;

No need for this local variable. The pair.selector is only used once.

> Source/WebCore/css/ElementRuleCollector.cpp:156
> +            RuleSet* regionRules = pair.ruleSet.get();
>              ASSERT(regionRules);

We don’t need a local variable for this either. We can still assert it if we want, but “pair.ruleSet” or “region.ruleSet” is clear enough without naming a local variable.

> Source/WebCore/css/MediaList.cpp:131
> +        String medium = string.stripWhiteSpace();

The use of stripWhitespace here is likely a bug. It should probably be stripLeadingAndTrailingHTMLSpaces.

> Source/WebCore/css/MediaList.cpp:323
> +            for (auto& exp : query->expressions()) {

We should probably rename “exp” to “expression” while touching this.

> Source/WebCore/css/MediaList.cpp:325
>                      CSSValue* cssValue =  exp->value();

Extra space here after the "=" sign.

> Source/WebCore/css/MediaQueryEvaluator.cpp:142
> +        if (result)
> +            break;

If we were keeping this local variable I would suggest putting this at the end of the loop instead of the start.

But instead, I suggest eliminating this local variable and changing the two lines of code that say:

    result = xxx;

To:

    if (xxx)
        return true;

Then return false at the end of the function.

> Source/WebCore/css/MediaQueryMatcher.cpp:115
> +void MediaQueryMatcher::addListener(PassRefPtr<MediaQueryListListener> queryListener, PassRefPtr<MediaQueryList> query)

I don’t think queryListener is quite the right name for this. We can probably stick with listener. The ambiguity comes from two things:

1) We call a stored pair with both listener and query a listener, making the word ambiguous.
2) This is the “new listener”.

We could name these “newListener” and “newQuery”, but I think there’s a better idea below.

> Source/WebCore/css/MediaQueryMatcher.cpp:120
> +    for (auto& listener : m_listeners) {

I think here we can use the name “existingListener”.

> Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:88
>      RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();

This should be Ref, not RefPtr. Better, I would probably prefer auto rather than repeating the type CSSValue.

The return value from the function this code is in should be Ref<CSSValue>, not RefPtr. It can’t return null.

> Source/WebCore/css/StyleResolver.cpp:348
> +    for (auto& entry : toRemove)

I think we should call this “key” rather than “entry”.

> Source/WebCore/css/StyleResolver.cpp:1350
> +        ASSERT(regionSelectorAndRuleSet.ruleSet.get());

I don’t think we need the .get() here.

> Source/WebCore/css/StyleResolver.cpp:1357
> +            ASSERT(regionSelectorAndRuleSet.ruleSet.get());

I don’t think we need the .get() here.

> Source/WebCore/css/StyleSheetContents.cpp:55
> +        if (StyleSheetContents* sheet = importRule->styleSheet())

Maybe auto* here?

> Source/WebCore/css/WebKitCSSMatrix.cpp:79
> +            if (operation.get()->apply(t, IntSize(0, 0))) {

I don’t think the .get() is needed here.

[Andreas Kling]

(In reply to comment #3)
> > Source/WebCore/css/CSSValueList.cpp:75
> > +        if (value.get().equals(*val))
> 
> Not sure if I prefer “.get().” over “->”, which we could also use in places
> like this. Would love to hear Andreas Kling’s take on that. Maybe we should
> remove the -> operator and always use .get(). for this.

I go back and forth.

Recently I've been using operator->, with the thinking that once operator. becomes available to us (fingers crossed for N4173 <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4173.pdf>), we can just replace Ref::operator-> with Ref::operator. and then fix all the build errors.

Also .get(). is a bit of an eyesore. Then again so is ->. :|

[Darin Adler]

(In reply to comment #4)
> Recently I've been using operator->, with the thinking that once operator.
> becomes available to us (fingers crossed for N4173
> <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4173.pdf>), we can
> just replace Ref::operator-> with Ref::operator. and then fix all the build
> errors.

That’s a good argument in favor of ->. We wouldn’t get build errors in all the places that use .get(). once operator. was available, but we would be able to get build errors in all the places that do use ->.

> Also .get(). is a bit of an eyesore. Then again so is ->. :|

The best argument in favor of .get(). is that it has only dots in it, so doesn’t lead the reader of the code to believe there’s a possibly null pointer involved.

One thing that would have both of these good properties would be some synonym for get() that we would only use in the .get(). idiom. Maybe something crazy like r() that should only be used as .r(). and never used in any other case. Even more ugly than -> or .get(). but has these two wonderful qualities of not looking like a pointer and being easy to roll out. It might also have a third good quality that you could roll out ".r()." from the entire project with a global replace.

[Hunseop Jeong]

Comment on attachment 264785 [details]
Patch

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

>> Source/WebCore/css/CSSBasicShapes.cpp:290
>> +        points.append(value->cssText());
> 
> Since we do reserveInitialCapacity above, this can and should be uncheckedAppend rather than append.

Oops,, done.

>> Source/WebCore/css/CSSBasicShapes.cpp:361
>> +            for (auto& radii : horizontalRadii) {
> 
> The word “radii” is plural. The singular would be “radius”, so this should be “radius”.

Okay, I will keep it in mind.

>> Source/WebCore/css/CSSBasicShapes.cpp:371
>> +                for (auto& radii : verticalRadii) {
> 
> Ditto.

Ditto.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1775
>>          list->append(cssValuePool.createValue((double)number, CSSPrimitiveValue::CSS_NUMBER));
> 
> Not new to the patch, but:
> 
> Doesn’t seem safe to convert the number to a short. The incrementValue and resetValue functions return an int. Better would be to use the type “int” for the local variable, or still better, to use “auto” so it the return type of incrementValue and resetValue functions determine the type.
> 
> The typecast to (double) should be unneeded. Other call site call createValue on values of type int without any typecast. I think the use of short here is the only reason that cast was needed before.
> 
> Someone should add a test that shows this old code is incorrect, with a value outside the range of a short.

I change it to use the auto keyword and remove the uneeded cast.

>> Source/WebCore/css/CSSGroupingRule.cpp:57
>>      }
> 
> The name childRule here is OK. I would probably use the name “wrapper” or “rule” instead. Generally speaking I like these very local names in these small loops to be short single words, since they don’t have to be unambiguous in a broader scope.
> 
> The argument to setParentRule should be nullptr, not 0.

Okay,, I change the name to "wrapper".

>> Source/WebCore/css/CSSKeyframesRule.cpp:99
>>      }
> 
> Ditto.
> 
> Really unpleasant that this code has to be repeated. Maybe there’s a way to refactor at some point so CSSGroupingRule and CSSKeyframeRule can share code.

Ditto.

>> Source/WebCore/css/CSSSelectorList.cpp:58
>> +    for (auto& current : selectorVector) {
> 
> It’s usually better if a variable name is a noun, not an adjective. I do appreciate your use of a single word here, though, so I’m not going to suggest a better name. The obvious one is selector, but that’s used for the inner loop. Maybe some vague word like “item” (one of the items in the vector) or “head” (head of a linked list where tagHistory is the “next” link) would be better, but I am not sure.

Okay,, I will keep it mind.
I change the name to "item".

>> Source/WebCore/css/CSSSelectorList.cpp:59
>> +        for (CSSParserSelector* selector = current.get(); selector; selector = selector->tagHistory())
> 
> Could use auto* here.

Done

>> Source/WebCore/css/CSSStyleSheet.cpp:128
>>      }
> 
> Same comments as above. Use nullptr, maybe a single word name “wrapper”?

Done

>> Source/WebCore/css/CSSValueList.cpp:75
>> +        if (value.get().equals(*val))
> 
> Not sure if I prefer “.get().” over “->”, which we could also use in places like this. Would love to hear Andreas Kling’s take on that. Maybe we should remove the -> operator and always use .get(). for this.

Do I leave the '.get().' ? or change to '->' ?

>> Source/WebCore/css/CSSValueList.cpp:255
>> +            if (!downcast<CSSFunctionValue>(*value.ptr()).buildParserValueSubstitutingVariables(&result, customProperties))
> 
> Instead of *value.ptr() this should use value.get(). No idea why the existing code was doing it that roundabout way.

Done.

>> Source/WebCore/css/CSSValueList.cpp:260
>> +            if (!downcast<CSSValueList>(*value.ptr()).buildParserValueSubstitutingVariables(&result, customProperties))
> 
> Ditto.

Done

>> Source/WebCore/css/CSSValueList.cpp:265
>> +            if (!downcast<CSSVariableValue>(*value.ptr()).buildParserValueListSubstitutingVariables(parserList, customProperties))
> 
> Ditto.

Done

>> Source/WebCore/css/CSSValueList.cpp:271
>> +            if (downcast<CSSPrimitiveValue>(*value.ptr()).buildParserValue(&result))
> 
> Ditto.

Done.

>> Source/WebCore/css/ElementRuleCollector.cpp:152
>> +    for (auto& pair : matchRequest.ruleSet->regionSelectorsAndRuleSets()) {
> 
> Could even consider naming this “region” instead of “pair”. It’s not *exactly* a region, but close enough. Because region.selector is the selector for the region, and region.ruleSet is the rule set for the region.

I change "pair" to "region".

>> Source/WebCore/css/ElementRuleCollector.cpp:153
>> +        const CSSSelector* regionSelector = pair.selector;
> 
> No need for this local variable. The pair.selector is only used once.

Done

>> Source/WebCore/css/ElementRuleCollector.cpp:156
>>              ASSERT(regionRules);
> 
> We don’t need a local variable for this either. We can still assert it if we want, but “pair.ruleSet” or “region.ruleSet” is clear enough without naming a local variable.

I remove the local variable.

>> Source/WebCore/css/MediaList.cpp:131
>> +        String medium = string.stripWhiteSpace();
> 
> The use of stripWhitespace here is likely a bug. It should probably be stripLeadingAndTrailingHTMLSpaces.

Done. Also I inclued the ""HTMLParserIdioms.h" to use the stripLeadingAndTrailingHTMLSpaces.

>> Source/WebCore/css/MediaList.cpp:323
>> +            for (auto& exp : query->expressions()) {
> 
> We should probably rename “exp” to “expression” while touching this.

Done.

>> Source/WebCore/css/MediaList.cpp:325
>>                      CSSValue* cssValue =  exp->value();
> 
> Extra space here after the "=" sign.

Done.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:142
>> +            break;
> 
> If we were keeping this local variable I would suggest putting this at the end of the loop instead of the start.
> 
> But instead, I suggest eliminating this local variable and changing the two lines of code that say:
> 
>     result = xxx;
> 
> To:
> 
>     if (xxx)
>         return true;
> 
> Then return false at the end of the function.

Wow.. I removed the local variable.

>> Source/WebCore/css/MediaQueryMatcher.cpp:120
>> +    for (auto& listener : m_listeners) {
> 
> I think here we can use the name “existingListener”.

I recover the 'queryListener' to 'listener' and change the 'listener' to 'existingListener'.

>> Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:88
>>      RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
> 
> This should be Ref, not RefPtr. Better, I would probably prefer auto rather than repeating the type CSSValue.
> 
> The return value from the function this code is in should be Ref<CSSValue>, not RefPtr. It can’t return null.

If I change the 'RefPtr<CSSValueList> to 'auto' and 'RefPtr<CSSValue>' to 'Ref<CSSValue>' , I get the build error.
"error: conversion function from 'Ref<WebCore::CSSValueList>' to 'Ref<WebCore::CSSValue>' invokes a deleted function"
So I change the return value to 'list.get()'.

>> Source/WebCore/css/StyleResolver.cpp:348
>> +    for (auto& entry : toRemove)
> 
> I think we should call this “key” rather than “entry”.

Done

>> Source/WebCore/css/StyleResolver.cpp:1350
>> +        ASSERT(regionSelectorAndRuleSet.ruleSet.get());
> 
> I don’t think we need the .get() here.

Done

>> Source/WebCore/css/StyleResolver.cpp:1357
>> +            ASSERT(regionSelectorAndRuleSet.ruleSet.get());
> 
> I don’t think we need the .get() here.

Done

>> Source/WebCore/css/StyleSheetContents.cpp:55
>> +        if (StyleSheetContents* sheet = importRule->styleSheet())
> 
> Maybe auto* here?

Done

>> Source/WebCore/css/WebKitCSSMatrix.cpp:79
>> +            if (operation.get()->apply(t, IntSize(0, 0))) {
> 
> I don’t think the .get() is needed here.

Done

[Hunseop Jeong]

Created attachment 265654 [details]
Patch

[Darin Adler]

Comment on attachment 265654 [details]
Patch

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

Looks good.

> Source/WebCore/css/CSSBasicShapes.cpp:369
>                  result.appendLiteral("/");

Not necessarily good to do in this patch: Someone should come back here and change this to append('/'). It’s always slightly less efficient to do appendLiteral on a single character literal than it is to append a character.

> Source/WebCore/css/CSSFontFace.cpp:123
> +        if (RefPtr<Font> result = source->font(fontDescription, syntheticBold, syntheticItalic, fontSelector, m_featureSettings, m_variantSettings)) {

Not necessarily good to do in this patch: I noticed a use of “result.release()” below that is incorrect. Should just be "return result;".

> Source/WebCore/css/CSSValueList.cpp:167
> +        if (value.get().traverseSubresources(handler))

It’s OK to land this “.get().” but Andreas gave us a reason to use -> instead; it will be easier to convert all the -> to . later when we get a newer compiler that supports operator. since they will be compiler errors. Whereas .get(). will continue to work and so be harder to remove.

> Source/WebCore/css/FontLoader.cpp:191
> +        dispatchEvent(event.release());

It’s no longer correct to call release() here. While this compiled at the time you posted the patch, it won’t any more. Instead this will need to be event.get().

> Source/WebCore/css/MediaList.cpp:132
> +        String medium = stripLeadingAndTrailingHTMLSpaces(string);

This is fixing a bug; the old code would strip some kinds of whitespace that it should not. When fixing a bug we want a test case that shows the proper behavior and that would have failed before. Chris Dumez wrote this type of test in a similar context in this change <http://trac.webkit.org/changeset/191349>. One way to do things would be to leave the change out of this patch and use a separate bug to both fix this to use the proper function and add an appropriate regression test.

By the way, the same problem exists below with the other call to stripWhiteSpace() and in fact with almost every call to stripWhiteSpace() everywhere in WebCore.

Not necessarily good to do in this patch: Long term it would be slightly more efficient for this to use StringView rather than String. That would require that parseMediaQuery and parseMediaDescriptor both be changed to take a StringView, and we’d also have to be careful to call the version of stripLeadingAndTrailingHTMLSpaces that takes and returns a StringView. Not something easy to do at this time and not super important.

> Source/WebCore/css/MediaList.cpp:319
> -    if (!queryCount)
> +    if (mediaQueries.isEmpty())
>          return;

This early return is not useful. We should delete it. The loop below will already efficiently do nothing if mediaQueries is empty.

[Hunseop Jeong]

Comment on attachment 265654 [details]
Patch

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

Thanks for the review.
Could you check the new patch again?

>> Source/WebCore/css/FontLoader.cpp:191
>> +        dispatchEvent(event.release());
> 
> It’s no longer correct to call release() here. While this compiled at the time you posted the patch, it won’t any more. Instead this will need to be event.get().

Done.

>> Source/WebCore/css/MediaList.cpp:132
>> +        String medium = stripLeadingAndTrailingHTMLSpaces(string);
> 
> This is fixing a bug; the old code would strip some kinds of whitespace that it should not. When fixing a bug we want a test case that shows the proper behavior and that would have failed before. Chris Dumez wrote this type of test in a similar context in this change <http://trac.webkit.org/changeset/191349>. One way to do things would be to leave the change out of this patch and use a separate bug to both fix this to use the proper function and add an appropriate regression test.
> 
> By the way, the same problem exists below with the other call to stripWhiteSpace() and in fact with almost every call to stripWhiteSpace() everywhere in WebCore.
> 
> Not necessarily good to do in this patch: Long term it would be slightly more efficient for this to use StringView rather than String. That would require that parseMediaQuery and parseMediaDescriptor both be changed to take a StringView, and we’d also have to be careful to call the version of stripLeadingAndTrailingHTMLSpaces that takes and returns a StringView. Not something easy to do at this time and not super important.

Okay, I recover the codes and separate bug to fix it with proper function and appropriate regression test.

>> Source/WebCore/css/MediaList.cpp:319
>>          return;
> 
> This early return is not useful. We should delete it. The loop below will already efficiently do nothing if mediaQueries is empty.

Done

[Hunseop Jeong]

Created attachment 265850 [details]
Patch

[Ahmad Saleem]

@simon & @zalan - Would be needed or it would be "RESOLVED LATER" or "RESOLVED WONTFIX"? Thanks!

[Alexey Proskuryakov]

A pure refactoring that was abandoned in 2015 is LATER at this point.