Bug 55338 – applyInlineStyleToPushDown and removeInlineStyleFromElement should take EditingStyle

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is a cleanup

[Ryosuke Niwa]

Created attachment 84009 [details]
fixes the bug

[Ryosuke Niwa]

I'll split the patch.

[Ryosuke Niwa]

(In reply to comment #2)
> I'll split the patch.

Oops, I take it back.  Fixing these bugs require that we share the code I'm moving in this patch.   On the other hand, sharing code without fixing the bugs is impossible.  So this patch can't be split into smaller pieces :(

[Ryosuke Niwa]

Comment on attachment 84009 [details]
fixes the bug

Found a way to split it!

[Ryosuke Niwa]

Created attachment 84014 [details]
cleanup

[Tony Chang]

Comment on attachment 84014 [details]
cleanup

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:1229
> -    
> +       

Nit: Did you mean to add spaces here?

> Source/WebCore/editing/EditingStyle.cpp:327
> +    ExceptionCode ec;
> +    m_mutableStyle->setProperty(propertyID, value, important, ec);

Do we need to check or assert the exception code here?

> Source/WebCore/editing/EditingStyle.h:39
> +#include <wtf/text/WTFString.h>

Why is this needed?

[Ryosuke Niwa]

Comment on attachment 84014 [details]
cleanup

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

>> Source/WebCore/editing/ApplyStyleCommand.cpp:1229
>> +       
> 
> Nit: Did you mean to add spaces here?

Oops, I meant to remove the spaces.

>> Source/WebCore/editing/EditingStyle.cpp:327
>> +    m_mutableStyle->setProperty(propertyID, value, important, ec);
> 
> Do we need to check or assert the exception code here?

I don't think we need to given:

void CSSMutableStyleDeclaration::setProperty(int propertyID, const String& value, bool important, ExceptionCode& ec)
{
    ec = 0;
    setProperty(propertyID, value, important, true);
}

>> Source/WebCore/editing/EditingStyle.h:39
>> +#include <wtf/text/WTFString.h>
> 
> Why is this needed?

Because setProperty takes a String reference.

[Ryosuke Niwa]

Committed r79976: <http://trac.webkit.org/changeset/79976>

[Ryosuke Niwa]

Thanks for the review, Tony.

[Tony Chang]

Comment on attachment 84014 [details]
cleanup

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

>>> Source/WebCore/editing/EditingStyle.h:39
>>> +#include <wtf/text/WTFString.h>
>> 
>> Why is this needed?
> 
> Because setProperty takes a String reference.

Can we forward declare String instead?

[Darin Adler]

(In reply to comment #10)
> >>> Source/WebCore/editing/EditingStyle.h:39
> >>> +#include <wtf/text/WTFString.h>
> >> 
> >> Why is this needed?
> > 
> > Because setProperty takes a String reference.
> 
> Can we forward declare String instead?

No, but we can include <wtf/Forward.h>, which is the right thing to do here.

[Ryosuke Niwa]

Created attachment 84370 [details]
Includes wtf/Forward.h instead of wtf/text/WTFString.h

[Tony Chang]

Comment on attachment 84370 [details]
Includes wtf/Forward.h instead of wtf/text/WTFString.h

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

> Source/WebCore/editing/EditingStyle.h:39
>  #include <wtf/Vector.h>

It looks like you can remove this as well since it's in Forward.h and only used as pointers and references.

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 84370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84370&action=review
> 
> > Source/WebCore/editing/EditingStyle.h:39
> >  #include <wtf/Vector.h>
> 
> It looks like you can remove this as well since it's in Forward.h and only used as pointers and references.

It seems like I can't do that because Forward.h only contains the version that have the inline capacity :(

[Ryosuke Niwa]

Comment on attachment 84370 [details]
Includes wtf/Forward.h instead of wtf/text/WTFString.h

Clearing flags on attachment: 84370

Committed r80213: <http://trac.webkit.org/changeset/80213>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.