[webkit-reviews] review canceled: [Bug 66771] [Qt][WK2] Drive tiling from the WebProcess and reuse TiledBackingStore. : [Attachment 105006] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at nokia.com> has canceled Jocelyn Turcotte
<jocelyn.turcotte at nokia.com>'s request for review:
Bug 66771: [Qt][WK2] Drive tiling from the WebProcess and reuse
TiledBackingStore.
https://bugs.webkit.org/show_bug.cgi?id=66771

Attachment 105006: Patch
https://bugs.webkit.org/attachment.cgi?id=105006&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at nokia.com>
(In reply to comment #6)
> > Source/WebCore/ChangeLog:20
> > +	     - TiledBackingStoreRemoteTile forwards tile creations, update and
removals to the proxy.
> 
> RemoteTile? Any reasons why this is not called TileProxy or so?
This is not a proxy itself but only an intermediate that output calls through
it's interface. The goal is to use the same class both for tiling in
GraphicLayers and in the DrawingArea for a while.

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:206
> >  
> > +float TiledBackingStore::coverageRatio(const WebCore::IntRect&
contentsRect)
> 
> Maybe it would be good with a short comment showing what the purpose are of
these methods.
Fixed

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:105
> > +	 HashMap<int, int> m_tilesToNodes;
> 
> I think it is more common in WebKit to call it someting like tileNodeMap. But
maybe just adding Map to the end would make this more clear
Fixed

> > Source/WebKit2/UIProcess/qt/SGAgent.cpp:48
> > +struct NodeUpdateSetBackBuffer : public NodeUpdate {
> 
> When we do a code camp next time, someone will have to explain to me how this
scene graph works :-)
Sure :)

> > Source/WebKit2/UIProcess/qt/SGTileNode.h:39
> > +	 void swapIfNeeded();
> 
> swapBuffersIfNeeded?
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:127
> > +	 return !(m_suspended || m_isWaitingForUIProcess);
> 
> I would prefer without the () ... makes the code easier to read
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:132
> > +	 return IntRect(IntPoint(0, 0), m_webPage->size());
> 
> IntPoint::zero() :-)
Fixed

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp:135
> > +IntRect TiledDrawingArea::tiledBackingStoreVisibleRect()
> 
> const ?
This is an implemented method from the interface, since it can be more than a
simple accessor I'd prefer not to force the const contract on all (possible)
implementations.

> > Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.h:58
> > +	 virtual void setVisibleContentRect(const WebCore::IntRect&);
> > +	 virtual void setContentsScale(float);
> 
> contentRect but content*S*Scale.
visibleContentRect is written everywhere without the s, and contentsScale is
written everywhere with the s.
For grep efficiency I'd keep it that way, but it would be nice to clean this up
once for all one day.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:75
> > +	 page()->process()->send(Messages::DrawingArea::RenderNextFrame(),
page()->pageID());
> 
> Frame is a pretty overloaded word :-( specially in web/graphics technology
Humm, RenderNextDocumentState? RenderNextContentsState? I think RenderNextFrame
isn't so ambiguous so I'd keep it, tell me what you would prefer.
The same should be applied to DidRenderFrame if we change it.