Bug 49526 – [WK2][Qt] WebKit2 implementation of tiled backing store

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Andreas Kling]

Patch coming.

[Andreas Kling]

Created attachment 73877 [details]
Proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 73877 [details]
Proposed patch

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

First round of comments.

> WebKit2/Shared/CoreIPCSupport/DrawingAreaMessageKinds.h:50
> +    // Called to request a tile update

Add a dot at the end

> WebKit2/Shared/CoreIPCSupport/DrawingAreaMessageKinds.h:53
> +    // Called to cancel a requested tile update

Here as well

> WebKit2/Shared/CoreIPCSupport/DrawingAreaMessageKinds.h:54
> +    CancelTileUpdate,

You have UpdateTile and CancelTileUpdate? Maybe the former should be called RequestTileUpdate?

> WebKit2/Shared/CoreIPCSupport/DrawingAreaProxyMessageKinds.h:44
> +    TileUpdatesComplete,

Maybe a better name can be found, as it is not clear what it complete. a current round of tile updates?

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:52
> +    void _q_scaleChanged();
> +    void commitScale();

Why is one with _q_? and the other not? Should both be, or none be?

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:74
> +    PassOwnPtr<DrawingAreaProxy> drawingAreaProxy;
> +#if ENABLE(TILED_BACKING_STORE)
> +    if (backingStoreType == Tiled) {
> +        drawingAreaProxy = TiledDrawingAreaProxy::create(this);
> +        connect(this, SIGNAL(scaleChanged()), this, SLOT(_q_scaleChanged()));
> +    } else
> +#endif
> +        drawingAreaProxy = ChunkedUpdateDrawingAreaProxy::create(this);

switch instead?

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:78
> +    d->page->setViewportSize(geometry().size().toSize());

does this make sense? What about setResizesToConten....

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:314
> +void QGraphicsWKView::commitScaleChange()

You have begin and commit? Submit, commit? or begin, end?

> WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:341
> +        // For now we block until complete

dot at the end

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:57
> +    , m_tileCreationDelay(0.01)
> +    , m_keepAreaMultiplier(2.5f, 4.5f)

Why the f some places but not all?

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:84
> +    page->process()->send(DrawingAreaMessage::SetSize, page->pageID(), CoreIPC::In(viewSize));

This is using old API, you should be able to call sent(DrawingAreaMessage::SetSize(viewSize) I beleive

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:105
> +    page->process()->send(DrawingAreaMessage::ResumePainting, page->pageID(), CoreIPC::In());

Here as well

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:125
> +void TiledDrawingAreaProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID messageID, CoreIPC::ArgumentDecoder* arguments)
> +{
> +    switch (messageID.get<DrawingAreaProxyMessage::Kind>()) {
> +    case DrawingAreaProxyMessage::TileUpdated: {

Can you verify it this is the way it is still done?

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:172
> +    page()->process()->connection()->send(DrawingAreaMessage::UpdateTile, page()->pageID(), CoreIPC::In(tileID, dirtyRect, contentsScale()));

new api

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:182
> +        static const double ipcTimeout = 2.0;

Is ipcTimeout the best name?

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:349
> +    IntRect visibleRect = mapFromContents(webViewVisibleRect());

I think we need to hook this up to visibleContentRect from webcore.

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:383
> +    // Manhattan distance, biased so that vertical distances are shorter.

Would be nice with a why

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:458
> +    // Now construct the tile(s)

dot at end

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:465
> +    // Paint the content of the newly created tiles

Dot at end

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:477
> +    bool resized = false;

wasResized?

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:535
> +    page->process()->send(DrawingAreaMessage::CancelTileUpdate, page->pageID(), CoreIPC::In(tile->ID()));

new API?

> WebKit2/UIProcess/TiledDrawingAreaProxy.h:92
> +    void getKeepAndCoverAreaMultipliers(WebCore::FloatSize& keepMultiplier, WebCore::FloatSize& coverMultiplier)

Remove get?

> WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:94
> +    // Layout if necessary.
> +    m_webPage->layoutIfNeeded();

that comment is quite useless :-)

> WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:124
> +    WebProcess::shared().connection()->send(DrawingAreaProxyMessage::DidSetSize, m_webPage->pageID(), CoreIPC::In(viewSize));

new API?

[Andreas Kling]

(In reply to comment #2)
> You have UpdateTile and CancelTileUpdate? Maybe the former should be called RequestTileUpdate?

Okydoky.

> > WebKit2/Shared/CoreIPCSupport/DrawingAreaProxyMessageKinds.h:44
> > +    TileUpdatesComplete,
> 
> Maybe a better name can be found, as it is not clear what it complete. a current round of tile updates?

Indeed. It fires when there are no more tile update requests pending.
Any ideas for something better? RequestedTilesUpdated?

> > WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:52
> > +    void _q_scaleChanged();
> > +    void commitScale();
> 
> Why is one with _q_? and the other not? Should both be, or none be?

The _q_ is Qt convention for handling the public class's signal in the private class. We could call it e.g. onScaleChanged() instead, I don't really mind.

> > WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:78
> > +    d->page->setViewportSize(geometry().size().toSize());
> 
> does this make sense? What about setResizesToConten....

Hmhmhm.. you want to hardcode setRTC in tiling mode?

> > WebKit2/UIProcess/API/qt/qgraphicswkview.cpp:314
> > +void QGraphicsWKView::commitScaleChange()
> 
> You have begin and commit? Submit, commit? or begin, end?

Will change to begin/end.

> This is using old API, you should be able to call sent(DrawingAreaMessage::SetSize(viewSize) I beleive

The necessary code isn't generated for this class. Fixable of course, but I'd rather do that in a separate patch.

> > WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:125
> > +void TiledDrawingAreaProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID messageID, CoreIPC::ArgumentDecoder* arguments)
> > +{
> > +    switch (messageID.get<DrawingAreaProxyMessage::Kind>()) {
> > +    case DrawingAreaProxyMessage::TileUpdated: {
> 
> Can you verify it this is the way it is still done?

It is.

> WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:182
> > +        static const double ipcTimeout = 2.0;
> 
> Is ipcTimeout the best name?

Maybe not.. I'll call it tileUpdateTimeout instead then.

> > WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:349
> > +    IntRect visibleRect = mapFromContents(webViewVisibleRect());
> 
> I think we need to hook this up to visibleContentRect from webcore.

Ok.

> > WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:477
> > +    bool resized = false;
> 
> wasResized?

Ok.

> > WebKit2/UIProcess/TiledDrawingAreaProxy.h:92
> > +    void getKeepAndCoverAreaMultipliers(WebCore::FloatSize& keepMultiplier, WebCore::FloatSize& coverMultiplier)
> 
> Remove get?

I'd rather not

[Andreas Kling]

(In reply to comment #3)
> > > WebKit2/UIProcess/TiledDrawingAreaProxy.h:92
> > > +    void getKeepAndCoverAreaMultipliers(WebCore::FloatSize& keepMultiplier, WebCore::FloatSize& coverMultiplier)
> > 
> > Remove get?
> 
> I'd rather not

Premature submit- I'd rather not since the method isn't returning anything but modifying output vars. What would you suggest?

[Andreas Kling]

Committed r72010: <http://trac.webkit.org/changeset/72010>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72010 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
fast/workers/storage/interrupt-database-sync.html