Summary: | CounterContentData::counter() and ImageContentData::image() should return references | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | CSS | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | allan.jensen, commit-queue, darin, ddkilzer, dino, esprehn+autocc, glenn, gyuyoung.kim, kling, koivisto, kondapallykalyan, macpherson, menard | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 128915, 129021 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2014-02-12 04:39:34 PST
Created attachment 223975 [details]
Patch v1
Comment on attachment 223975 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=223975&action=review > Source/WebCore/rendering/style/ContentData.h:94 > void setImage(PassRefPtr<StyleImage> image) { m_image = image; } Is this function ever used in a way that makes m_image null? Comment on attachment 223975 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=223975&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1408 > + const CounterContent& counter = toCounterContentData(contentData)->counter(); > + list.get().append(cssValuePool().createValue(counter.identifier(), CSSPrimitiveValue::CSS_COUNTER_NAME)); Why not auto&? Maybe drop the local variable entirely and do this in a single line? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1411 > + const StyleImage& image = toImageContentData(contentData)->image(); > + list.get().append(image.cssValue()); Why not auto&? Maybe drop the local variable entirely and do this in a single line? > Source/WebCore/css/StyleResolver.cpp:3552 > + RefPtr<StyleImage> loadedImage = loadPendingImage(static_cast<StylePendingImage*>(const_cast<StyleImage*>(&image))); This const cast is ugly. Separate point: Can we add checked casts for StyleImage? > Source/WebCore/rendering/RenderElement.cpp:135 > + const StyleImage& styleImage = toImageContentData(contentData)->image(); Why not auto&? > Source/WebCore/rendering/style/ContentData.h:93 > + const StyleImage& image() const { return *m_image; } Why did you get rid of the const overload? >> Source/WebCore/rendering/style/ContentData.h:94 >> void setImage(PassRefPtr<StyleImage> image) { m_image = image; } > > Is this function ever used in a way that makes m_image null? Probably should add an assertion here too, as in the constructor and consider changing to PassRef maybe? > Source/WebCore/rendering/style/ContentData.h:156 > } We should assert m_counter. > Source/WebCore/rendering/style/ContentData.h:159 > void setCounter(std::unique_ptr<CounterContent> counter) { m_counter = std::move(counter); } We should assert counter (before) or m_counter (after). (In reply to comment #3) > (From update of attachment 223975 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223975&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1408 > > + const CounterContent& counter = toCounterContentData(contentData)->counter(); > > + list.get().append(cssValuePool().createValue(counter.identifier(), CSSPrimitiveValue::CSS_COUNTER_NAME)); > > Why not auto&? Ahh, I didn't realize one had to use 'auto&' for a reference. I'll switch them back. > Maybe drop the local variable entirely and do this in a single line? I don't think it improves readability to make this long line even longer. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1411 > > + const StyleImage& image = toImageContentData(contentData)->image(); > > + list.get().append(image.cssValue()); > > Why not auto&? Will fix. > Maybe drop the local variable entirely and do this in a single line? This line still seems readable as one line. > > Source/WebCore/css/StyleResolver.cpp:3552 > > + RefPtr<StyleImage> loadedImage = loadPendingImage(static_cast<StylePendingImage*>(const_cast<StyleImage*>(&image))); > > This const cast is ugly. It gets cleaned up in a subsequent patch that makes loadPendingImage() take a reference. > Separate point: Can we add checked casts for StyleImage? I have a follow-up patch to add the checked casts for StyleImage, but it currently depends on this patch. > > Source/WebCore/rendering/RenderElement.cpp:135 > > + const StyleImage& styleImage = toImageContentData(contentData)->image(); > > Why not auto&? Will fix. > > Source/WebCore/rendering/style/ContentData.h:93 > > + const StyleImage& image() const { return *m_image; } > > Why did you get rid of the const overload? It didn't really add anything. (As noted above, the const cast goes away in a future patch.) > >> Source/WebCore/rendering/style/ContentData.h:94 > >> void setImage(PassRefPtr<StyleImage> image) { m_image = image; } > > > > Is this function ever used in a way that makes m_image null? > > Probably should add an assertion here too, as in the constructor and consider changing to PassRef maybe? Good point. Will add. > > Source/WebCore/rendering/style/ContentData.h:156 > > } > > We should assert m_counter. Will add. > > Source/WebCore/rendering/style/ContentData.h:159 > > void setCounter(std::unique_ptr<CounterContent> counter) { m_counter = std::move(counter); } > > We should assert counter (before) or m_counter (after). Will add. Committed r164224: <http://trac.webkit.org/changeset/164224> (In reply to comment #4) > (In reply to comment #3) > > Separate point: Can we add checked casts for StyleImage? > > I have a follow-up patch to add the checked casts for StyleImage, but it currently depends on this patch. Bug 128915 (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 223975 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=223975&action=review > > > > > Source/WebCore/css/StyleResolver.cpp:3552 > > > + RefPtr<StyleImage> loadedImage = loadPendingImage(static_cast<StylePendingImage*>(const_cast<StyleImage*>(&image))); > > > > This const cast is ugly. > > It gets cleaned up in a subsequent patch that makes loadPendingImage() take a reference. Bug 129021 |