SelectionController::typingStyle() should return EditingStyle* in lieu of CSSMutableStyleDeclaration*.
Created attachment 74407 [details] refactoring
Attachment 74407 [details] did not build on mac: Build output: http://queues.webkit.org/results/6230070
(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'
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.
Created attachment 74416 [details] fixed mac build
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.
(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.
(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.
Created attachment 74607 [details] fixed per Tony's comment
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.
Thanks for the review!
Committed r72573: <http://trac.webkit.org/changeset/72573>