[webkit-reviews] review granted: [Bug 46840] FormatBlockCommand and IndentOutdentCommand should use the same code to iterate paragraphs : [Attachment 69525] adds an abstract class to be inherited by FormatBlockCommand and IndentOutdentCommand

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 46840: FormatBlockCommand and IndentOutdentCommand should use the same code
to iterate paragraphs
https://bugs.webkit.org/show_bug.cgi?id=46840

Attachment 69525: adds an abstract class to be inherited by FormatBlockCommand
and IndentOutdentCommand
https://bugs.webkit.org/attachment.cgi?id=69525&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69525&action=review

It might have been better to start ApplyBlockElementCommand.cpp as a copy of
IndentOutdentCommand.cpp so we could see the unchanged code that came from
there and preserve the change history of the code. Now it looks like you wrote
all of it!

> WebCore/editing/ApplyBlockElementCommand.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Did you write all this code? If not, then you need to have the copyright from
the copied code on this file, rather than just saying 2010 Google.

> WebCore/editing/ApplyBlockElementCommand.h:45
> +    virtual void doApply();
> +    virtual void formatSelection(const VisiblePosition& startOfSelection,
const VisiblePosition& endOfSelection);
> +    virtual void formatParagraph(const VisiblePosition&
endOfCurrentParagraph, RefPtr<Element>&) = 0;

You might even consider making some of these private.

> WebCore/editing/ApplyBlockElementCommand.h:52
> +    QualifiedName m_tagName;
> +    AtomicString m_className;
> +    AtomicString m_inlineStyle;

Can these be private instead of protected?

> WebCore/editing/EditorCommand.cpp:442
> +    ExceptionCode ec;
> +    String localName, prefix;
> +    if (!Document::parseQualifiedName(tagName, prefix, localName, ec))
> +	   return false;
> +    QualifiedName qualifiedTagName(prefix, localName, xhtmlNamespaceURI);

Seems to me we need to come up with a parseQualifiedName function that returns
a QualifiedName!