[webkit-reviews] review denied: [Bug 41737] [Chromium][Win] Crashes with <keygen> with huge padding. : [Attachment 60690] patch v0

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 41737: [Chromium][Win] Crashes with <keygen> with huge padding.
https://bugs.webkit.org/show_bug.cgi?id=41737

Attachment 60690: patch v0
https://bugs.webkit.org/attachment.cgi?id=60690&action=review

------- Additional Comments from David Levin <levin at chromium.org>
WebCore/rendering/RenderThemeChromiumWin.cpp:91
 +	    PassOwnPtr<TransparencyWin> fallback = adoptPtr(new
TransparencyWin());
Style nit: Use OwnPtr. PassOwnPtr should only be used for (return and input)
arguments.

WebCore/rendering/RenderThemeChromiumWin.cpp:89
 +	    // Because TransparencyWin doesn't allocate temporally buffer for
NoLayer,
s/temporally/temporary/

WebCore/ChangeLog:9
 +	    allocate a temporal buffer to composition.	This change add
s/add/adds a/

WebCore/ChangeLog:10
 +	    fallback path to ThemePainter to handle the bufer allocation
s/bufer/buffer/

WebCore/ChangeLog:13
 +	    ThemePainter is no lonnger a subclass of TransparencyWin.  It has
s/lonnger/longer/

WebCore/ChangeLog:9
 +	    allocate a temporal buffer to composition.	This change add
s/to/for/

WebCore/ChangeLog:19
 +	    (WebCore::TransparencyWin::destContext): Added to expose an
internal state.
Exposed state for use in the creation of the fallback window.

WebCore/ChangeLog:22
 +	    (WebCore::TransparencyWin::transformMode): Added to expose an
internal state.
s/Added to expose an internal state./Ditto./


WebCore/ChangeLog:24
 +	    (WebCore::ThemePainter): Added a fallabck path.
s/fallabck/fallback/

WebCore/rendering/RenderThemeChromiumWin.cpp:92
 +	    fallback->init(m_helper.destContext(), TransparencyWin::NoLayer,
m_helper.transformMode(), m_helper.sourceRect());
ASSERT(fallback->context());
?


WebCore/rendering/RenderThemeChromiumWin.cpp:88
 +	    ASSERT(!m_fallbackHelper);
This assert seems odd since this method doesn't even touch m_fallbackHelper.

WebCore/rendering/RenderThemeChromiumWin.cpp:90
 +	    // it can be used for fallback when buffer allocation failed.
Why is it ok to use NoLayer?

WebCore/platform/graphics/chromium/TransparencyWin.h:148
 +	GraphicsContext* destContext() const { return m_destContext; }
Avoid abbreviations: s/dest/destination/ (for the method name).

WebCore/rendering/RenderThemeChromiumWin.cpp:65
 +	    m_helper.init(context, getLayerMode(context, transformMode),
transformMode, r);
Why not just do this?

if (!m_helper.context())
    m_helper(context, TransparencyWin::NoLayer, transformMode, r);

Then, get rid of a lot of stuff like helper(), etc.