Bug 152816

Created attachment 268429 [details] Patch

Created attachment 268430 [details] Patch

Created attachment 268431 [details] Patch

Created attachment 268432 [details] Patch

Created attachment 268433 [details] Patch

Created attachment 268440 [details] Patch

Created attachment 268442 [details] Patch

Comment on attachment 268433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268433&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:168 > + bool extentKnown() const { return static_cast<bool>(m_extent); } m_extent.isValid()? > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:493 > + Path m_path; It would be great to have them as const Path m_path; > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2016 > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:58 > + if (item.isDrawingItem() || item.type() == ItemType::ApplyStrokePattern || item.type() == ItemType::ApplyStrokePattern) { early return instead? > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2016 > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:30 > +#include "GraphicsContext.h" // For InterpolationQuality. I don't think the comment has much value here. > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2016 > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2016 > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.h:33 > +class AffineTransform; Not needed.

Created attachment 268443 [details] Patch

Comment on attachment 268443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268443&action=review I briefly looked over this patch and noticed some minor nits. > Source/WebCore/ChangeLog:9 > + - DisplayList, a class that holds a vector of display items, and in future will hold metadata Nit: in => "in the" > Source/WebCore/platform/graphics/GraphicsContext.cpp:109 > + GraphicsContextState::StateChangeFlags changeFlags = 0; Would it make sense to initialize this to GraphicsContextState::NoChange? > Source/WebCore/platform/graphics/GraphicsContext.cpp:147 > + // FIXME: this code should move to GraphicsContextState. Nit: The t in "this" should be capitalized so that this comment is a proper sentence. > Source/WebCore/platform/graphics/GraphicsContext.cpp:161 > + // FIXME: deal with state.shadowsUseLegacyRadius) Nit: The d in "deal" should be capitalized so that this comment is a proper sentence. > Source/WebCore/platform/graphics/GraphicsContext.h:225 > +struct GraphicsContextStateChange { I know that you are debating whether to keep this data structure standalone or combine it with GraphicsContextState. Although it is unwritten convention, we tend to reserve the prefix "m_" for private instance variables of a class and omit the prefix "m_" from the name the instance variables in a struct. One such example of this style can be seen in the struct State defined in file Source/WebCore/html/canvas/CanvasRenderingContext2D.h: <http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.h?rev=192140#L268>. Another example of this style can be seen in the struct CompositionEventInit defined in file Source/WebCore/dom/CompositionEvent.h: <http://trac.webkit.org/browser/trunk/Source/WebCore/dom/CompositionEvent.h?rev=194688#L34>. > Source/WebCore/platform/graphics/GraphicsContext.h:241 > + GraphicsContextState::StateChangeFlags m_changeFlags { 0 }; Would it make sense to initialize this to GraphicsContextState::NoChange? > Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:71 > + Nit (OK as-is): I don't feel that this empty line improves the readability of the code. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:36 > +// Should match RenderTheme::platformFocusRingWidth() > +static const float platformFocusRingWidth = 3; I wish we could find a better way to keep this value in sync with RenderTheme. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:164 > + Nit (OK as-is): I don't feel that this empty line improves the readability of this function and its presence makes the formatting for the end of this function asymmetric with the beginning of this function given that there is no empty line under "ts.startGroup();". > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:194 > + Nit (OK as-is): I don't feel that this empty line improves the readability of this function. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:206 > + Ditto. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:218 > + Ditto. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:236 > + Ditto. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:390 > + for (unsigned i = 0; i < m_glyphs.size(); ++i) { Although it is OK to use unsigned as the data type for i given the context of this code, I would suggest using size_t to be consistent with the return value data type of Vector::size(). I'm unclear if the compiler is smart enough to cache m_glyphs.size(). (This would also make the notation of this for-loop consistent with the notation you used in the for-loop on line 413). I suggest that we cache m_glyphs.size() in a local variable and reference this local variable in the for-loop condition instead of evaluating m_glyphs.size() on each iteration. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:403 > + UNUSED_PARAM(m_smoothingMode); I assume that we do this because clang is unhappy that m_smoothingMode is never used in a meaningful way. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:409 > + // the glyph lies entirely within its (ascent+descent x advance) rect. "ascent+descent x advance" => "(ascent + descent) * advance" > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:413 > + for (size_t i = 0; i < m_glyphs.size(); ++i) { I'm unclear if the compiler is smart enough to cache m_glyphs.size(). I suggest that we cache m_glyphs.size() in a local variable and reference this local variable in the for-loop condition instead of evaluating m_glyphs.size() on each iteration. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:679 > +// FIXME: share this code. Nit: The s in "share" should be capitalized so that this comment is a proper sentence. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:680 > +void addConvexPolygonToPath(Path& path, size_t numPoints, const FloatPoint* points) Nit: This patch alternates between numPoints and numberOfPoints (one example is on line 690). I suggest that we use numberOfPoints throughout this patch to conform to the naming conventions in the WebKit Code Style Guideline. > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:45 > + // FIXME: hook up recorder in the GraphicsContext. Nit: The h in "hook" should be capitalized so that this comment is a proper sentence.

👍