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
Created attachment 240197 [details] Patch
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().
Created attachment 241109 [details] Patch Updated patch
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 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?
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.
Created attachment 241350 [details] Patch v3 The changelog is missing from the previous patch.
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 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 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 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.
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.
(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 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 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.
Created attachment 241580 [details] Patch v5 The new line, and the .get() has been removed.
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 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 )
(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.
Created attachment 242194 [details] Patch v6 updated patch
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.
(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 on attachment 242194 [details] Patch v6 Clearing flags on attachment: 242194 Committed r176668: <http://trac.webkit.org/changeset/176668>
All reviewed patches have been landed. Closing bug.