[Webkit-unassigned] [Bug 49526] [WK2][Qt] WebKit2 implementation of tiled backing store

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49526





--- Comment #2 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-11-15 02:24:53 PST ---
(From update of attachment 73877)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.