Split EditingStyle into several classes |
||||
Issue descriptionEditingStyle is a large class used to deal with CSS styles in editing, but we can split it into several classes: - It creates an EditingStyle for various purposes so there are many methods for creating an EditingStyle that are called in EditingStyle and Command related classes. So, we need to create a Factory class for this. - It has some utility methods that don't use member variables so those methods can be added to EditingUtility.
,
Jan 12 2017
Why do you want to split it? Largeness is not enough to do.
,
Jan 13 2017
Yes, the larger size of the class is not the main reason to split this class. Creating an EditingStyle happens in many places inside EditingStyle and from other classes. For example, the following methods are used to create a EditingStyle in EditingStyle class: EditingStyle* create(); EditingStyle* create(ContainerNode* node, PropertiesToInclude propertiesToInclude = OnlyEditingInheritableProperties); EditingStyle* create(const Position& position, PropertiesToInclude propertiesToInclude = OnlyEditingInheritableProperties); EditingStyle* create(const StylePropertySet* style); EditingStyle* create(CSSPropertyID propertyID, const String& value); EditingStyle* EditingStyle::wrappingStyleForAnnotatedSerialization(ContainerNode* context) EditingStyle* EditingStyle::wrappingStyleForSerialization(ContainerNode* context) EditingStyle* EditingStyle::copy() EditingStyle* EditingStyle::extractAndRemoveBlockProperties() EditingStyle* EditingStyle::extractAndRemoveTextDirection() EditingStyle* EditingStyle::styleAtSelectionStart(const VisibleSelection& selection, bool shouldUseBackgroundColorInEffect, MutableStylePropertySet* styleToCheck) I think method names should be consistent. Each create method name needs to have more meaning because EditingStyle has 5 create methods. So, I'm planning to introduce a factory class to deal with these issues. EditingStyle is used to deal with m_mutableStyle, but some methods don't touch m_mutableStyle such as static WritingDirection EditingStyle::textDirectionForSelection(const VisibleSelection& selection, EditingStyle* typingStyle, bool& hasNestedOrMultipleEmbeddings) static bool EditingStyle::isEmbedOrIsolate(CSSValueID unicodeBidi) static bool EditingStyle::elementIsStyledSpanOrHTMLEquivalent(const HTMLElement* element) They may need to be moved to a utility class in Editing. Some methods create an EditingStyle to only get m_mutableStyle, but there might be a better way to get or access m_mutableStyle. void EditingStyle::prepareToApplyAt(const Position& position, ShouldPreserveWritingDirection shouldPreserveWritingDirection) static bool executeToggleStyle(LocalFrame& frame, EditCommandSource source, InputEvent::InputType inputType, CSSPropertyID propertyID, const char* offValue, const char* onValue) void EditingStyle::mergeInlineAndImplicitStyleOfElement( Element* element, CSSPropertyOverrideMode mode, PropertiesToInclude propertiesToInclude)
,
Jan 13 2017
Could you introduce "EditingStyleUtilities.{cpp,h}"?
"EditingUtilties.{cpp,h}" is also kitchen sink needed to be split. ;-)
,
Jan 13 2017
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f6dfe4978e72853c4debad300c97b4d3872ad89 commit 3f6dfe4978e72853c4debad300c97b4d3872ad89 Author: joone.hur <joone.hur@intel.com> Date: Wed Feb 08 02:31:09 2017 Add EditingStyleUtilities.{cpp,h} EditingStyle is used to deal with CSS styles in editing. but it has many static methods to create an EditingStyle and some of the methods don't access member variables so they can be split into EditingStyleUtilities class. This CL copies EditingStyle.{cpp,h} to EditingStyleUtilities.{cpp,h} as part of refactoring EditingStyle class. BUG= 679817 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2649613002 Cr-Commit-Position: refs/heads/master@{#448866} [add] https://crrev.com/3f6dfe4978e72853c4debad300c97b4d3872ad89/third_party/WebKit/Source/core/editing/EditingStyleUtilities.cpp [add] https://crrev.com/3f6dfe4978e72853c4debad300c97b4d3872ad89/third_party/WebKit/Source/core/editing/EditingStyleUtilities.h
,
Feb 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b68fa9e4f675ec4f158c40297a5638bbb7089f29 commit b68fa9e4f675ec4f158c40297a5638bbb7089f29 Author: joone.hur <joone.hur@intel.com> Date: Thu Feb 09 08:38:32 2017 Add EditingStyleUtitilies.{cpp,h} to BUILD.gn EditingStyle is used to deal with CSS styles in editing. but it has many static methods to create an EditingStyle and some methods don't access member variables so they could be split into EditingStyleUtilities class. We just copied EditingStyle class and renamed to EditingStyleUtilities class in the previous CL(https://crrev.com/2649613002). This CL only keeps the static methods used to create an EditingStyle and some static methods for utility purpose in EditingStyleUtilities class. The next CL will rename the methods and their call sites. BUG= 679817 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2685783004 Cr-Commit-Position: refs/heads/master@{#449238} [modify] https://crrev.com/b68fa9e4f675ec4f158c40297a5638bbb7089f29/third_party/WebKit/Source/core/editing/BUILD.gn [modify] https://crrev.com/b68fa9e4f675ec4f158c40297a5638bbb7089f29/third_party/WebKit/Source/core/editing/EditingStyle.h [modify] https://crrev.com/b68fa9e4f675ec4f158c40297a5638bbb7089f29/third_party/WebKit/Source/core/editing/EditingStyleUtilities.cpp [modify] https://crrev.com/b68fa9e4f675ec4f158c40297a5638bbb7089f29/third_party/WebKit/Source/core/editing/EditingStyleUtilities.h
,
Feb 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08 commit c39bf24ed5228e0da8b7cc1700e1cac6f4997b08 Author: joone.hur <joone.hur@intel.com> Date: Fri Feb 10 05:46:22 2017 Rename the methods in EditingStyleUtilities class and make them used in editing EditingStyle is used to deal with CSS styles in editing. but it has many static methods to create an EditingStyle and some methods don't access member variables so they could be split into EditingStyleUtilities class. In the previous CLs, we just copied EditingStyle class and renamed it to EditingStyleUtilities class(https://crrev.com/2649613002). In addition, we only kept the static methods used to create an EditingStyle and some static methods for utility purpose in EditingStyleUtilities class(https://crrev.com/2685783004). This CL renames the methods in EditingStyleUtilities class and make them used in editing. The next CL will get rid of the static methods moved to EditingStyleUtilities class from EditiingStyle class. BUG= 679817 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2687243002 Cr-Commit-Position: refs/heads/master@{#449554} [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/EditingStyle.cpp [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/EditingStyleUtilities.cpp [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/EditingStyleUtilities.h [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/Editor.cpp [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/commands/ApplyStyleCommand.cpp [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp [modify] https://crrev.com/c39bf24ed5228e0da8b7cc1700e1cac6f4997b08/third_party/WebKit/Source/core/editing/serializers/StyledMarkupSerializer.cpp
,
Feb 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae8ce8f3f082cbb2a2e71f86a78a506cb231c606 commit ae8ce8f3f082cbb2a2e71f86a78a506cb231c606 Author: joone.hur <joone.hur@intel.com> Date: Mon Feb 13 05:52:26 2017 Remove the methods of EditingStyle class moved to EditingStyleUtilities class EditingStyle is used to deal with CSS styles in editing. but it has many static methods to create an EditingStyle and some methods don't access member variables so they could be split into EditingStyleUtilities class. In the previous CLs, we copied EditingStyle class and renamed it to EditingStyleUtilities class(https://crrev.com/2649613002). Then, we only kept the static methods used to create an EditingStyle and some static methods for utility purpose in EditingStyleUtilities class(https://crrev.com/2685783004). In addition, we renamed the methods in EditingStyleUtilities class and make them used in editing(https://crrev.com/2687243002). This CL gets rid of the static methods moved to EditingStyleUtilities class from EditiingStyle class. BUG= 679817 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2692553002 Cr-Commit-Position: refs/heads/master@{#449913} [modify] https://crrev.com/ae8ce8f3f082cbb2a2e71f86a78a506cb231c606/third_party/WebKit/Source/core/editing/EditingStyle.cpp [modify] https://crrev.com/ae8ce8f3f082cbb2a2e71f86a78a506cb231c606/third_party/WebKit/Source/core/editing/EditingStyle.h
,
Mar 1 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by joone....@intel.com
, Jan 10 2017Labels: OS-All