Summary: | [Qt] When any geometry change happens to a node it will resize the backing cache | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Magnuson <smagnuso> | ||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | CLOSED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hausmann, jedrzej.nowacki, kenneth, laszlo.gombos, noam | ||||||||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 38744 | ||||||||||||||||||||||||
Attachments: |
|
Description
Sam Magnuson
2010-06-09 11:44:15 PDT
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 |