Bug 137918

Summary: Use references instead of pointers in EditingStyle
Product: WebKit Reporter: Tibor Mészáros <mtiborinf>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, darin, ossy, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
cdumez: review-, cdumez: commit-queue-
Patch v2
none
Patch v3
cdumez: review+, cdumez: commit-queue-
Patch v4
none
Patch v5
darin: review+
Patch v6 none

Description Tibor Mészáros 2014-10-21 05:01:34 PDT
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
Comment 1 Tibor Mészáros 2014-10-21 05:16:48 PDT
Created attachment 240197 [details]
Patch
Comment 2 Darin Adler 2014-10-21 15:51:12 PDT
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().
Comment 3 Tibor Mészáros 2014-11-06 08:21:41 PST
Created attachment 241109 [details]
Patch

Updated patch
Comment 4 Chris Dumez 2014-11-06 10:04:57 PST
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?
Comment 5 Chris Dumez 2014-11-06 10:11:27 PST
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?
Comment 6 Tibor Mészáros 2014-11-11 04:06:22 PST
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.
Comment 7 Tibor Mészáros 2014-11-11 04:09:28 PST
Created attachment 241350 [details]
Patch v3

The changelog is missing from the previous patch.
Comment 8 Chris Dumez 2014-11-11 09:34:05 PST
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))
Comment 9 Darin Adler 2014-11-11 09:35:34 PST
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.
Comment 10 Darin Adler 2014-11-11 09:36:00 PST
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!
Comment 11 Csaba Osztrogonác 2014-11-13 07:12:32 PST
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.
Comment 12 Tibor Mészáros 2014-11-13 07:38:44 PST
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.
Comment 13 Tibor Mészáros 2014-11-13 08:02:42 PST
(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.
Comment 14 Csaba Osztrogonác 2014-11-13 08:08:52 PST
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.
Comment 15 Darin Adler 2014-11-13 20:30:04 PST
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.
Comment 16 Tibor Mészáros 2014-11-14 04:44:12 PST
Created attachment 241580 [details]
Patch v5

The new line, and the .get() has been removed.
Comment 17 Darin Adler 2014-11-14 17:22:34 PST
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?
Comment 18 Csaba Osztrogonác 2014-11-14 18:03:43 PST
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 )
Comment 19 Csaba Osztrogonác 2014-11-17 06:37:03 PST
(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.
Comment 20 Tibor Mészáros 2014-11-25 06:46:27 PST
Created attachment 242194 [details]
Patch v6

updated patch
Comment 21 Tibor Mészáros 2014-11-25 06:57:47 PST
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.
Comment 22 Csaba Osztrogonác 2014-12-02 09:14:14 PST
(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?
Comment 23 WebKit Commit Bot 2014-12-02 09:58:52 PST
Comment on attachment 242194 [details]
Patch v6

Clearing flags on attachment: 242194

Committed r176668: <http://trac.webkit.org/changeset/176668>
Comment 24 WebKit Commit Bot 2014-12-02 09:58:58 PST
All reviewed patches have been landed.  Closing bug.