[Webkit-unassigned] [Bug 46335] Add EditingStyle

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Eric Seidel <eric at webkit.org>  2010-09-29 12:43:00 PST ---
(From update of attachment 68496)
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.

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

prepareToApplyAt?

> 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?

> 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?

> WebCore/editing/SelectionController.h:249
>  inline void SelectionController::clearTypingStyle()
>  {
> -    m_typingStyle.clear();
> +    m_typingStyle->clear();
>  }
>  
>  inline void SelectionController::setTypingStyle(PassRefPtr<CSSMutableStyleDeclaration> style)
>  {
> -    m_typingStyle = style;
> +    m_typingStyle->setStyle(style);
>  }

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

> WebCore/editing/EditingStyle.cpp:80
> +    : m_mutableStyle(0)

No need to explictly 0 RefPtr.

> WebCore/editing/EditingStyle.cpp:112
> +    RefPtr<CSSComputedStyleDeclaration> computedStyleAtPosition = computedStyle(node);
> +    RefPtr<CSSMutableStyleDeclaration> style;
> +    if (!computedStyleAtPosition)
> +        style = CSSMutableStyleDeclaration::create();
> +    else
> +        style = removeNonEditingProperties(computedStyleAtPosition.get());

This feels like a helper function.

> WebCore/editing/EditingStyle.cpp:127
> +    if (node && node->computedStyle()) {
> +        RenderStyle* renderStyle = node->computedStyle();
> +        // If a node's text fill color is invalid, then its children use 
> +        // their font-color as their text fill color (they don't
> +        // inherit it).  Likewise for stroke color.
> +        ExceptionCode ec = 0;
> +        if (!renderStyle->textFillColor().isValid())
> +            style->removeProperty(CSSPropertyWebkitTextFillColor, ec);
> +        if (!renderStyle->textStrokeColor().isValid())
> +            style->removeProperty(CSSPropertyWebkitTextStrokeColor, ec);
> +        ASSERT(ec == 0);
> +        if (renderStyle->fontDescription().keywordSize())
> +            style->setProperty(CSSPropertyFontSize, computedStyleAtPosition->getFontSizeCSSValuePreferringKeyword()->cssText());
> +    }

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?

> WebCore/editing/EditingStyle.cpp:137
> +    /*    if (shouldIncludeTypingStyle == IncludeTypingStyle) {
> +     CSSMutableStyleDeclaration* typingStyle = pos.node()->document()->frame()->selection()->typingStyle();
> +     if (typingStyle)
> +     style->merge(typingStyle);
> +     }*/
> +}

Commented out code?

> WebCore/editing/EditingStyle.cpp:141
> +    return !m_mutableStyle || m_mutableStyle->length() == 0;

no isEmpty() here too?

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

What does your FIXME mean?

> WebCore/editing/EditingStyle.cpp:175
> +    RefPtr<CSSComputedStyleDeclaration> parentStyle = computedStyle(node->parentNode());
> +    RefPtr<CSSMutableStyleDeclaration> styleAddedByNode = computedStyle(node)->copy();
> +
> +    parentStyle->diff(styleAddedByNode.get());
> +    removeNonEditingProperties(styleAddedByNode.get());
> +
> +    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?

> 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.

> WebCore/editing/EditingStyle.cpp:195
> +    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
> +        const CSSProperty& property = *it;
> +        m_mutableStyle->removeProperty(property.id());
> +    }

Can remove local and {}

> WebCore/editing/EditingStyle.cpp:212
> +        if (color.alpha() == 0)

Isn't there a !transparent()?

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