[Webkit-unassigned] [Bug 46335] Add EditingStyle

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46335





--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org>  2010-09-29 18:49:53 PST ---
(From update of attachment 68496)
Thanks for the feedback!  I'll update a new smaller patch soon.

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

>> WebCore/editing/CompositeEditCommand.cpp:935
>> +        styleInEmptyParagraph = editingStyleWithTypingStyle(startOfParagraphToMove.deepEquivalent());
> 
> Maybe editingStyleIncludingTypingStyle?  I'm not sure what exactly this method means.

Fixed.

>> WebCore/editing/ReplaceSelectionCommand.cpp:641
>> +    sourceDocumentStyle->prepareAppliedAt(Position(context, 0));
> 
> prepareToApplyAt?

Fixed.

>> WebCore/editing/ReplaceSelectionCommand.cpp:689
>> +    setNodeAttribute(static_cast<Element*>(copiedRangeStyleSpan), styleAttr, copiedRangeStyle->style()->cssText());
> 
> Are we eventually going to hide the existance of the style() member?  And expose only the functions callers need?  Or will EditingStyle always expose to the world that it "has" a CSSMutableStyleDeclaration?

Yes, that's the plan.  I want to centralize all style manipulations in editing at one place so that we have a better control of it.

>> WebCore/editing/Editor.cpp:3262
>> +    mutableStyle->mergeAndOverrideWith(typingStyle.get());
> 
> This might be cleaner as a merge( fucntion with an OverrideWithNew enum (or some similarly named enum to explain the differnet modes).  Do we have a non-overriding merge?

I'm removing mergeAndOverrideWith from my initial patch.

>> WebCore/editing/SelectionController.h:249
>>  }
> 
> I guess not in this patch, but do we need to be clearing the whole typing style?  It seems strange to only clear its style member.  I understand you're not trying to change behavior here, but it's unclear to me why this part of code would only want to clear "part" of an EditingStyle

I set m_shouldUseFixedDefaultFontSize = false in setStyle / clear so I think we're clearing the whole editing style.  I'm not sure what you're referring to by "part".

>> WebCore/editing/EditingStyle.cpp:80
>> +    : m_mutableStyle(0)
> 
> No need to explictly 0 RefPtr.

Good point.  Removed.

>> WebCore/editing/EditingStyle.cpp:112
>> +        style = removeNonEditingProperties(computedStyleAtPosition.get());
> 
> This feels like a helper function.

Added editingStyleFromCompuedStyle.

>> WebCore/editing/EditingStyle.cpp:127
>> +    }
> 
> This also feels like a separate function.  Once you break it out into a seprate function, maybe we can move it to somewhere else more fitting?

Added removeTextFillAndStroleColorsIfNeeded and replaceFontSizeByKeywordIfPossible.

>> WebCore/editing/EditingStyle.cpp:137
>> +}
> 
> Commented out code?

Oops, removed.

>> WebCore/editing/EditingStyle.cpp:147
>> +    m_shouldUseFixedDefaultFontSize = false; // FIXME: we should take care of this
> 
> What does your FIXME mean?

Fixed.

>> WebCore/editing/EditingStyle.cpp:175
>> +    styleAddedByNode->diff(m_mutableStyle.get());
> 
> This feels wrong.  Why are we copying here?  If this is wrong, please add a FIXME, otherwise maybe a comment to explain why this is here?

I'm removing this function from my initial patch.

>> WebCore/editing/EditingStyle.cpp:186
>> +void EditingStyle::removePropertiesPresentIn(CSSMutableStyleDeclaration* style)
> 
> Python calls this operation:
> difference_update(other, ...)
> set -= other | ...
> 
> But I'm not sure differenceUpdate makes any more sense.  But this does feel like a set operation.  removePropertiesSharedWith?  removeProperitesAlsoIn?  removePropertiesPresentIn is actually a fine name.

I'm removing this one as well.

>> WebCore/editing/EditingStyle.cpp:212
>> +        if (color.alpha() == 0)
> 
> Isn't there a !transparent()?

No.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.