Bug 49813 – SelectionController::typingStyle() should return EditingStyle*

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

SelectionController::typingStyle() should return EditingStyle* in lieu of CSSMutableStyleDeclaration*.

[Ryosuke Niwa]

Created attachment 74407 [details]
refactoring

[Eric Seidel (no email)]

Attachment 74407 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6230070

[Darin Adler]

(In reply to comment #2)
> Attachment 74407 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/6230070

/Projects/MacEWS/WebKit/mac/WebView/WebFrame.mm: In function 'DOMCSSStyleDeclaration* -[WebFrame(WebInternal) _typingStyle](WebFrame*, objc_selector*)':
/Projects/MacEWS/WebKit/mac/WebView/WebFrame.mm:906: error: 'class WebCore::EditingStyle' has no member named 'copy'

[Ryosuke Niwa]

Comment on attachment 74407 [details]
refactoring

(In reply to comment #3)
> (In reply to comment #2)
> > Attachment 74407 [details] [details] did not build on mac:
> > Build output: http://queues.webkit.org/results/6230070
> 
> /Projects/MacEWS/WebKit/mac/WebView/WebFrame.mm: In function 'DOMCSSStyleDeclaration* -[WebFrame(WebInternal) _typingStyle](WebFrame*, objc_selector*)':
> /Projects/MacEWS/WebKit/mac/WebView/WebFrame.mm:906: error: 'class WebCore::EditingStyle' has no member named 'copy'

I'm on it.  I'll fix and upload again.

[Ryosuke Niwa]

Created attachment 74416 [details]
fixed mac build

[Tony Chang]

Comment on attachment 74416 [details]
fixed mac build

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

> WebCore/editing/EditingStyle.cpp:206
> -    if (!node || !node->parentNode())
> +    if (!node || !node->parentNode() || !m_mutableStyle)

Is this check needed by the refactor or was it needed in the old code too?

> WebCore/editing/EditingStyle.cpp:235
> +    if (shouldPreserveWritingDirection) {

Should there be an == PreserveWritingDirection here?  This looks backwards or perhaps undefined.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 74416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74416&action=review
> 
> > WebCore/editing/EditingStyle.cpp:206
> > -    if (!node || !node->parentNode())
> > +    if (!node || !node->parentNode() || !m_mutableStyle)
> 
> Is this check needed by the refactor or was it needed in the old code too?

This should have been there in the old code as well.

> > WebCore/editing/EditingStyle.cpp:235
> > +    if (shouldPreserveWritingDirection) {
> 
> Should there be an == PreserveWritingDirection here?  This looks backwards or perhaps undefined.

Ah, good point.  I initially used a boolean variable and forgot to change it.  Will fix.

[Tony Chang]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 74416 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74416&action=review
> > 
> > > WebCore/editing/EditingStyle.cpp:206
> > > -    if (!node || !node->parentNode())
> > > +    if (!node || !node->parentNode() || !m_mutableStyle)
> > 
> > Is this check needed by the refactor or was it needed in the old code too?
> 
> This should have been there in the old code as well.

If it's possible to write a layout test that crashes this case without the null check, it would be nice to add.  If it's not possible, then don't worry about it.

[Ryosuke Niwa]

Created attachment 74607 [details]
fixed per Tony's comment

[Ryosuke Niwa]

Comment on attachment 74416 [details]
fixed mac build

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

>>>> WebCore/editing/EditingStyle.cpp:206
>>>> +    if (!node || !node->parentNode() || !m_mutableStyle)
>>> 
>>> Is this check needed by the refactor or was it needed in the old code too?
>> 
>> This should have been there in the old code as well.
> 
> If it's possible to write a layout test that crashes this case without the null check, it would be nice to add.  If it's not possible, then don't worry about it.

I don't think it's possible.  This function is only called in ReplaceSelectionCommand.cpp:626

    RefPtr<EditingStyle> sourceDocumentStyle = EditingStyle::create(static_cast<HTMLElement*>(sourceDocumentStyleSpan)->getInlineStyleDecl());
    ContainerNode* context = sourceDocumentStyleSpan->parentNode();

    // If Mail wraps the fragment with a Paste as Quotation blockquote, or if you're pasting into a quoted region,
    // styles from blockquoteNode are allowed to override those from the source document, see <rdar://problem/4930986> and <rdar://problem/5089327>.
    Node* blockquoteNode = isMailPasteAsQuotationNode(context) ? context : nearestMailBlockquote(context);
    if (blockquoteNode) {
        sourceDocumentStyle->removeStyleConflictingWithStyleOfNode(blockquoteNode);
        context = blockquoteNode->parentNode();
    }

But getInlineStyleDecl always returns a valid CSSMutableStyleDeclaration, creating an empty one if necessary.

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r72573: <http://trac.webkit.org/changeset/72573>