[webkit-reviews] review denied: [Bug 40378] [Qt] When any geometry change happens to a node it will resize the backing cache : [Attachment 58806] Attached wrong patch in previous modification.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Sam Magnuson
<smagnuson at netflix.com>'s request for review:
Bug 40378: [Qt] When any geometry change happens to a node it will resize the
backing cache
https://bugs.webkit.org/show_bug.cgi?id=40378

Attachment 58806: Attached wrong patch in previous modification.
https://bugs.webkit.org/attachment.cgi?id=58806&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
=WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:44
 +  // #define QT_DEBUG_CACHEDUMP
This alone accounts for an r-, please remove

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:222
 +	    QSizeF	      size;
We do not do indentation like this; just one space

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:316
 +	if ( QPixmapCache::find(m_backingStore.key, &pixmap) )
no space before QPixmap and at the end.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:317
 +	    QPixmapCache::remove(m_backingStore.key); // remove the reference
to the pixmap in the cache to avoid a detach
Comments starts with capital and ends with a punctuation mark (dot) 

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:323
 +	    // If the pixmap is not in the cache or the view has grown since
the last cache
missing dot

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:323
 +	    // If the pixmap is not in the cache or the view has grown since
the last cache
since the last cache? since it was last cached?

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:333
 +		// If the pixmap
I do not understand this comment

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:344
 +		// Blit the contents of oldPixmap back into the cached pixmap
as we are just adding new pixels
misses dot at the end

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:346
 +		    const QRegion cleanRegion = (QRegion(0, 0, m_size.width(),
m_size.height()) &
the & should be at the next line, consult our style guide, please

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:348
 +		    if ( !cleanRegion.isEmpty() ) {
wrong style again

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:363
 +		if ( fill && !region.isEmpty() ) { // Clear the entire pixmap
with the background
again

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:373
 +	    // If we have something to draw its time to erase it and render the
contents
misses dot at the end

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:374
 +	    if ( !region.isEmpty() ) {
wrong style (spaces)

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:387
 +		if ( !erased ) { // Erase the area in cache that we're drawing
into
again

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:395
 +  
unneeded newline

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:405
 +		pixmap.save(QString().sprintf("/tmp/%05d_C.png", recacheCount),
"PNG");
is PNG supposed to be uppercase?

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:409
 +	    m_backingStore.size = m_size; // Store the used size of the pixmap
dot

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:413
 +	m_backingStore.key = QPixmapCache::insert(pixmap);
dot

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:531
 +		const QRectF r(0, 0, m_backingStore.size.width(),
m_backingStore.size.height());
r is not a good variable name