Bug 40378 - [Qt] When any geometry change happens to a node it will resize the backing cache
Summary: [Qt] When any geometry change happens to a node it will resize the backing cache
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-06-09 11:44 PDT by Sam Magnuson
Modified: 2010-07-01 01:25 PDT (History)
6 users (show)

See Also:


Attachments
Continue to grow the cache, but never toss it. (8.62 KB, patch)
2010-06-09 12:36 PDT, Sam Magnuson
kenneth: review-
Details | Formatted Diff | Diff
Proposed patch with proper coding style (8.76 KB, patch)
2010-06-12 10:22 PDT, Sam Magnuson
jedrzej.nowacki: review-
jedrzej.nowacki: commit-queue-
Details | Formatted Diff | Diff
Proposed patch with proper coding style, I had reapplied this after a pull from origin this week but never reposted (9.43 KB, patch)
2010-06-15 12:26 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Attached wrong patch in previous modification. (9.45 KB, patch)
2010-06-15 12:39 PDT, Sam Magnuson
kenneth: review-
Details | Formatted Diff | Diff
Take 3, same patch with cleanups proposed in last review. (9.34 KB, patch)
2010-06-15 14:14 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
define to 0 as requested rather than just removing the define. (9.51 KB, patch)
2010-06-16 09:35 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
define to 0 as requested rather than just removing the define. (9.51 KB, patch)
2010-06-16 09:36 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
rebase and rediff (9.65 KB, patch)
2010-06-21 15:21 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Rediff against trunk (9.13 KB, patch)
2010-06-22 16:09 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Patch (8.81 KB, patch)
2010-06-23 00:03 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Magnuson 2010-06-09 11:44:15 PDT
We should attempt to use the same cache if possible since the whole area doesn't necessarily dirty and allocating a new backing surface may a bit costly.
Comment 1 Sam Magnuson 2010-06-09 12:36:52 PDT
Created attachment 58275 [details]
Continue to grow the cache, but never toss it.
Comment 2 Kenneth Rohde Christiansen 2010-06-11 18:48:54 PDT
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
Comment 3 Sam Magnuson 2010-06-12 10:22:16 PDT
Created attachment 58557 [details]
Proposed patch with proper coding style
Comment 4 Jędrzej Nowacki 2010-06-15 01:25:34 PDT
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.
Comment 5 Sam Magnuson 2010-06-15 12:26:51 PDT
Created attachment 58805 [details]
Proposed patch with proper coding style, I had reapplied this after a pull from origin this week but never reposted
Comment 6 Sam Magnuson 2010-06-15 12:39:59 PDT
Created attachment 58806 [details]
Attached wrong patch in previous modification.
Comment 7 Kenneth Rohde Christiansen 2010-06-15 13:35:40 PDT
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
Comment 8 Sam Magnuson 2010-06-15 14:14:29 PDT
Created attachment 58817 [details]
Take 3, same patch with cleanups proposed in last review.
Comment 9 Sam Magnuson 2010-06-15 14:17:37 PDT
(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
Comment 10 Kenneth Rohde Christiansen 2010-06-16 05:26:44 PDT
> 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 11 Kenneth Rohde Christiansen 2010-06-16 05:28:16 PDT
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.
Comment 12 Sam Magnuson 2010-06-16 09:35:33 PDT
Created attachment 58899 [details]
define to 0 as requested rather than just removing the define.
Comment 13 Noam Rosenthal 2010-06-16 09:36:11 PDT
LGTM, as are all of Sam's patches to AC :)
Comment 14 Sam Magnuson 2010-06-16 09:36:36 PDT
Created attachment 58900 [details]
define to 0 as requested rather than just removing the define.
Comment 15 WebKit Commit Bot 2010-06-19 04:44:44 PDT
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
Comment 16 Sam Magnuson 2010-06-21 15:21:32 PDT
Created attachment 59299 [details]
rebase and rediff
Comment 17 Sam Magnuson 2010-06-21 15:23:03 PDT
(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 18 Noam Rosenthal 2010-06-21 16:17:33 PDT
Comment on attachment 59299 [details]
rebase and rediff

Clearing the flags, it still doesn't apply cleanly, we're working on it :)
Comment 19 Eric Seidel (no email) 2010-06-22 13:23:48 PDT
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.
Comment 20 Sam Magnuson 2010-06-22 16:09:58 PDT
Created attachment 59433 [details]
Rediff against trunk
Comment 21 Noam Rosenthal 2010-06-22 16:50:00 PDT
Comment on attachment 59433 [details]
Rediff against trunk

I have to take it off the cq because it doesn't compile
Comment 22 Sam Magnuson 2010-06-23 00:03:26 PDT
Created attachment 59483 [details]
Patch
Comment 23 WebKit Commit Bot 2010-06-25 12:31:22 PDT
Comment on attachment 59483 [details]
Patch

Clearing flags on attachment: 59483

Committed r61880: <http://trac.webkit.org/changeset/61880>
Comment 24 WebKit Commit Bot 2010-06-25 12:31:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Simon Hausmann 2010-07-01 01:24:15 PDT
Revision r61880 cherry-picked into qtwebkit-2.0 with commit 110642d6f33af6437d305058b73dbd9f03fc1c92