Summary: | Clipping in accelerated Canvas2D should be faster | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||
Component: | Canvas | Assignee: | Stephen White <senorblanco> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | jamesr, kbr, mdelaney7 | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Stephen White
2011-03-30 09:20:33 PDT
Created attachment 87552 [details]
Patch
Comment on attachment 87552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87552&action=review The idea is good but the implementation needs cleanup. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:72 > + Path m_path; > + AffineTransform m_transform; In WebKit style the m_ prefix isn't needed for members of structs. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:81 > + , m_numClippingPaths(0) Please add a method to this struct like "bool clippingEnabled()" which does the test of m_numClippingPaths > 0. The tests strewn throughout this patch are gross and will be fragile in the long run. Even better, make the method call m_clippingPaths.size() and remove the duplicated m_numClippingPaths member entirely. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:221 > + if (m_state->m_ctm.isIdentity() && !m_state->m_numClippingPaths) { Replace the test of m_numClippingPaths with a method call. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254 > + applyClipping(m_state->m_numClippingPaths > 0); Here and below, please replace the test of "m_state->m_numClippingPaths > 0" with a method call. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:355 > + m_state->m_clippingPaths.append(PathAndTransform(path, m_state->m_ctm)); > + m_state->m_numClippingPaths++; This looks like replicated data. m_numClippingPaths == m_clippingPaths.size(). Comment on attachment 87552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87552&action=review Thanks for the review. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:72 >> + AffineTransform m_transform; > > In WebKit style the m_ prefix isn't needed for members of structs. Fixed. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:81 >> + , m_numClippingPaths(0) > > Please add a method to this struct like "bool clippingEnabled()" which does the test of m_numClippingPaths > 0. The tests strewn throughout this patch are gross and will be fragile in the long run. Even better, make the method call m_clippingPaths.size() and remove the duplicated m_numClippingPaths member entirely. No, that won't work. m_numClippingPaths is the total number of paths, in all States. m_clippingPaths contains only the paths in the current State (see the comment in the GLES2Canvas copy constructor). >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:221 >> + if (m_state->m_ctm.isIdentity() && !m_state->m_numClippingPaths) { > > Replace the test of m_numClippingPaths with a method call. Done. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254 >> + applyClipping(m_state->m_numClippingPaths > 0); > > Here and below, please replace the test of "m_state->m_numClippingPaths > 0" with a method call. Done. >> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:355 >> + m_state->m_numClippingPaths++; > > This looks like replicated data. m_numClippingPaths == m_clippingPaths.size(). Nope (see above). Created attachment 87620 [details]
Patch
Comment on attachment 87620 [details]
Patch
Thanks for doing the cleanups. I understand now the difference between m_clippingPaths' length and m_numClippingPaths. r=me
Committed r82502: <http://trac.webkit.org/changeset/82502> |