Summary: | ContentData equals() methods are not inline-able | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | CSS | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | andersca, ap, benjamin, commit-queue, darin, ddkilzer, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2014-02-10 09:27:27 PST
(In reply to comment #0) > Per Darin Adler in Bug 128510 Comment #2: > > (From update of attachment 223660 [details] [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223660&action=review > > > Source/WebCore/rendering/style/ContentData.h:105 > > +inline bool ImageContentData::equals(const ContentData& data) const > > +{ > > + if (!data.isImage()) > > + return false; > > + return *toImageContentData(data).image() == *image(); > > +} > > I think we could move this to the .cpp file, because I think everyone calls it polymorphically and so nobody really inlines it. > > I also think this could read nicely with && rather than early return. The only place the polymorphism (and the operator==() method) is used is in StyleRareNonInheritedData.cpp: bool StyleRareNonInheritedData::contentDataEquivalent(const StyleRareNonInheritedData& o) const { ContentData* a = m_content.get(); ContentData* b = o.m_content.get(); while (a && b && *a == *b) { // operator==() used here. a = a->next(); b = b->next(); } return !a && !b; } Is it worth leaving ContentData as an abstract base class for this one use? An alternative approach would be to introduce a type enum for each subclass (a la CachedResource), which would let us devirtualize the type methods (isCounter, isImage, isQuote, isText), and implement the equals() methods as inline operator==() overloads instead of as pure virtual methods. This is what the current operator==() method would look like with that change (plus individual operator==() methods for each specific type): inline bool operator==(const ContentData& a, const ContentData& b) { if (a.type() != b.type()) return false; switch (a.type()) { case (ContentData::CounterType): return toCounterContentData(a) == toCounterContentData(b); case (ContentData::ImageType): return toImageContentData(a) == toImageContentData(b); case (ContentData::QuoteType): return toQuoteContentData(a) == toQuoteContentData(b); case (ContentData::TextType): return toTextContentData(a) == toTextContentData(b); } ASSERT_NOT_REACHED(); return false; } Thoughts? Created attachment 223726 [details]
Patch v1
Comment on attachment 223726 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=223726&action=review > Source/WebCore/rendering/style/ContentData.h:68 > + ContentData(Type type) explicit > Source/WebCore/rendering/style/ContentData.h:160 > const CounterContent* counter() const { return m_counter.get(); } Seems like this should return a reference rather than a pointer, unless it can be null. Committed r163936: <http://trac.webkit.org/changeset/163936> (In reply to comment #3) > (From update of attachment 223726 [details]) > > Source/WebCore/rendering/style/ContentData.h:160 > > const CounterContent* counter() const { return m_counter.get(); } > > Seems like this should return a reference rather than a pointer, unless it can be null. Filed Bug 128671. |