Bug 40378

Created attachment 58275 [details] Continue to grow the cache, but never toss it.

Comment on attachment 58275 [details] Continue to grow the cache, but never toss it. Can you please run check-webkit-style (in WebKitTools/Scripts) and fix the style issues before putting this up for review again? Please read the style guide on webkit.org

Created attachment 58557 [details] Proposed patch with proper coding style

Comment on attachment 58557 [details] Proposed patch with proper coding style Patch doesn't apply (cq-) and it contains a coding style issue (r-).Please look at comment in 40376 (https://bugs.webkit.org/show_bug.cgi?id=40376#c3) similar problem applies.

Created attachment 58805 [details] Proposed patch with proper coding style, I had reapplied this after a pull from origin this week but never reposted

Created attachment 58806 [details] Attached wrong patch in previous modification.

Comment on attachment 58806 [details] Attached wrong patch in previous modification. =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

Created attachment 58817 [details] Take 3, same patch with cleanups proposed in last review.

(In reply to comment #7) > (From update of attachment 58806 [details]) > =WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:44 > + // #define QT_DEBUG_CACHEDUMP > This alone accounts for an r-, please remove > It seems convenient to document the define somewhere for turning on debugging - where should that go instead? > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:222 > + QSizeF size; > We do not do indentation like this; just one space > Ok. > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:316 > + if ( QPixmapCache::find(m_backingStore.key, &pixmap) ) > no space before QPixmap and at the end. > I thought I've had such things flagged by check-webkit-style before - but these seemed to slip by... > 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 > Fixed these. > 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 > I don't see that documented anywhere, or flagged by check-webkit-style. Where should I be looking for these details? > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:405 > + pixmap.save(QString().sprintf("/tmp/%05d_C.png", recacheCount), "PNG"); > is PNG supposed to be uppercase? > Yes, I believe so: http://doc.qt.nokia.com/4.6/qimage.html#reading-and-writing-image-files //Sam

> It seems convenient to document the define somewhere for turning on debugging - where should that go instead? define it as 0 instead of commenting it out would probably be ok > > 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 > > > > I don't see that documented anywhere, or flagged by check-webkit-style. Where should I be looking for these details? should follow the same rule as &&, || etc. When the line is broken they go to the front of the next line, not to the end of the previous one.

Comment on attachment 58817 [details] Take 3, same patch with cleanups proposed in last review. No'am can you have a quick look at this patch to see if you find anything out of order? Looks good to me.

Created attachment 58899 [details] define to 0 as requested rather than just removing the define.

LGTM, as are all of Sam's patches to AC :)

Created attachment 58900 [details] define to 0 as requested rather than just removing the define.

Comment on attachment 58900 [details] define to 0 as requested rather than just removing the define. Rejecting patch 58900 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1 Parsed 2 diffs from patch file(s). patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp Hunk #2 succeeded at 229 (offset 11 lines). Hunk #3 FAILED at 321. Hunk #4 succeeded at 567 (offset 40 lines). Hunk #5 succeeded at 753 (offset 60 lines). Hunk #6 FAILED at 796. Hunk #7 FAILED at 1070. 3 out of 7 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp.rej Full output: http://webkit-commit-queue.appspot.com/results/3337357

Created attachment 59299 [details] rebase and rediff

(In reply to comment #15) > (From update of attachment 58900 [details]) > Rejecting patch 58900 from commit-queue. > Weird, I did a rebase to origin and resubmitted the patch.

Comment on attachment 59299 [details] rebase and rediff Clearing the flags, it still doesn't apply cleanly, we're working on it :)

Comment on attachment 58900 [details] define to 0 as requested rather than just removing the define. Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 58900 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 59433 [details] Rediff against trunk

Comment on attachment 59433 [details] Rediff against trunk I have to take it off the cq because it doesn't compile

Created attachment 59483 [details] Patch

Comment on attachment 59483 [details] Patch Clearing flags on attachment: 59483 Committed r61880: <http://trac.webkit.org/changeset/61880>

All reviewed patches have been landed. Closing bug.

Revision r61880 cherry-picked into qtwebkit-2.0 with commit 110642d6f33af6437d305058b73dbd9f03fc1c92