Bug 55338

Summary: applyInlineStyleToPushDown and removeInlineStyleFromElement should take EditingStyle
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: darin, eric, ojan, tkent, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 55339, 49956, 55349    
Attachments:
Description Flags
fixes the bug
none
cleanup
none
Includes wtf/Forward.h instead of wtf/text/WTFString.h none

Ryosuke Niwa
Reported 2011-02-27 17:41:05 PST
This is a cleanup
Attachments
fixes the bug (33.03 KB, patch)
2011-02-27 19:47 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
cleanup (18.79 KB, patch)
2011-02-27 21:29 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Includes wtf/Forward.h instead of wtf/text/WTFString.h (1.08 KB, patch)
2011-03-01 23:07 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-02-27 19:47:17 PST
Created attachment 84009 [details] fixes the bug
Ryosuke Niwa
Comment 2 2011-02-27 19:49:12 PST
I'll split the patch.
Ryosuke Niwa
Comment 3 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 :(
Ryosuke Niwa
Comment 4 2011-02-27 21:10:14 PST
Comment on attachment 84009 [details] fixes the bug Found a way to split it!
Ryosuke Niwa
Comment 5 2011-02-27 21:29:39 PST
Created attachment 84014 [details] cleanup
Tony Chang
Comment 6 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?
Ryosuke Niwa
Comment 7 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.
Ryosuke Niwa
Comment 8 2011-03-01 00:40:09 PST
Committed r79976: <http://trac.webkit.org/changeset/79976>
Ryosuke Niwa
Comment 9 2011-03-01 00:51:19 PST
Thanks for the review, Tony.
Tony Chang
Comment 10 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?
Darin Adler
Comment 11 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.
Ryosuke Niwa
Comment 12 2011-03-01 23:07:53 PST
Created attachment 84370 [details] Includes wtf/Forward.h instead of wtf/text/WTFString.h
Tony Chang
Comment 13 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.
Ryosuke Niwa
Comment 14 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 :(
Ryosuke Niwa
Comment 15 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>
Ryosuke Niwa
Comment 16 2011-03-03 00:35:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.