Summary: | applyInlineStyleToPushDown and removeInlineStyleFromElement should take EditingStyle | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | HTML Editing | Assignee: | 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
Ryosuke Niwa
2011-02-27 17:41:05 PST
Created attachment 84009 [details]
fixes the bug
I'll split the patch. (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 on attachment 84009 [details]
fixes the bug
Found a way to split it!
Created attachment 84014 [details]
cleanup
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 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. Committed r79976: <http://trac.webkit.org/changeset/79976> Thanks for the review, Tony. 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? (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. Created attachment 84370 [details]
Includes wtf/Forward.h instead of wtf/text/WTFString.h
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. (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 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> All reviewed patches have been landed. Closing bug. |