Bug 137918 – Use references instead of pointers in EditingStyle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tibor Mészáros]

As Darin Adler suggested:
"Good idea for future refactoring. Both arguments to this function template should be references."

Reference url: https://bugs.webkit.org/show_bug.cgi?id=133796#c20

[Tibor Mészáros]

Created attachment 240197 [details]
Patch

[Darin Adler]

Comment on attachment 240197 [details]
Patch

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

Looks good.

> Source/WebCore/editing/EditingStyle.cpp:648
>  template<typename T>
>  TriState EditingStyle::triStateOfStyle(T* styleToCompare, ShouldIgnoreTextOnlyProperties shouldIgnoreTextOnlyProperties) const

Seems like T* needs to change to T& here too, in a subsequent patch.

> Source/WebCore/editing/EditingStyle.cpp:851
> +    return getPropertiesNotIn(*m_mutableStyle.get(), computedStyle)->isEmpty();

No need for ".get()" here or in the many other places that do *x.get().

[Tibor Mészáros]

Created attachment 241109 [details]
Patch

Updated patch

[Chris Dumez]

Comment on attachment 241109 [details]
Patch

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

r- because at least one of the dereference does not seem safe.

> Source/WebCore/editing/EditingStyle.cpp:54
> +#include <stdio.h>

Why?

> Source/WebCore/editing/EditingStyle.cpp:136
> +        return propertyCSSValue;

return propertyCSSValue.release();

> Source/WebCore/editing/EditingStyle.cpp:137
> +    return PassRefPtr<CSSValue>();

We usually just return nullptr; as this is shorter.

> Source/WebCore/editing/EditingStyle.cpp:344
> +    m_mutableStyle = style.mutableCopy();

Can this be moved to the initializer list now?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:489
> +        RefPtr<EditingStyle> newInlineStyle = EditingStyle::create(*inlineStyle);

Why is this safe? There is a null check right after...

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:791
> +    RefPtr<EditingStyle> style = EditingStyle::create(*wrappingStyleSpan->inlineStyle());

Why is this safe?

[Chris Dumez]

Comment on attachment 241109 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:673
> +        return triStateOfStyle(*EditingStyle::styleAtSelectionStart(selection));

EditingStyle::styleAtSelectionStart() can return null, why is it safe?

[Tibor Mészáros]

Created attachment 241349 [details]
Patch v2

Almost all of the suggested change has been taken, but some not:

> Source/WebCore/editing/EditingStyle.cpp:344
> +    m_mutableStyle = style.mutableCopy();

Can this be moved to the initializer list now?

No, it could not be moved to initializer list, because style could be nullptr.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:489
> +        RefPtr<EditingStyle> newInlineStyle = EditingStyle::create(*inlineStyle);

Why is this safe? There is a null check right after...

This change has been removed from the patch.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:791
> +    RefPtr<EditingStyle> style = EditingStyle::create(*wrappingStyleSpan->inlineStyle());

Why is this safe?

This change has been removed too.

[Tibor Mészáros]

Created attachment 241350 [details]
Patch v3

The changelog is missing from the previous patch.

[Chris Dumez]

Comment on attachment 241350 [details]
Patch v3

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

r=me with one fix before landing.

> Source/WebCore/editing/EditingStyle.cpp:1520
> +    if (!is<CSSPrimitiveValue>(&fontWeight))

No, this is wrong. You will do an unnecessary null check. Just use if (!is<CSSPrimitiveValue>(fontWeight))

[Darin Adler]

Comment on attachment 241350 [details]
Patch v3

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

> Source/WebCore/editing/EditingStyle.cpp:135
> +    if (RefPtr<CSSValue> propertyCSSValue = style.getPropertyCSSValue(propertyID))
> +        return propertyCSSValue.release();
> +    return 0;

There is no reason for this if statement. Just:

    return style.getPropertyCSSValue(propertyID);

Also, please use nullptr, not 0, in the future.

>> Source/WebCore/editing/EditingStyle.cpp:1520
>> -    if (!is<CSSPrimitiveValue>(fontWeight))
>> +    if (!is<CSSPrimitiveValue>(&fontWeight))
> 
> No, this is wrong. You will do an unnecessary null check. Just use if (!is<CSSPrimitiveValue>(fontWeight))

There is no need for this change. The is template should work on references as well as pointers.

[Darin Adler]

Comment on attachment 241350 [details]
Patch v3

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

>>> Source/WebCore/editing/EditingStyle.cpp:1520
>>> +    if (!is<CSSPrimitiveValue>(&fontWeight))
>> 
>> No, this is wrong. You will do an unnecessary null check. Just use if (!is<CSSPrimitiveValue>(fontWeight))
> 
> There is no need for this change. The is template should work on references as well as pointers.

Oops, my comment was not a response to Chris’s comment. In fact I was trying to say the same thing he was!

[Csaba Osztrogonác]

Comment on attachment 241350 [details]
Patch v3

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

> Source/WebCore/editing/EditingStyle.cpp:1550
> -    return fontWeightIsBold(extractPropertyValue(style, CSSPropertyFontWeight).get());
> +    return fontWeightIsBold(*extractPropertyValue(style, CSSPropertyFontWeight).get());

It isn't safe, extractPropertyValue() can return with nullptr.
In this case you should return false.

[Tibor Mészáros]

Created attachment 241485 [details]
Patch v4

I have updated the patch. The problem with it:
+    if (!is<CSSPrimitiveValue>(&fontWeight))

was that Csaba said, so I fixed the code as He suggested.

[Tibor Mészáros]

(In reply to comment #9)
> Comment on attachment 241350 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241350&action=review
> 
> > Source/WebCore/editing/EditingStyle.cpp:135
> > +    if (RefPtr<CSSValue> propertyCSSValue = style.getPropertyCSSValue(propertyID))
> > +        return propertyCSSValue.release();
> > +    return 0;
> 
> There is no reason for this if statement. Just:
> 
>     return style.getPropertyCSSValue(propertyID);
> 
> Also, please use nullptr, not 0, in the future.

Ok, I will use nullptr instead of 0, in the future.

> 
> >> Source/WebCore/editing/EditingStyle.cpp:1520
> >> -    if (!is<CSSPrimitiveValue>(fontWeight))
> >> +    if (!is<CSSPrimitiveValue>(&fontWeight))
> > 
> > No, this is wrong. You will do an unnecessary null check. Just use if (!is<CSSPrimitiveValue>(fontWeight))
> 
> There is no need for this change. The is template should work on references
> as well as pointers.

I have updated this part of the code, as Chris, Csaba and You suggested.

[Csaba Osztrogonác]

Comment on attachment 241485 [details]
Patch v4

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

> Source/WebCore/editing/EditingStyle.cpp:1163
> +    

Please remove this empty line before landing.

[Darin Adler]

Comment on attachment 241485 [details]
Patch v4

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

> Source/WebCore/editing/EditingStyle.cpp:1548
> +    RefPtr<CSSValue> fontWeight = extractPropertyValue(style, CSSPropertyFontWeight).get();

There is no need for the ".get()" here.

[Tibor Mészáros]

Created attachment 241580 [details]
Patch v5

The new line, and the .get() has been removed.

[Darin Adler]

Comment on attachment 241580 [details]
Patch v5

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

r=me as long as you can give a satisfactory answer to all the questions about how we know various pointers are non-null

> Source/WebCore/editing/EditingStyle.cpp:651
> +    RefPtr<MutableStyleProperties> difference = getPropertiesNotIn(*m_mutableStyle, styleToCompare);

What guarantees that m_mutableStyle is non-null here?

> Source/WebCore/editing/EditingStyle.cpp:1412
> +        extractTextStyles(document, *mutableStyle, computedStyle.useFixedFontDefaultSize());

What guarantees that mutableStyle is non-null here? Can getPropertiesNotIn ever return null?

> Source/WebCore/editing/EditingStyle.cpp:1548
> +    return fontWeight ? fontWeightIsBold(*fontWeight) : false;

Forgot to mention this last time. I think this is even nicer with &&:

    return fontWeight && fontWeightIsBold(*fontWeight);

> Source/WebCore/editing/EditingStyle.cpp:1559
> +    diffTextDecorations(*mutableStyle, CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());

What guarantees mutableStyle is not null? Can EditingStyle::style ever return null?

[Csaba Osztrogonác]

Comment on attachment 241580 [details]
Patch v5

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

>> Source/WebCore/editing/EditingStyle.cpp:1559
>> +    diffTextDecorations(*mutableStyle, CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
> 
> What guarantees mutableStyle is not null? Can EditingStyle::style ever return null?

Theoretically (without knowing the exact logic) it can, it simply returns m_mutableStyle.get().

But assuming if it can return nullptr, it isn't safe before this change, because diffTextDecorations()
uses it without nullptr check: RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(propertID);
( http://trac.webkit.org/browser/trunk/Source/WebCore/editing/EditingStyle.cpp#L1502 )

[Csaba Osztrogonác]

(In reply to https://bugs.webkit.org/show_bug.cgi?id=133796#c20)
> >> Source/WebCore/editing/EditingStyle.cpp:1557
> >> +    ASSERT(baseStyle);
> > 
> > If you're going to ASSERT this, why not make it a reference?
> Good idea for future refactoring. Both arguments to this function template
> should be references.

The original idea was to refactor the code to use references instead
of pointers for the arguments of extractPropertiesNotIn(...):
https://trac.webkit.org/browser/trunk/Source/WebCore/editing/EditingStyle.cpp?rev=175067#L1553

But I think this patch tries to do too much thing at once. I tried to review
it in details and find how to address the issues that Darin noticed, but it
is to find the proper fix or answer quickly.

So I suggest splitting up this patch to smaller pieces. The original idea
can be a good starting point. Converting the second argument (baseStyle)
from pointer to reference seems to be easy and safe at all. 

But I'm not sure if it is possible to do the same with the first argument
(styleWithRedundantProperties), because extractPropertiesNotIn(...) is called
from getPropertiesNotIn(...) directly and getPropertiesNotIn(...) is called
from many places - most of them seems to be safe (null pointer checked), except
one call: https://trac.webkit.org/browser/trunk/Source/WebCore/editing/EditingStyle.cpp?rev=175067#L651

Maybe it can be fixed with a null pointer check, but I have no idea what
return value should triStateOfStyle() have if m_mutableStyle is null.
Darin, Ryosuke? Maybe you have any idea for this issue.

[Tibor Mészáros]

Created attachment 242194 [details]
Patch v6

updated patch

[Tibor Mészáros]

Comment on attachment 241580 [details]
Patch v5

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

>> Source/WebCore/editing/EditingStyle.cpp:651
>> +    RefPtr<MutableStyleProperties> difference = getPropertiesNotIn(*m_mutableStyle, styleToCompare);
> 
> What guarantees that m_mutableStyle is non-null here?

Good point, I added a nullptr check to guarantee that m_mutableStyle isn't nullptr.
This case was previously handled by if(difference->isEmpty()) check below.

>> Source/WebCore/editing/EditingStyle.cpp:1412
>> +        extractTextStyles(document, *mutableStyle, computedStyle.useFixedFontDefaultSize());
> 
> What guarantees that mutableStyle is non-null here? Can getPropertiesNotIn ever return null?

mutableStyle is the return value of getPropertiesNotIn(*style->style(), computedStyle). getPropertiesNotIn() simply pass through the return value of extractPropertiesNotIn(), which can't be nullptr.

>> Source/WebCore/editing/EditingStyle.cpp:1548
>> +    return fontWeight ? fontWeightIsBold(*fontWeight) : false;
> 
> Forgot to mention this last time. I think this is even nicer with &&:
> 
>     return fontWeight && fontWeightIsBold(*fontWeight);

Thanks, I fixed it too.

>>> Source/WebCore/editing/EditingStyle.cpp:1559
>>> +    diffTextDecorations(*mutableStyle, CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
>> 
>> What guarantees mutableStyle is not null? Can EditingStyle::style ever return null?
> 
> Theoretically (without knowing the exact logic) it can, it simply returns m_mutableStyle.get().
> 
> But assuming if it can return nullptr, it isn't safe before this change, because diffTextDecorations()
> uses it without nullptr check: RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(propertID);
> ( http://trac.webkit.org/browser/trunk/Source/WebCore/editing/EditingStyle.cpp#L1502 )

Result is copied from the non null styleWithRedundantProperties, which guarantees that result->style() alias mutableStyle is non nullptr.

[Csaba Osztrogonác]

(In reply to comment #21)
> Comment on attachment 241580 [details]
> Patch v5
...
The answers convinced me, I can't find any other issue.
Darin, do you agree if it is ready for landing now?

[WebKit Commit Bot]

Comment on attachment 242194 [details]
Patch v6

Clearing flags on attachment: 242194

Committed r176668: <http://trac.webkit.org/changeset/176668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.