Bug 61296 – [WK2] Change TiledDrawingArea to use ShareableBitmap instead of UpdateChunk.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andreas Kling]

Most use of UpdateChunk was removed in <http://trac.webkit.org/changeset/86990> and Qt is the only remaining user of it (through TiledDrawingArea.) We should move to using UpdateInfo and ShareableBitmap instead.

[Andreas Kling]

Created attachment 94452 [details]
Proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 94452 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94452&action=review

How are these shared bitmaps being shared, ie what is the actual difference/improvement over what we did before?

> Source/WebKit2/ChangeLog:8
> +        Pass UpdateInfo containing ShareableBitmaps instead of UpdateChunk for tile updates.

So I guess this is the new preferred way?

> Source/WebKit2/ChangeLog:9
> +        Only the bounds rect and bitmap handle in the UpdateInfo is used since none of the

*are* used

> Source/WebKit2/Shared/ShareableBitmap.h:123
> +    QImage createQImage();

createStaticQImage then ?

> Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:234
> +        WebProcess::shared().connection()->deprecatedSend(DrawingAreaProxyLegacyMessage::SnapshotTaken, m_webPage->pageID(), CoreIPC::In(updateInfo));

How is this supposed to be fixed? The SnapshotTaken is Qt specific right?

[Simon Hausmann]

Comment on attachment 94452 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94452&action=review

> Source/WebKit2/Shared/qt/UpdateChunk.cpp:-110
> -    if (QPixmap::defaultDepth() == 16)
> -        bpp = 2;
> -    else
> -        bpp = 4;

This is the only part I'm missing in the new patch. Andreas, do you want to add it in this patch or do a follow-up patch?

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 94452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94452&action=review
> 
> How are these shared bitmaps being shared, ie what is the actual difference/improvement over what we did before?

For the intent and purpose at hand, there is no real difference or improvement beyond a decrease in unshared code. This is essentially about making TiledDrawingArea use the same primitives as DrawingAreaImpl.

> > Source/WebKit2/ChangeLog:8
> > +        Pass UpdateInfo containing ShareableBitmaps instead of UpdateChunk for tile updates.
> 
> So I guess this is the new preferred way?

Yes. Like I said in the description, Qt is the only remaining user of UpdateChunk.

> > Source/WebKit2/ChangeLog:9
> > +        Only the bounds rect and bitmap handle in the UpdateInfo is used since none of the
> 
> *are* used

Oops! Will fix.

> > Source/WebKit2/Shared/ShareableBitmap.h:123
> > +    QImage createQImage();
> 
> createStaticQImage then ?

Why? The QImage created references the underlying raw pixel data (no copy is made), so it's not necessarily static.

> 
> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:234
> > +        WebProcess::shared().connection()->deprecatedSend(DrawingAreaProxyLegacyMessage::SnapshotTaken, m_webPage->pageID(), CoreIPC::In(updateInfo));
> 
> How is this supposed to be fixed? The SnapshotTaken is Qt specific right?

You mean the "deprecatedX" functions? I plan to remove our use of those in a separate patch.

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 94452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94452&action=review
> 
> > Source/WebKit2/Shared/qt/UpdateChunk.cpp:-110
> > -    if (QPixmap::defaultDepth() == 16)
> > -        bpp = 2;
> > -    else
> > -        bpp = 4;
> 
> This is the only part I'm missing in the new patch. Andreas, do you want to add it in this patch or do a follow-up patch?

Good point, Simon. I had originally planned to do it separately by out-of-lining ShareableBitmap::numBytesForSize() etc. But if you prefer, I certainly don't mind doing it in this patch. :)

[Simon Hausmann]

Comment on attachment 94452 [details]
Proposed patch

r=me . Nice cleanup!

I'd say fix the typo before landing that Kenneth pointed out. I'm fine with the pixmap depth change being done in a follow-up patch.

[Andreas Kling]

Committed r87160: <http://trac.webkit.org/changeset/87160>