Bug 55902 - selectionHasStyle and selectionStartHasStyle should use EditingStyle
Summary: selectionHasStyle and selectionStartHasStyle should use EditingStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 54932 55904
  Show dependency treegraph
 
Reported: 2011-03-07 13:41 PST by Ryosuke Niwa
Modified: 2011-03-07 19:54 PST (History)
5 users (show)

See Also:


Attachments
cleanup (22.72 KB, patch)
2011-03-07 16:31 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup 2 (22.53 KB, patch)
2011-03-07 16:43 PST, Ryosuke Niwa
darin: 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 2011-03-07 13:41:01 PST
We should deploy EditingStyle in selectionHasStyle and selectionStartHasStyle.
Comment 1 Ryosuke Niwa 2011-03-07 16:31:15 PST
Created attachment 84993 [details]
cleanup
Comment 2 Ryosuke Niwa 2011-03-07 16:43:30 PST
Created attachment 84994 [details]
cleanup 2
Comment 3 Ryosuke Niwa 2011-03-07 16:45:03 PST
Comment on attachment 84993 [details]
cleanup

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

> Source/WebCore/page/ContextMenuController.cpp:41
> +#include "EditingStyle.h"

I got rid of this line but two patches are identical otherwise.
Comment 4 Darin Adler 2011-03-07 18:06:27 PST
Comment on attachment 84994 [details]
cleanup 2

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

> Source/WebCore/editing/EditingStyle.cpp:516
> +// CSS properties that only has a visual difference when applied to text.

Verb agreement problem here. Should be “have” instead of “has”.

But I think the verb “create” is better.

> Source/WebCore/editing/EditingStyle.cpp:527
> +    RefPtr<CSSMutableStyleDeclaration> diff = getPropertiesNotIn(m_mutableStyle.get(), styleToCompare);

Even though diff is a “term of art”, I would use the word “difference” here in this code you moved.
Comment 5 Ryosuke Niwa 2011-03-07 18:46:26 PST
Thanks for the review as always!  Will fix both before landing the patch.

(In reply to comment #4)
> (From update of attachment 84994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84994&action=review
> 
> > Source/WebCore/editing/EditingStyle.cpp:516
> > +// CSS properties that only has a visual difference when applied to text.
> 
> Verb agreement problem here. Should be “have” instead of “has”.
> 
> But I think the verb “create” is better.
> 
> > Source/WebCore/editing/EditingStyle.cpp:527
> > +    RefPtr<CSSMutableStyleDeclaration> diff = getPropertiesNotIn(m_mutableStyle.get(), styleToCompare);
> 
> Even though diff is a “term of art”, I would use the word “difference” here in this code you moved.
Comment 6 Ryosuke Niwa 2011-03-07 19:54:58 PST
Committed r80528: <http://trac.webkit.org/changeset/80528>