Bug 40377 - [Qt] When Qt graphicslayer gets a setNeedsDisplay (ie all) it shouldn't record the rect then
Summary: [Qt] When Qt graphicslayer gets a setNeedsDisplay (ie all) it shouldn't recor...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-06-09 11:42 PDT by Sam Magnuson
Modified: 2010-10-06 20:36 PDT (History)
2 users (show)

See Also:


Attachments
Using the updateAll flag that was there all along. (4.95 KB, patch)
2010-06-09 12:35 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Version of the same idea, fixed a few issues. Sam, does this solve the same problem? (3.27 KB, patch)
2010-06-11 13:26 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Proposed patch with proper coding style (6.05 KB, patch)
2010-06-12 10:21 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Same patch - some cleanup (5.87 KB, patch)
2010-06-12 21:27 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (5.57 KB, patch)
2010-06-12 21:31 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Putting up my patch and No'am's changes for review now that I finally tested it (6.80 KB, patch)
2010-06-15 12:53 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Rediff against trunk (6.24 KB, patch)
2010-06-22 16:09 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
...and fix style issue. (6.23 KB, patch)
2010-06-22 16:26 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:42:31 PDT
We should just use the flag that is there to mark the whole area dirty, and then calculate the full rectangle at the point of recache instead.
Comment 1 Sam Magnuson 2010-06-09 12:35:22 PDT
Created attachment 58274 [details]
Using the updateAll flag that was there all along.
Comment 2 Noam Rosenthal 2010-06-11 13:18:07 PDT
Comment on attachment 58274 [details]
Using the updateAll flag that was there all along. 

> @@ -458,10 +458,14 @@ void GraphicsLayerQtImpl::paint(QPainter* painter, const QStyleOptionGraphicsIte
>      switch (m_currentContent.contentType) {
>      case HTMLContentType:
>          if (m_state.drawsContent) {
> -            QPixmap backingStore;
> -            // We might need to recache, in case we try to paint and the cache was purged (e.g. if it was full).
> -            if (!QPixmapCache::find(m_backingStoreKey, &backingStore) || backingStore.size() != m_size.toSize())
> -                backingStore = recache(QRegion(m_state.contentsRect));
> +			QRegion region;
> +			if( m_pendingContent.updateAll )
> +				region |= boundingRect().toRect();
> +			else
> +				region |= m_pendingContent.regionToUpdate;
> +			m_pendingContent.updateAll = false;
> +			m_pendingContent.regionToUpdate = QRegion();
> +            const QPixmap backingStore = recache(region);
>              painter->drawPixmap(0, 0, backingStore);

This part doesn't seem correct - if we got to this point where we paint and there's nothing in the cache, we might need to paint more than the dirty region. This might occur, for example, when the cache was purged because it was full and some other item obscuring this one has moved away. 




TtThi
Comment 3 Noam Rosenthal 2010-06-11 13:26:55 PDT
Created attachment 58502 [details]
Version of the same idea, fixed a few issues. Sam, does this solve the same problem?
Comment 4 Sam Magnuson 2010-06-11 13:50:48 PDT
(In reply to comment #2)
> (From update of attachment 58274 [details])
> > @@ -458,10 +458,14 @@ void GraphicsLayerQtImpl::paint(QPainter* painter, const QStyleOptionGraphicsIte
> >      switch (m_currentContent.contentType) {
> >      case HTMLContentType:
> >          if (m_state.drawsContent) {
> > -            QPixmap backingStore;
> > -            // We might need to recache, in case we try to paint and the cache was purged (e.g. if it was full).
> > -            if (!QPixmapCache::find(m_backingStoreKey, &backingStore) || backingStore.size() != m_size.toSize())
> > -                backingStore = recache(QRegion(m_state.contentsRect));
> > +			QRegion region;
> > +			if( m_pendingContent.updateAll )
> > +				region |= boundingRect().toRect();
> > +			else
> > +				region |= m_pendingContent.regionToUpdate;
> > +			m_pendingContent.updateAll = false;
> > +			m_pendingContent.regionToUpdate = QRegion();
> > +            const QPixmap backingStore = recache(region);
> >              painter->drawPixmap(0, 0, backingStore);
> 
> This part doesn't seem correct - if we got to this point where we paint and there's nothing in the cache, we might need to paint more than the dirty region. This might occur, for example, when the cache was purged because it was full and some other item obscuring this one has moved away. 
> 

I'm not sure I understand. When something is dirty in webkit we'll get the setNeedsDisplay(). Thus the pendingContent updateAll/regionToUpdate knows what is dirty. If graphicsview needs to paint we'll get a paint for ourselves and our children - inside of recache() it will determine if the cache'd pixmap is already gone and if so it will still paint QRegion(0,0,m_size.width(),m_size.height()). Looking it up in ::paint and in ::recache seems unnecessary to me.

> 
> 
> 
> TtThi
Comment 5 Noam Rosenthal 2010-06-11 13:57:19 PDT
Oh, I see what you're saying. My bad. Your patch is right though it should follow the webkit coding style before it can be accepted :)
No'am
Comment 6 Sam Magnuson 2010-06-12 10:21:15 PDT
Created attachment 58556 [details]
Proposed patch with proper coding style
Comment 7 Noam Rosenthal 2010-06-12 21:27:51 PDT
Created attachment 58577 [details]
Same patch - some cleanup

There was some code duplication and style issues that I'm cleaning up with this patch. Sam, if it works for you you can resubmit or I can mark mine for review, whatever works for you.
Comment 8 Noam Rosenthal 2010-06-12 21:31:45 PDT
Created attachment 58578 [details]
Patch

(See above)
Comment 9 Sam Magnuson 2010-06-14 10:15:12 PDT
This patch will do for me, I like that recache() doesn't require the same (semi-redundant) region. However, it seems dubious (to me) that you clear out the regionToUpdate and updateAll members in recache, and thus have to call update first. Maybe just as well to pass a flag to recache that tells it to do the update at the end? 

In any event this looks to achieve the same as my proposed patch, thanks.

//Sam
Comment 10 Sam Magnuson 2010-06-15 12:53:29 PDT
Created attachment 58808 [details]
Putting up my patch and No'am's changes for review now that I finally tested it
Comment 11 Sam Magnuson 2010-06-22 16:09:21 PDT
Created attachment 59432 [details]
Rediff against trunk
Comment 12 WebKit Review Bot 2010-06-22 16:12:06 PDT
Attachment 59432 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:686:  Extra space after ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:686:  Extra space before )  [whitespace/parens] [2]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Sam Magnuson 2010-06-22 16:26:39 PDT
Created attachment 59440 [details]
...and fix style issue.
Comment 14 Kenneth Rohde Christiansen 2010-08-14 07:38:17 PDT
Comment on attachment 59440 [details]
...and fix style issue.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:170
 +      QPixmap recache(bool);
In the WebKit project we are trying to move away from using bools as arguments where it is not obvious what they mean. recache(false) does that mean to not recache? probably not. Use an enum instead.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:348
 +      // Nothing to render
Add dot at end of comment.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:343
 +      m_pendingContent.updateAll = false;
All what? all regions? everything?
Comment 15 Noam Rosenthal 2010-08-14 08:12:33 PDT
We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.