[Webkit-unassigned] [Bug 46335] Add EditingStyle

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46335





--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org>  2010-09-30 13:00:55 PST ---
(From update of attachment 69363)
View in context: https://bugs.webkit.org/attachment.cgi?id=69363&action=review

>> WebCore/editing/CompositeEditCommand.cpp:935
>> +        styleInEmptyParagraph = editingStyleIncludingTypingStyle(startOfParagraphToMove.deepEquivalent());
> 
> Is there an editing style which does not include typing style?

Yes.  See where we call EditingStyle::create.  As I mentioned yesterday, it might make sense to make editingStyle() for those places as supposed to be calling EditingStyle::create.

>> WebCore/editing/CompositeEditCommand.cpp:1052
>> +        applyStyle(style->style());
> 
> Does applyStyle ASSERT ! empty?  Or should this check just go into applyStyle?

I think this check is an optimization because we do check this condition in ApplyStyleCommand::doApply.  See http://trac.webkit.org/browser/trunk/WebCore/editing/ApplyStyleCommand.cpp#L599.  i.e. applyStyle shouldn't be doing anything when style->isEmpty() is true.

>> WebCore/editing/DeleteSelectionCommand.cpp:297
>> +    m_typingStyle = EditingStyle::create(positionBeforeTabSpan(m_selectionToDelete.start()));
> 
> I take it this is an editing style which does not include the typing style?

Yes.

>> WebCore/editing/EditingStyle.cpp:51
>> +: m_shouldUseFixedDefaultFontSize(false)
> 
> indent.

Will fix.

>> WebCore/editing/EditingStyle.cpp:56
>> +: m_shouldUseFixedDefaultFontSize(false)
> 
> again.

Will fix.

>> WebCore/editing/EditingStyle.cpp:131
>> +    m_mutableStyle->removeBlockProperties();
> 
> I would have probably just reversed this if since it's one line.

Will fix.

>> WebCore/editing/EditingStyle.cpp:146
>> +        Color color = Color(primitiveValue->getRGBA32Value());
> 
> This is the way we build colors from CSSPrimativeValues?  I would have expected there to be a function or constructor which did this.

I should have done:
if (!alphaChannel(primitiveValue->getRGBA32Value()))
  m_mutableStyle->removeProperty(CSSPropertyBackgroundColor, ec);

Will fix.

>> WebCore/editing/EditingStyle.h:78
>> +    void init(Node*);
> 
> What is the purpose of a separate init() call?  Generally we don't have separate init() in webkit.

Constructors that take Node and Position need to do the same initialization.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.