Bug 61296 - [WK2] Change TiledDrawingArea to use ShareableBitmap instead of UpdateChunk.
Summary: [WK2] Change TiledDrawingArea to use ShareableBitmap instead of UpdateChunk.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-23 11:58 PDT by Andreas Kling
Modified: 2011-05-24 09:31 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (23.10 KB, patch)
2011-05-23 12:08 PDT, Andreas Kling
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-05-23 11:58:52 PDT
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.
Comment 1 Andreas Kling 2011-05-23 12:08:11 PDT
Created attachment 94452 [details]
Proposed patch
Comment 2 Kenneth Rohde Christiansen 2011-05-23 13:50:05 PDT
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?
Comment 3 Simon Hausmann 2011-05-23 14:20:35 PDT
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?
Comment 4 Andreas Kling 2011-05-24 05:45:43 PDT
(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.
Comment 5 Andreas Kling 2011-05-24 05:47:24 PDT
(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. :)
Comment 6 Simon Hausmann 2011-05-24 09:18:36 PDT
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.
Comment 7 Andreas Kling 2011-05-24 09:31:08 PDT
Committed r87160: <http://trac.webkit.org/changeset/87160>