Bug 40377 – [Qt] When Qt graphicslayer gets a setNeedsDisplay (ie all) it shouldn't record the rect then

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Qt

[Sam Magnuson]

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.

[Sam Magnuson]

Created attachment 58274 [details]
Using the updateAll flag that was there all along.

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 58502 [details]
Version of the same idea, fixed a few issues. Sam, does this solve the same problem?

[Sam Magnuson]

(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

[Noam Rosenthal]

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

[Sam Magnuson]

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

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 58578 [details]
Patch

(See above)

[Sam Magnuson]

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

[Sam Magnuson]

Created attachment 58808 [details]
Putting up my patch and No'am's changes for review now that I finally tested it

[Sam Magnuson]

Created attachment 59432 [details]
Rediff against trunk

[WebKit Review Bot]

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.

[Sam Magnuson]

Created attachment 59440 [details]
...and fix style issue.

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.