Summary: | StyleChange::init needs clean up before fixing the bug 23892 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | darin, eric, hyatt, justin.garcia | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 23892 | ||||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2009-07-27 18:29:53 PDT
Created attachment 33590 [details]
fixes the bug
Created attachment 33591 [details]
minor correction
SVN diff did terrible job here: Original if statments: 118 if (property->id() == CSSPropertyWhiteSpace && (isTabSpanTextNode(position.node()) || isTabSpanNode((position.node())))) 119 continue; 120 121 // If needed, figure out if this change is a legacy HTML style change. 122 if (useHTMLFormattingTags && checkForLegacyHTMLStyleChange(property)) 123 continue; 124 125 if (property->id() == CSSPropertyDirection) { 126 if (addedDirection) 127 continue; 128 addedDirection = true; 129 } 130 131 // Add this property 132 if (property->id() == CSSPropertyTextDecoration || property->id() == CSSPropertyWebkitTextDecorationsInEffect) { 133 // Always use text-decoration because -webkit-text-decoration-in-effect is internal. 134 CSSProperty alteredProperty(CSSPropertyTextDecoration, property->value(), property->isImportant()); 135 // We don't add "text-decoration: none" because it doesn't override the existing text decorations; i.e. redundant 136 if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none")) 137 styleText += alteredProperty.cssText(); 138 } else 139 styleText += property->cssText(); 140 141 if (!addedDirection && property->id() == CSSPropertyUnicodeBidi) { 142 styleText += "direction: " + style->getPropertyValue(CSSPropertyDirection) + "; "; 143 addedDirection = true; 144 } My switch statement: switch (property->id()) { case CSSPropertyWhiteSpace: // Changing the whitespace style in a tab span would collapse the tab into a space. if (isTabSpanTextNode(position.node()) || isTabSpanNode(position.node())) continue; break; case CSSPropertyDirection: if (!addedDirection) { styleText += property->cssText(); addedDirection = true; } continue; case CSSPropertyUnicodeBidi: if (!addedDirection) { styleText += "direction: " + style->getPropertyValue(CSSPropertyDirection) + "; "; addedDirection = true; } break; case CSSPropertyTextDecoration: case CSSPropertyWebkitTextDecorationsInEffect: // Always use text-decoration because -webkit-text-decoration-in-effect is internal. CSSProperty alteredProperty(CSSPropertyTextDecoration, property->value(), property->isImportant()); // We don't add "text-decoration: none" because it doesn't override the existing text decorations; i.e. redundant if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none")) styleText += alteredProperty.cssText(); continue; } // If needed, convert the CSS property to HTML tags by turning on m_apply* styleText += useHTMLFormattingTags ? convertCSSPropertyToApplyFlags(property) : property->cssText(); Comment on attachment 33591 [details]
minor correction
Maybe convertCSSPropertyToApplyFlags should just be "styleText += cssTextForPropertyRespectingLegacyTags(propertyId, useHTMLFormattingTags)"
or
"checkForFormatingTagsAndReturnCSSText"
or
"cssTextIgnoringFormattingTags"
Any of these would just take the useHTMLFormattingTags parameter, and then you wouldn't have two places where you have to call property->cssText()
Created attachment 33652 [details]
fixed per comment
(In reply to comment #4) > (From update of attachment 33591 [details]) > Maybe convertCSSPropertyToApplyFlags should just be "styleText += > cssTextForPropertyRespectingLegacyTags(propertyId, useHTMLFormattingTags)" I was considering this earlier but I thought passing boolean value and bailing out right away isn't so elegant. But I guess that's much cleaner. > "cssTextIgnoringFormattingTags" I like the name It turned out that we need more substantial clean up. Created attachment 34712 [details]
fixes the bug, a minor rebaseline is required
Comment on attachment 34712 [details] fixes the bug, a minor rebaseline is required > + void cleanupTextDecorationProperties(CSSMutableStyleDeclaration* style); The verb "clean up" is two words, so this would be "cleanUpTextDecorationProperties". But it's not clear what "clean up" means and I'd like the function to have a better name. I think a better verb might be "reconcile" or "standardize". This function needs a name that explains what it does as clearly as possible. The declaration of this function should not have a name for the argument "style" since the type is unambiguous. Same for the other function added in this patch. > + RefPtr<CSSValue> textDecorationsInEffect = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect); > + RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration); > + // We shouldn't have both text-decoration and -webkit-text-decorations-in-effect because that wouldn't make sense. > + ASSERT(!textDecorationsInEffect || !textDecoration); The textDecoration value fetched here is never used except in the assertion. It would be slightly better not to even make the getPropertyCSSValue call in debug builds. > + textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration); > + if (textDecoration && !textDecoration->isValueList()) > + style->removeProperty(CSSPropertyTextDecoration); I don't understand the purpose of removing the text-decoration property when its value is not a value list. The code needs a comment explaining why this is a good idea. > + if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontWeight), "bold")) { Even though the old code already did this, it would be more efficient to do this check on CSSValue rather than constructing a string with the text form of the value just so you can compare with a constant string. You can make a helper function to help automate checking that it's a CSSPrimitiveValue, then calling getIdent() and checking for the particular identifier value. > + if (equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle), "italic") > + || equalIgnoringCase(style->getPropertyValue(CSSPropertyFontStyle), "oblique")) { Ditto. > + if (equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign), "sub")) { > + style->removeProperty(CSSPropertyVerticalAlign); > + m_applySubscript = true; > + } else if (equalIgnoringCase(style->getPropertyValue(CSSPropertyVerticalAlign), "super")) { > + style->removeProperty(CSSPropertyVerticalAlign); > + m_applySuperscript = true; > + } Ditto. It would also be more efficient to not get the same property value twice. > + if (RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor)) { > + RGBA32 rgba = 0; > + CSSParser::parseColor(rgba, colorValue->cssText()); > + Color color(rgba); > + m_applyFontColor = color.name(); > + style->removeProperty(CSSPropertyColor); > + } Is there no more efficient way to find out what color is specified by a CSSValue than to convert to a string and then parse the color? For example, would the getRGBA32Value function cover enough cases? Or do we also have to parse color strings? > + RefPtr<CSSValue> fontSize = style->getPropertyCSSValue(CSSPropertyFontSize); > + if (fontSize) { > + if (!fontSize->isPrimitiveValue()) > + style->removeProperty(CSSPropertyFontSize); // Can't make sense of the number. Put no font size. > + else { > + CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(fontSize.get()); > > + if (value->primitiveType() < CSSPrimitiveValue::CSS_PX || value->primitiveType() > CSSPrimitiveValue::CSS_PC) > + ;// Size keyword or relative unit. The formatting here is confusing. I suggest using braces instead of just a semicolon. I also suggest a longer comment, and if it's two lines then it would justify having braces! Understood that it's a size keyword or a relative unit, but why are does this code do nothing in that case? Is it something we want to handle later or is it already correct? > + RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy(); > CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get()); > + for (size_t i = 0; i < computedValueList->length(); i++) > + resultValueList->removeAll(computedValueList->item(i)); > + result->setProperty(textDecorationProperties[n], resultValueList->cssText()); I don't understand why you're copying the CSSValueList here. If the list is "live" and associated with the original property, then it seems that we could just modify the list and we wouldn't need to call setProperty at all. If the list is not live and is not associated with the property, then it seems we could modify it without making a copy. I'm going to say review- because there are a few items above that might inspire code changes. Generally the patch seems quite good. Thanks you for your feedback! (In reply to comment #9) > I'd like the function to have a better name. I think a better verb might be > "reconcile" or "standardize". This function needs a name that explains what it > does as clearly as possible. I'll use reconcileTextDecorationProperties unless we come up with a better name. > The declaration of this function should not have a name for the argument > "style" since the type is unambiguous. Same for the other function added in > this patch. Ugh, style glitch. Removed. > The textDecoration value fetched here is never used except in the assertion. It > would be slightly better not to even make the getPropertyCSSValue call in debug > builds. I changed the logic slightly so that textDecoration is used when textDecorationsInEffect is 0. > I don't understand the purpose of removing the text-decoration property when > its value is not a value list. The code needs a comment explaining why this is > a good idea. We don't want to add "text-decoration: none" because it's redundant > Even though the old code already did this, it would be more efficient to do > this check on CSSValue rather than constructing a string with the text form of > the value just so you can compare with a constant string. You can make a helper > function to help automate checking that it's a CSSPrimitiveValue, then calling > getIdent() and checking for the particular identifier value. I added int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID) don't know why I didn't come up with this earlier. > It would also be more efficient to not get the same property value twice. Done. A local copy of the identifier is made. > > + if (RefPtr<CSSValue> colorValue = style->getPropertyCSSValue(CSSPropertyColor)) { > > + RGBA32 rgba = 0; > > + CSSParser::parseColor(rgba, colorValue->cssText()); > > + Color color(rgba); > > + m_applyFontColor = color.name(); > > + style->removeProperty(CSSPropertyColor); > > + } > > Is there no more efficient way to find out what color is specified by a > CSSValue than to convert to a string and then parse the color? For example, > would the getRGBA32Value function cover enough cases? Or do we also have to > parse color strings? I'm working on this. > > + CSSPrimitiveValue* value = static_cast<CSSPrimitiveValue*>(fontSize.get()); > > > > + if (value->primitiveType() < CSSPrimitiveValue::CSS_PX || value->primitiveType() > CSSPrimitiveValue::CSS_PC) > > + ;// Size keyword or relative unit. > > The formatting here is confusing. I suggest using braces instead of just a > semicolon. I also suggest a longer comment, and if it's two lines then it would > justify having braces! Understood that it's a size keyword or a relative unit, > but why are does this code do nothing in that case? Is it something we want to > handle later or is it already correct? In the original function, there was a break statement so I was intending to negate the condition and delete the empty if but totally forgot. Now done. > > > + RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy(); > > CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get()); > > + for (size_t i = 0; i < computedValueList->length(); i++) > > + resultValueList->removeAll(computedValueList->item(i)); > > + result->setProperty(textDecorationProperties[n], resultValueList->cssText()); > > I don't understand why you're copying the CSSValueList here. If the list is > "live" and associated with the original property, then it seems that we could > just modify the list and we wouldn't need to call setProperty at all. If the > list is not live and is not associated with the property, then it seems we > could modify it without making a copy. So it turned out that when we copy() computed style, it won't create a copy of CSSValueList. i.e. we will be changing the value of both mutable object and computed style. Take a look at the lines 147-161 of CSSStyleDeclaration. It only copies CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means CSSValueList isn't really copied. I think this is a bad implementation of copy, but if we decided to copy every single CSSValue then performance may regress significantly, so I won't touch it this time. I should add a comment maybe? > I'm going to say review- because there are a few items above that might inspire > code changes. Generally the patch seems quite good. Thanks for the review. I'm posting the follow up patch shortly. (In reply to comment #10) > > I don't understand the purpose of removing the text-decoration property when > > its value is not a value list. The code needs a comment explaining why this is > > a good idea. > > We don't want to add "text-decoration: none" because it's redundant Needs a comment. !isValueList() is not an obvious way to check for "none". > > Even though the old code already did this, it would be more efficient to do > > this check on CSSValue rather than constructing a string with the text form of > > the value just so you can compare with a constant string. You can make a helper > > function to help automate checking that it's a CSSPrimitiveValue, then calling > > getIdent() and checking for the particular identifier value. > > I added > int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID) I don't think that's quite the right name. A primitive value is not necessarily an identifier, and I think you need a name that mentions the word identifier. > > > + RefPtr<CSSValueList> resultValueList = static_cast<CSSValueList*>(resultValue.get())->copy(); > > > CSSValueList* computedValueList = static_cast<CSSValueList*>(computedValue.get()); > > > + for (size_t i = 0; i < computedValueList->length(); i++) > > > + resultValueList->removeAll(computedValueList->item(i)); > > > + result->setProperty(textDecorationProperties[n], resultValueList->cssText()); > > > > I don't understand why you're copying the CSSValueList here. If the list is > > "live" and associated with the original property, then it seems that we could > > just modify the list and we wouldn't need to call setProperty at all. If the > > list is not live and is not associated with the property, then it seems we > > could modify it without making a copy. > > So it turned out that when we copy() computed style, it won't create a copy of > CSSValueList. > i.e. we will be changing the value of both mutable object and computed style. > > Take a look at the lines 147-161 of CSSStyleDeclaration. It only copies > CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means > CSSValueList isn't really copied. I think this is a bad implementation of > copy, but if we decided to copy every single CSSValue then performance may > regress significantly, so I won't touch it this time. I should add a comment > maybe? I don't understand how a computed style is involved here. The CSSValueList you are copying is from resultValue. Can that be a a computed style? (In reply to comment #11) > Needs a comment. !isValueList() is not an obvious way to check for "none". I'm adding comment. > > int getPrimitiveValue(CSSMutableStyleDeclaration* style, int propertyID) > > I don't think that's quite the right name. A primitive value is not necessarily > an identifier, and I think you need a name that mentions the word identifier. How about getPrimitiveValueIdentifier then? > > So it turned out that when we copy() computed style, it won't create a copy of > > CSSValueList. > > i.e. we will be changing the value of both mutable object and computed style. > > > > Take a look at the lines 147-161 of CSSStyleDeclaration. It only copies > > CSSProperty but CSSProperty merely copies the RefPtr to CSSValue* which means > > CSSValueList isn't really copied. I think this is a bad implementation of > > copy, but if we decided to copy every single CSSValue then performance may > > regress significantly, so I won't touch it this time. I should add a comment > > maybe? > > I don't understand how a computed style is involved here. The CSSValueList you > are copying is from resultValue. Can that be a a computed style? So we're creating mutable style by calling computedStyle->copy(). But then this copy function does NOT copy CSSValue object. It merely copies the pointer. So if we alter the content of CSSValue, we'll be modifying the content of all computed / mutable styles that share this CSSValueList. And it turned out that there is a bug that forces us to re-parse color. I'll file a bug and possibility fix it. (In reply to comment #12) > How about getPrimitiveValueIdentifier then? I think getIdentifierValue would be even better. Returns the identifier of a value unless it's not an identifier in which case it returns 0. > So we're creating mutable style by calling computedStyle->copy(). But then this > copy function does NOT copy CSSValue object. It merely copies the pointer. So > if we alter the content of CSSValue, we'll be modifying the content of all > computed / mutable styles that share this CSSValueList. I think we should take the risk of slowing down performance and fix this in the style copy function. The code is getting more twisted and we need it to be less twisted. Complicated rules about when we need to copy create trouble. > And it turned out that there is a bug that forces us to re-parse color. I'll > file a bug and possibility fix it. OK. (In reply to comment #13) > I think getIdentifierValue would be even better. Returns the identifier of a > value unless it's not an identifier in which case it returns 0. Ok, I'll use that. > > So we're creating mutable style by calling computedStyle->copy(). But then this > > copy function does NOT copy CSSValue object. It merely copies the pointer. So > > if we alter the content of CSSValue, we'll be modifying the content of all > > computed / mutable styles that share this CSSValueList. > > I think we should take the risk of slowing down performance and fix this in the > style copy function. The code is getting more twisted and we need it to be less > twisted. Complicated rules about when we need to copy create trouble. Eric S. mentioned that we're trying to avoid copying computed styles and CSSValue to minimize memory usage and such. I think changing copy method is quite expensive operation since it's used everywhere. One idea Eric broguht up is to make the return value of getPropertyCSSValue const so that we don't accidently modify it. I think it's quite misleading that it returns RefPtr. I first though it'll return new object because of it. +hyatt since he knows a lot about CSS. (In reply to comment #14) > One idea Eric broguht up > is to make the return value of getPropertyCSSValue const so that we don't > accidently modify it. I think it's quite misleading that it returns RefPtr. I > first though it'll return new object because of it. It returns PassRefPtr because it returns a new object in the case of computed style. I think we may want a different call for use when you have a mutable style. Maybe we can move getPropertyCSSValue out of the base class into the mutable style derived class? (In reply to comment #13) > > And it turned out that there is a bug that forces us to re-parse color. I'll > > file a bug and possibility fix it. > > OK. Filed: CSSMutableStyleDeclaration::setProperty does not parse color property https://bugs.webkit.org/show_bug.cgi?id=28282 Created attachment 34791 [details]
fixed per comment
(In reply to comment #15) > It returns PassRefPtr because it returns a new object in the case of computed > style. > > I think we may want a different call for use when you have a mutable style. > > Maybe we can move getPropertyCSSValue out of the base class into the mutable > style derived class? That might be a good call. I'll look into it. But that should be a separate patch though. This cleanup is already getting pretty big. Talked with Eric S. in person. It seems that we need duplicateCSSValues that deep-copies the content of a CSSStyleDeclaration. For time being, we can implement this with just iterating through properties and call setProperty for each property. But ultimately, we want copy method in each derived class of CSSValue to speed up this. (In reply to comment #19) > Talked with Eric S. in person. It seems that we need duplicateCSSValues that > deep-copies the content of a CSSStyleDeclaration. For time being, we can > implement this with just iterating through properties and call setProperty for > each property. But ultimately, we want copy method in each derived class of > CSSValue to speed up this. Mn... this might not be as efficient as we want it to be. > Maybe we can move getPropertyCSSValue out of the base class into the mutable > style derived class? Maybe that we should copy when getPropertyCSSValue is first called for the mutable style. Either way, it's not clear to me whether we should copy every property at once or should copy on demand. + and adding functions cleanupTextDecorationProperties and extractTextStyle to handle the style. Typo new name is reconcileTextDecorationProperties. Created attachment 34995 [details]
removed the fix on how text decorations are trimmed, no behavioral change
Created attachment 34998 [details]
made formatted diff more readable
Comment on attachment 34998 [details]
made formatted diff more readable
Changes look good. I'm saying r=me.
But doesn't this patch change the results of some layout tests? Where are the changes to the expected results?
(In reply to comment #24) > (From update of attachment 34998 [details]) > Changes look good. I'm saying r=me. > > But doesn't this patch change the results of some layout tests? Where are the > changes to the expected results? No, the code I had in getPropertiesNotInComputedStyle was preparation to use it in other places. But since that patch hasn't been submitted yet, there is no harm in replacing it with a simpler code. In fact, that was why the bug survived there without being noticed by me. Comment on attachment 34998 [details] made formatted diff more readable Clearing flags on attachment: 34998 Committed r47466: <http://trac.webkit.org/changeset/47466> All reviewed patches have been landed. Closing bug. |