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.
Created attachment 69266 [details] Patch
Created attachment 69267 [details] FormatBlockCommand.cpp with patch
Created attachment 69268 [details] IndentOutdentCommand.cpp with patch
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.
Created attachment 69271 [details] FormatBlockCommand.cpp with patch
Created attachment 69272 [details] IndentOutdentCommand.cpp with patch
Sorry, I failed. None of the attached files are what I intended to post here.
Comment on attachment 69266 [details] Patch On second thought, it makes more senes to add an abstract class shared by FormatBlockCommand and IndentOutdentCommand.
Created attachment 69525 [details] adds an abstract class to be inherited by FormatBlockCommand and IndentOutdentCommand
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!
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.
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.
(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.
Committed r68972: <http://trac.webkit.org/changeset/68972>