Bug 55338 - applyInlineStyleToPushDown and removeInlineStyleFromElement should take EditingStyle
Summary: applyInlineStyleToPushDown and removeInlineStyleFromElement should take Editi...
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: 55339 49956 55349
  Show dependency treegraph
 
Reported: 2011-02-27 17:41 PST by Ryosuke Niwa
Modified: 2011-03-03 00:35 PST (History)
5 users (show)

See Also:


Attachments
fixes the bug (33.03 KB, patch)
2011-02-27 19:47 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (18.79 KB, patch)
2011-02-27 21:29 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Includes wtf/Forward.h instead of wtf/text/WTFString.h (1.08 KB, patch)
2011-03-01 23:07 PST, Ryosuke Niwa
no flags 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-02-27 17:41:05 PST
This is a cleanup
Comment 1 Ryosuke Niwa 2011-02-27 19:47:17 PST
Created attachment 84009 [details]
fixes the bug
Comment 2 Ryosuke Niwa 2011-02-27 19:49:12 PST
I'll split the patch.
Comment 3 Ryosuke Niwa 2011-02-27 19:54:14 PST
(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 :(
Comment 4 Ryosuke Niwa 2011-02-27 21:10:14 PST
Comment on attachment 84009 [details]
fixes the bug

Found a way to split it!
Comment 5 Ryosuke Niwa 2011-02-27 21:29:39 PST
Created attachment 84014 [details]
cleanup
Comment 6 Tony Chang 2011-02-28 11:02:36 PST
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?
Comment 7 Ryosuke Niwa 2011-03-01 00:38:36 PST
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.
Comment 8 Ryosuke Niwa 2011-03-01 00:40:09 PST
Committed r79976: <http://trac.webkit.org/changeset/79976>
Comment 9 Ryosuke Niwa 2011-03-01 00:51:19 PST
Thanks for the review, Tony.
Comment 10 Tony Chang 2011-03-01 10:16:52 PST
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?
Comment 11 Darin Adler 2011-03-01 11:26:56 PST
(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.
Comment 12 Ryosuke Niwa 2011-03-01 23:07:53 PST
Created attachment 84370 [details]
Includes wtf/Forward.h instead of wtf/text/WTFString.h
Comment 13 Tony Chang 2011-03-02 12:01:23 PST
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.
Comment 14 Ryosuke Niwa 2011-03-02 19:30:32 PST
(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 :(
Comment 15 Ryosuke Niwa 2011-03-03 00:35:21 PST
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>
Comment 16 Ryosuke Niwa 2011-03-03 00:35:28 PST
All reviewed patches have been landed.  Closing bug.