Bug 40378 – [Qt] When any geometry change happens to a node it will resize the backing cache

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit Qt

[Sam Magnuson]

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.

[Sam Magnuson]

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

[Kenneth Rohde Christiansen]

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

[Sam Magnuson]

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

[Jędrzej Nowacki]

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.

[Sam Magnuson]

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

[Sam Magnuson]

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

[Kenneth Rohde Christiansen]

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

[Sam Magnuson]

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

[Sam Magnuson]

(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

[Kenneth Rohde Christiansen]

> 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.

[Kenneth Rohde Christiansen]

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.

[Sam Magnuson]

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

[Noam Rosenthal]

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

[Sam Magnuson]

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

[WebKit Commit Bot]

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

[Sam Magnuson]

Created attachment 59299 [details]
rebase and rediff

[Sam Magnuson]

(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.

[Noam Rosenthal]

Comment on attachment 59299 [details]
rebase and rediff

Clearing the flags, it still doesn't apply cleanly, we're working on it :)

[Eric Seidel (no email)]

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.

[Sam Magnuson]

Created attachment 59433 [details]
Rediff against trunk

[Noam Rosenthal]

Comment on attachment 59433 [details]
Rediff against trunk

I have to take it off the cq because it doesn't compile

[Sam Magnuson]

Created attachment 59483 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 59483 [details]
Patch

Clearing flags on attachment: 59483

Committed r61880: <http://trac.webkit.org/changeset/61880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

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