Bug 27325 - copyInheritableProperties and removeComputedInheritablePropertiesFrom should be deprecated
Summary: copyInheritableProperties and removeComputedInheritablePropertiesFrom should ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 20348
  Show dependency treegraph
 
Reported: 2009-07-15 17:46 PDT by Ryosuke Niwa
Modified: 2009-07-24 16:16 PDT (History)
4 users (show)

See Also:


Attachments
removes removeComputedInheritablePropertiesFrom and renames copyInheritableProperties (13.56 KB, patch)
2009-07-15 18:26 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Deprecates copyInheritableProperties and removes removeComputedInheritablePropertiesFrom (14.13 KB, patch)
2009-07-15 18:50 PDT, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2009-07-15 17:46:22 PDT
removeComputedInheritablePropertiesFrom is not used anywhere and copyInheritableProperties is used across the editing code but very inconsistently.  copyInheritableProperties is poorly named considering the usage of the function.  copyInheritableProperties should be deprecated and callers should use a better named function.

Below are just examples of how copyInheritableProperties is used:

553@ReplaceSelectionCommand.cpp
    RefPtr<CSSMutableStyleDeclaration> styleAtInsertionPos = rangeCompliantEquivalent(insertionPos).computedStyle()->copyInheritableProperties();
    String styleText = styleAtInsertionPos->cssText();
    
    if (styleText == static_cast<Element*>(sourceDocumentStyleSpan)->getAttribute(styleAttr)) {

Here, it's used to compare the styles of the source and destination.


55@RemoveFormatCommand.cpp
    // Get the default style for this editable root, it's the style that we'll give the
    // content that we're operating on.
    Node* root = frame->selection()->rootEditableElement();
    RefPtr<CSSMutableStyleDeclaration> defaultStyle = computedStyle(root)->copyInheritableProperties();

Here, it's used to copy styles.


281@DeleteSelectionCommand.cpp
    // Figure out the typing style in effect before the delete is done.
    RefPtr<CSSComputedStyleDeclaration> computedStyle = positionBeforeTabSpan(m_selectionToDelete.start()).computedStyle();
    m_typingStyle = computedStyle->copyInheritableProperties();

And here, it's used to retrieve the typing style.
Comment 1 Ryosuke Niwa 2009-07-15 17:55:57 PDT
Eric and I figured out that the last time any significant change was made to this function was http://trac.webkit.org/changeset/7314.
Comment 2 Ryosuke Niwa 2009-07-15 18:00:42 PDT
It seems to me that while there are several usages of this function, we can categorize it into two:

1. Preserving styles - e.g. you want to preserve styles while editing

Because we almost always remove redundant styles when reapplying the style, we should make a function that takes the destination position, and applies the minimum styles automatically.

2. Typing styles - we set typingStyle

A lot of problems are caused by typing styles (e.g. highlight is lost in various cases).  It may make sense to make a new class, say, TypyingStyle class that centralizes code related to typing style.

Any alternative ideas / suggestions?
Comment 3 Ryosuke Niwa 2009-07-15 18:26:36 PDT
Created attachment 32825 [details]
removes removeComputedInheritablePropertiesFrom and renames copyInheritableProperties
Comment 4 Ryosuke Niwa 2009-07-15 18:50:59 PDT
Created attachment 32826 [details]
Deprecates copyInheritableProperties and removes removeComputedInheritablePropertiesFrom

Added more detailed comments
Comment 5 Justin Garcia 2009-07-16 12:59:17 PDT
This seems reasonable except that preserving styles will still rely on the concept of inheritable properties.  Will it just not use this method that you're deprecating?
Comment 6 Ryosuke Niwa 2009-07-20 16:18:59 PDT
(In reply to comment #5)
> This seems reasonable except that preserving styles will still rely on the
> concept of inheritable properties.  Will it just not use this method that
> you're deprecating?

I think when we preserve style during editing operations, we also want to preserve background color and text decorations, which are not inheritable properties of CSS.  So the name is still confusing.
Comment 7 Eric Seidel (no email) 2009-07-22 13:09:06 PDT
Comment on attachment 32826 [details]
Deprecates copyInheritableProperties and removes removeComputedInheritablePropertiesFrom

Seems fine.
Comment 8 Ryosuke Niwa 2009-07-24 16:16:46 PDT
Landed in http://trac.webkit.org/changeset/46280