Bug 46840 – FormatBlockCommand and IndentOutdentCommand should use the same code to iterate paragraphs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Currently FormatBlockCommand and IndentOutdentCommand each implements its own paragraph iteration mechanism in FormatBlock::doApply and IndentOutdentCommand::indentRegion.  They should use the same iteration mechanism and in fact share the code.

[Ryosuke Niwa]

Created attachment 69266 [details]
Patch

[Ryosuke Niwa]

Created attachment 69267 [details]
FormatBlockCommand.cpp with patch

[Ryosuke Niwa]

Created attachment 69268 [details]
IndentOutdentCommand.cpp with patch

[Ryosuke Niwa]

Since I'm moving a lot of code, diff & pretty diff are impossible to read.  I'd recommend comparing the attached cpp files with the current cpp files.

[Ryosuke Niwa]

Created attachment 69271 [details]
FormatBlockCommand.cpp with patch

[Ryosuke Niwa]

Created attachment 69272 [details]
IndentOutdentCommand.cpp with patch

[Ryosuke Niwa]

Sorry, I failed.  None of the attached files are what I intended to post here.

[Ryosuke Niwa]

Comment on attachment 69266 [details]
Patch

On second thought, it makes more senes to add an abstract class shared by FormatBlockCommand and IndentOutdentCommand.

[Ryosuke Niwa]

Created attachment 69525 [details]
adds an abstract class to be inherited by FormatBlockCommand and IndentOutdentCommand

[Darin Adler]

Comment on attachment 69525 [details]
adds an abstract class to be inherited by FormatBlockCommand and IndentOutdentCommand

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!

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #10)
> (From update of attachment 69525 [details])
> 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!

I considered that as well but that resulted in unreadable svn diff so decided not to.  Hopefully people read changelog when they're tracking code.

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

Good point.  I intended to add Apple's copyright to cpp file and forgot.  Thanks for pointing that out.

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

I think all of them need to be because they need to be overridden.  But I made splitTextNodes private.

> > WebCore/editing/ApplyBlockElementCommand.h:52
> > +    QualifiedName m_tagName;
> > +    AtomicString m_className;
> > +    AtomicString m_inlineStyle;
> 
> Can these be private instead of protected?

Made everything private and added a protected tagName() accessor.

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

Yes.  That'll be nice.  But the fact there is no null value for QualifiedName prevents us from doing so at the moment.

[Darin Adler]

Comment on attachment 69525 [details]
adds an abstract class to be inherited by FormatBlockCommand and IndentOutdentCommand

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

>>> WebCore/editing/ApplyBlockElementCommand.h:45
>>> +    virtual void formatParagraph(const VisiblePosition& endOfCurrentParagraph, RefPtr<Element>&) = 0;
>> 
>> You might even consider making some of these private.
> 
> I think all of them need to be because they need to be overridden.  But I made splitTextNodes private.

A function does not need to be protected to be overriden. Whether it’s protected or private affects only whether it can be called, no effect on overriding.

[Ryosuke Niwa]

(In reply to comment #12)
> >> You might even consider making some of these private.
> > 
> > I think all of them need to be because they need to be overridden.  But I made splitTextNodes private.
> 
> A function does not need to be protected to be overriden. Whether it’s protected or private affects only whether it can be called, no effect on overriding.

Ah!  I guess I can make doApply and formatParagraph private then.  Will do.

[Ryosuke Niwa]

Committed r68972: <http://trac.webkit.org/changeset/68972>