Bug 69151 – [Qt][WK2] Synchronize tiling with accelerated compositing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

Right now tiling works on the non-composited content only, which creates an unnatural distinction between "composited" and "non-composited" content. This results in hard-to-detect bugs, such as composited content looking different than non-composited content.

The idea is to use the tiling functionality in TiledBackingStore and TiledDrawingAreaProxy in each layer, with some optimizations for the non-composited content, and to force compositing mode so that the rendering only goes through a single path.

[Noam Rosenthal]

Created attachment 109313 [details]
Not for review.

WIP patch.

[Kenneth Rohde Christiansen]

Comment on attachment 109313 [details]
Not for review.

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

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:247
> +    bool found = false;

exists?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:256
> +    if (!m_transforms.target.isInvertible())

// not visible?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:389
> +
> +

only one newline

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:393
> +        HashMap<int, ManagedTile>::iterator endIterator = m_managedTiles.end();

normally just called end in webkit

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:400
> +            else if (opacity > 0.95)
> +                tiles.prepend(&it->second);

comment?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:404
> +        TransformationMatrix scaledMatrix = m_transforms.target;
> +        TransformationMatrix replicaMatrix;

is this needed?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:719
> -            m_tiles.clear();
> +            m_basicTiles.clear();

owned/adopted tiles. externallyManagedTiles

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:111
> +    enum TileMode {

TileOwnership { Externally , I...

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:245
> +        bool backBufferUpdated;

isBack....

[Noam Rosenthal]

Created attachment 110060 [details]
Patch

[Noam Rosenthal]

Comment on attachment 110060 [details]
Patch

This is the WebCore part of the patch. It can stand on its own, though functionality is still disabled.

[Kenneth Rohde Christiansen]

Comment on attachment 110060 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:360
> +                targetRect.x() + (tileRect.x() - targetRect.x()) / m_state.contentsScale,

It would be great if you could do such renames in separate patches... I can quickly r=cq=me them. It will keep the patch size down.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:394
> +
> +

double newline

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:395
> +    if (m_state.tileOwnership == ExternallyManagedTiles) {

This indeed looks nicer!

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:405
> +            else if (opacity > 0.95)
> +                tiles.prepend(&it->second);

You forgot adding the comment here that we talked about

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:426
> +
> +

Maybe for the sake of clearness you could add 
// InternallyManagedTiles:

here

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:598
> +int TextureMapperNode::createContentTile(float scale)

Contents?

I wonder if there is a nice way to make it clear which methods are for the external managed tiles only

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:607
> +void TextureMapperNode::removeContentTile(int id)

Contents?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:612
> +void TextureMapperNode::setTileBackBufferTextureForDirectlyCompositedImage(int id, const IntRect& sourceRect, const FloatRect& targetRect, BitmapTexture* texture)

You use FloatRect& targetRect here... in setContentTileBackBuffer you use IntRect... why the inconsistency?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:628
> +void TextureMapperNode::clearDirectlyCompositedImageTiles()

It could be a bit more clear if you add "All" in there. ie. clearAllDirec...

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:657
> +void TextureMapperNode::swapContentBuffers()

Contents*?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:659
> +    HashMap<int, ExternallyManagedTile>::iterator endIterator = m_externallyManagedTiles.end();

We normally just use "end" and not "endIterator"

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:772
>  
> +

Wny this newline?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:153
> +    void setTileOwnership(TileOwnership o) { m_state.tileOwnership = o; }

please write "ownership" We normally use "o" for object in the Render* classes.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:248
> +        ExternallyManagedTile(float newScale = 1.0)

=1 ? Why newScale... isn't it just scale as this is a ctor

[Noam Rosenthal]

Created attachment 110383 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 110383 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Enable "externally managed" tiles in TextureMapperNodes.
> +        Currently, TextureMapperNodes manage tiles themselves, the tiles being there
> +        only to overcome the 2k texture size limitation. For WebKit2, we want those tiles to be managed
> +        externally, namely through the web process via the remote tile backend for TiledBackingStore.

Why is the line length so different? This is a changelog.. and I would wrap around 80-100 chars and try to keep all lines at the about same length

> Source/WebCore/ChangeLog:18
> +        Reviewed by NOBODY (OOPS!).

Move this up

> Source/WebCore/ChangeLog:21
> +        No new tests, code is still disabled. Eventually existing compositing tests running on WK2 would
> +        test this.

Better write:

Code is disabled for now, but covered by existing compositing tests.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:391
> +
> +

double newline

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:406
> +            // This code creates a transitional effect, showing tiles rendered in the "old" contents scale behind tiles rendered in the current contents scale.
> +            // This effect looks good only when there's no transparency, so we only use it when the opacity for the layer is above a certain threshold.

I would keep the comment to around 100 chars

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:255
> +    struct ExternallyManagedTile {
> +        bool isBackBufferUpdated;
> +        bool isDirectlyCompositedImage;
> +        ExternallyManagedTileBuffer frontBuffer;
> +        ExternallyManagedTileBuffer backBuffer;
> +        ExternallyManagedTile(float scale = 1.0)
> +            : isBackBufferUpdated(false)
> +            , isDirectlyCompositedImage(false)
> +            , scale(scale)
> +        {
> +        }
> +        float scale;
> +    };

your ordering here is a bit weird

[Noam Rosenthal]

Created attachment 110525 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110525 [details]
Patch

Clearing flags on attachment: 110525

Committed r97163: <http://trac.webkit.org/changeset/97163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Not finished just yet.

[Csaba Osztrogonác]

Guys, it broke the minimal build ... :-S

http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/34135/steps/compile-webkit/logs/stdio

You should have watch the bots ... Could you fix it ASAP or should we roll it out?

[Simon Hausmann]

(In reply to comment #12)
> Guys, it broke the minimal build ... :-S
> 
> http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/34135/steps/compile-webkit/logs/stdio
> 
> You should have watch the bots ... Could you fix it ASAP or should we roll it out?

Should be simple to fix, it's just about #if ENABLE(TILED_BACKING_STORE).

[Simon Hausmann]

On second thought, after Noam's change the texture mapper doesn't make sense anymore if tiling is disabled. So either the tiled backing store is added to the minimal config (does that make sense?) or the texture mapper should be disabled if there's no backing store.

[Noam Rosenthal]

(In reply to comment #14)
> On second thought, after Noam's change the texture mapper doesn't make sense anymore if tiling is disabled. So either the tiled backing store is added to the minimal config (does that make sense?) or the texture mapper should be disabled if there's no backing store.

For WebKit2... but for WebKit1 TextureMapper can still potentially work with TILED_BACKING_STORE disabled. I'd rather just fix the #ifdefs for now.

[Noam Rosenthal]

Created attachment 110576 [details]
Unbreak the minimal Qt bot

[WebKit Review Bot]

Comment on attachment 110576 [details]
Unbreak the minimal Qt bot

Clearing flags on attachment: 110576

Committed r97186: <http://trac.webkit.org/changeset/97186>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Created attachment 110595 [details]
Patch

This is still disabled, but most of the code is there.
When enabled, it will allow using AC for all layers, including the main layer. We'd use "force compositing" mode to ensure that.
This would in turn obsolete TiledDrawingArea and SGAgent, as those functionalities now reside in TiledBackingStore+TextureMapperNode.

[Ryosuke Niwa]

This patch appear to have broken Qt minimal build. Fix attempted in http://trac.webkit.org/changeset/97210.

[Ryosuke Niwa]

(In reply to comment #20)
> This patch appear to have broken Qt minimal build. Fix attempted in http://trac.webkit.org/changeset/97210.

Fixed in http://trac.webkit.org/changeset/97213.

[Kenneth Rohde Christiansen]

Comment on attachment 110595 [details]
Patch

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

Some comments from my first look thru... still need to make sense of this patch

> Source/WebKit2/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Layer-tree synchronization is done by passing WebKit2 messages between a LayerTreeHost
> +        on the WebProcess and a LayerTreeHostProxy on the UI process. Every layer in the tree has
> +        its own tiled backing store, and uses the LayerTreeHost communication channel to pass
> +        content up to the UI process. The UI process will later creates its own GraphicsLayer tree,
> +        based on TextureMapper, which can be painted directly with OpenGL.

Would be nice if you started with a reasoning for the patch, before descripting what it is doing

> Source/WebKit2/Shared/WebLayerTreeInfo.h:76
> +            bool imageUpdated: 1;

imageWasUpdated?

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:87
> +    virtual void updateWebView();

Wouldn't updateViewport be a better name? WebView is totally overloaded

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:48
> +class LayerTreeMessageToRenderer;
> +class LayerTreeHostProxy : public GraphicsLayerClient {

I would adda newline between these

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:75
> +protected:
> +
> +    PassOwnPtr<GraphicsLayer> createLayer(WebLayerID);

I would remove that newline

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:83
> +    virtual bool showDebugBorders() const { return false; }
> +    virtual bool showRepaintCounter() const { return false; }

#if !NDEBUG ?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:103
> +    int remoteTileIDToNodeTileID(int tileID) const;

I would have called it "nodeTileIDFromRemoteTileID"

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:121
> +    void ensureLayer(WebLayerID);
> +
> +#endif

remove that newline

> Source/WebKit2/UIProcess/LayerTreeHostProxy.messages.in:1
> +# Copyright (C) 2010, 2011 Apple Inc. All rights reserved.

s/Apple/Nokia ??

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:39
> +#include <cairo/OpenGLShims.h>

cairo?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:87
> +    m_layerInfo.id = sID++;

s_id? :-) s means static right

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:297
> +    setContentsToImage(0);

setImageAsContents? the name is a bit confusing especially when passing 0

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:416
> +    if (m_mainBackingStore && m_mainBackingStore->contentsScale() == scale)

no fuzzy comparison needed here?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:524
> +    if (!drawsContent() || m_image)

so you dont do this if you have an m_image?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:63
> +    , m_suspended(false)

isSuspended? or isPageSuspended ?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:192
> +void LayerTreeHostQt::didInstallPageOverlay()

Attach?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:199
> +void LayerTreeHostQt::didUninstallPageOverlay()

Detach?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:291
> +    m_pageOverlayLayer = nullptr;

Why not use that everywhere then instead of 0?

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:294
> +
> +

double newlines

[Noam Rosenthal]

 
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:39
> > +#include <cairo/OpenGLShims.h>
> 
> cairo?
We're reusing Cairo's OpenGL shims, in other places as well.

> 
> > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:192
> > +void LayerTreeHostQt::didInstallPageOverlay()
> Attach?
> > +void LayerTreeHostQt::didUninstallPageOverlay()
> Detach?
These are copied from LayerTreeHostCA.

[Noam Rosenthal]

Created attachment 110705 [details]
Patch

Addressed Kenneth's comments, though some of them are addressed in the previous reply.

[Simon Hausmann]

Comment on attachment 110705 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:1
> +    /*

Stray tab? :)

> Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:31
> +#include "LayerTreeHostQt.h"

This header file seems missing?

[Noam Rosenthal]

Created attachment 110873 [details]
Patch

[WebKit Review Bot]

Attachment 110873 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:69:  The parameter name "layerID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:70:  The parameter name "layerID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:71:  The parameter name "layerID" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 110873 [details]
Patch

Great stuff :)

[Simon Hausmann]

Comment on attachment 110873 [details]
Patch

r=me but please fix the style issues before landing :)

[Noam Rosenthal]

Created attachment 110878 [details]
Patch

Fixed style issues and last comments from Simon.

[Noam Rosenthal]

Created attachment 110938 [details]
Patch for landing

[Noam Rosenthal]

Still some patches to go.

[Collabora GTK+ EWS bot]

Comment on attachment 110878 [details]
Patch

Attachment 110878 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10068216

[Simon Hausmann]

Comment on attachment 110878 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:35
> +#include "LayerTreeHostProxyMessages.h"

As the EWS indicated: This header inclusion should be either guarded with the appropriate #ifdefs or the LTHPM.in file is added to the build systems of the other ports.

[Noam Rosenthal]

(In reply to comment #34)
> (From update of attachment 110878 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110878&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:35
> > +#include "LayerTreeHostProxyMessages.h"
> 
> As the EWS indicated: This header inclusion should be either guarded with the appropriate #ifdefs or the LTHPM.in file is added to the build systems of the other ports.

Yup. I'll push another patch with #ifdef guards, and make sure it passes EWS.

[Noam Rosenthal]

Created attachment 111020 [details]
Patch

This should fix it, will let go through EWS and then land.

[Noam Rosenthal]

Created attachment 111051 [details]
Patch

[WebKit Review Bot]

Comment on attachment 111051 [details]
Patch

Clearing flags on attachment: 111051

Committed r97549: <http://trac.webkit.org/changeset/97549>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #38)
> (From update of attachment 111051 [details])
> Clearing flags on attachment: 111051
> 
> Committed r97549: <http://trac.webkit.org/changeset/97549>

It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout.

[Csaba Osztrogonác]

Rollout patch landed in http://trac.webkit.org/changeset/97554

[Noam Rosenthal]

> It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout.

Easy to say. It took over 6 hours for the commit queue to land in that patch.
I'm ok with rolling this out for now.

[Noam Rosenthal]

(In reply to comment #42)
> > It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout.
> 
> Easy to say. It took over 6 hours for the commit queue to land in that patch.
> I'm ok with rolling this out for now.

It didn't actually break any bot. Flaky tests.... recommitting. Please don't roll out unless bot failures have something to do with the bug.

[Noam Rosenthal]

Comment on attachment 111051 [details]
Patch

Re-committing, patch was rolled out because it was mistakenly blamed for flaky tests.

[WebKit Review Bot]

Comment on attachment 111051 [details]
Patch

Clearing flags on attachment: 111051

Committed r97559: <http://trac.webkit.org/changeset/97559>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Breaking stuff... more offensive #includes to move around.

[Csaba Osztrogonác]

(In reply to comment #42)
> > It broke the build on the SL bot ... :-/ Guys, after landing you _should have_ watch the bots ... The fix might be simple, but I won't fix bugs at weekend instead of others, but rollout.
> 
> Easy to say. It took over 6 hours for the commit queue to land in that patch.
> I'm ok with rolling this out for now.

Using commit-queue isn't compulsory. You can land your patch manually or with webkit-patch land-from-bug if you can't wait for CQ. But you have to ensure that you don't break build on any platform. It is another question why Mac port doesn't have EWS as other ports.

[Noam Rosenthal]

Created attachment 111274 [details]
Patch

One more try... moved some headers around and adapted to trunk. Should build on everything now - will follow the bots.

[WebKit Review Bot]

Attachment 111274 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   bc67541..f85897f  master     -> origin/master
	M	Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/garden-o-matic.html
	M	Tools/ChangeLog
r97634 = 07d47256354548be7595c405835f38f40c1b61e9 (refs/remotes/trunk)
	M	LayoutTests/fast/replaced/table-percent-width-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/rendering/RenderBox.h
	M	Source/WebCore/rendering/RenderBox.cpp
r97635 = f85897f3766189463c74375cf745a6d69f470fbd (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Created attachment 111289 [details]
Patch

[Noam Rosenthal]

Comment on attachment 111289 [details]
Patch

Clearing flags on attachment: 111289

Committed r97639: <http://trac.webkit.org/changeset/97639>

[Noam Rosenthal]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Still some patches to go.

[Noam Rosenthal]

Created attachment 111706 [details]
Patch

This patch effectively enables the previous patches.
It's a "big" change in terms of rendering and might cause some instability. However, I hope we'd be able to fix those by going forwards and not backwards, as it's very hard to fix all those issues in advance on a moving target like QTouchWebView.

[Kenneth Rohde Christiansen]

Comment on attachment 111706 [details]
Patch

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

Looks good, but I want jocelyn to have a quick look

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:48
> +void QTouchWebPage::initSceneGraphConnections()

initialize*?

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53
> +    QSGCanvas* canvas = this->canvas();

Any reason for this?

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:173
> +static void computeOverlaps(const QSGItem* curItem, const QSGItem* item, bool& hasBehind, bool& hasInFront, const QRectF& itemBoundingRect, bool nowInFront = false)

currentItem. What is the other item? That could be more clear. hasOverlapInFront? maybe someone has better suggestions

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:181
> +    QList<QSGItem *> childItems = curItem->childItems();

coding style

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:190
> +        (nowInFront ? hasInFront : hasBehind) = true;

That is pretty ugly :-) bnut well

> Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:58
> +    QWebPreferencesPrivate::get(preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, false);

Maybe we should just add a public setting?

> Source/WebKit2/UIProcess/qt/qtouchwebpageproxy.cpp:60
> +    DrawingAreaProxy* drawingArea = m_webPageProxy->drawingArea();
> +    if (drawingArea)

This could be one line

[Jocelyn Turcotte]

Comment on attachment 111706 [details]
Patch

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

LGTM beside a few details.

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:92
>      if (!oldNode)
>          oldNode = new QSGNode;

You could remove updatePaintNode completely and remove "setFlag(ItemHasContents);" from the constructor.

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:259
> +    QObject::connect(engine, SIGNAL(beforeRendering()), q, SLOT(_q_onBeforeSceneRender()), Qt::DirectConnection);
> +    QObject::connect(engine, SIGNAL(afterRendering()), q, SLOT(_q_onAfterSceneRender()), Qt::DirectConnection);

I'd prefer if it was hard-coded to one of the two to make the limitation obvious (probably beforeRendering to allow overlays) until we find a proper way to integrate with the scene graph, or decide that this is the way to go.

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:70
> -
> +    

This one slipped in.

[Noam Rosenthal]

(In reply to comment #56)
> > Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.cpp:58
> > +    QWebPreferencesPrivate::get(preferences())->setAttribute(QWebPreferencesPrivate::AcceleratedCompositingEnabled, false);
> 
> Maybe we should just add a public setting?
See comment in code... There's no point in a public setting if we're going to rely on AC as the main rendering code path.

[Noam Rosenthal]

> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:92
> >      if (!oldNode)
> >          oldNode = new QSGNode;
> 
> You could remove updatePaintNode completely and remove "setFlag(ItemHasContents);" from the constructor.

OK, though eventually the idea would be to implement this if the item is in the middle of the scene.

> 
> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:259
> > +    QObject::connect(engine, SIGNAL(beforeRendering()), q, SLOT(_q_onBeforeSceneRender()), Qt::DirectConnection);
> > +    QObject::connect(engine, SIGNAL(afterRendering()), q, SLOT(_q_onAfterSceneRender()), Qt::DirectConnection);
> 
> I'd prefer if it was hard-coded to one of the two to make the limitation obvious (probably beforeRendering to allow overlays) until we find a proper way to integrate with the scene graph, or decide that this is the way to go.

Right now, though, there are items behind the web-view, background and such. But I can make it work with "behind" I believe by changing the QML in MiniBrowser. It would make the code much simpler because computeOverlap can be omitted.

[Viatcheslav Ostapenko]

(In reply to comment #56)
> (From update of attachment 111706 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review
> > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53
> > +    QSGCanvas* canvas = this->canvas();
> 
> Any reason for this?

QTouchWebView/QTouchWebPage can be constructed without attaching to canvas initially. In this case initialization will be delayed until QTouchWebPage::itemChange with change == ItemSceneChange .

[Noam Rosenthal]

(In reply to comment #60)
> (In reply to comment #56)
> > (From update of attachment 111706 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111706&action=review
> > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53
> > > +    QSGCanvas* canvas = this->canvas();
> > 
> > Any reason for this?
> 
> QTouchWebView/QTouchWebPage can be constructed without attaching to canvas initially. In this case initialization will be delayed until QTouchWebPage::itemChange with change == ItemSceneChange .
I think the question was about using a local variable, which is probably not necessary.
We should add a comment along the lines of your reply here, though.

[Noam Rosenthal]

Created attachment 111978 [details]
Patch

For now, the easiest way forward is to render the web view after the rest of the scene. Later on we can use FBO for the rest of the cases.

[Noam Rosenthal]

Created attachment 111979 [details]
Patch

[WebKit Review Bot]

Attachment 111978 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:193:  QTouchWebPagePrivate::_q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:199:  QTouchWebPagePrivate::_q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:41:  _q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

(In reply to comment #64)
> Attachment 111978 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> 
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:193:  QTouchWebPagePrivate::_q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:199:  QTouchWebPagePrivate::_q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:41:  _q_onAfterSceneRender is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Source/WebKit2/UIProcess/API/qt/qtouchwebpage_p.h:42:  _q_onSceneGraphInitialized is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 4 in 14 files
> 

False positives, those things are supposed to work with the Qt coding style.

[Simon Hausmann]

Comment on attachment 111979 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54
> +    m_pageView->update();

This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me.

[Noam Rosenthal]

(In reply to comment #66)
> (From update of attachment 111979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54
> > +    m_pageView->update();
> 
> This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me.

Should we fix it with setting ItemHasContents to true, or by updating the canvas some other way?

[Simon Hausmann]

(In reply to comment #67)
> (In reply to comment #66)
> > (From update of attachment 111979 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111979&action=review
> > 
> > > Source/WebKit2/UIProcess/qt/QtTouchViewInterface.cpp:54
> > > +    m_pageView->update();
> > 
> > This call to QSGItem::update() causes ASSERT: "flags() & ItemHasContents" in file /home/shausman/dev/qt5/qtdeclarative/src/declarative/items/qsgitem.cpp, line 3039 for me.
> 
> Should we fix it with setting ItemHasContents to true, or by updating the canvas some other way?

I think setting ItemHasContents to true is ok. It did the trick for me.

I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same?

[Noam Rosenthal]

> I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same?

I've seen this before, and it might have come back... it's hard to notice crispness bugs sometimes.
Does it go back to crispness once you zoom in and out? and is it crisp on other layers?

No'am

[Simon Hausmann]

(In reply to comment #69)
> > I got pixels onto the screen (Desktop) :). But I noticed that the tiles on the root layer were smooth-scaled, so the text didn't look crisp compared to the earlier rendering. Do you see the same?
> 
> I've seen this before, and it might have come back... it's hard to notice crispness bugs sometimes.
> Does it go back to crispness once you zoom in and out? and is it crisp on other layers?

It goes away when I zoom in enough. Ah well, that shouldn't prevent us from moving forward.

[Simon Hausmann]

Comment on attachment 111979 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:53
> +void QTouchWebPage::initSceneGraphConnections()
> +{
> +    if (d->paintingIsInitialized)
> +        return;
> +    d->paintingIsInitialized = true;
> +    if (!canvas())
> +        return;

With the QML mini browser I'm running into the situation where when this function is called from the constructor, we do not have a canvas() yet. Later when the canvas is assigned and we get notified through itemChange(), this function is called again. Unfortunately paintingIsInitialized was set to true and therefore we don't end up connecting to the scene graph signal(s). Moving the paintingIsInitialized = true assigned to _after_ the "if (!canvas()) return" block fixes that.

[Simon Hausmann]

Yay, I got it to run on the N9.

The only "error" I saw was this:

ERROR: [TextureMapper GL] Command failed: glBindFramebuffer(GL_FRAMEBUFFER, 0) (501)

../../../Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp(725) : void WebCore::debugGLCommand(const char*, int)

from


void TextureMapperGL::bindSurface(BitmapTexture *surfacePointer)
{
    BitmapTextureGL* surface = static_cast<BitmapTextureGL*>(surfacePointer);

    if (!surface) {
        GL_CMD(glBindFramebuffer(GL_FRAMEBUFFER, 0))


Anyway, I don't think that's a "showstopper" :)

[Noam Rosenthal]

(In reply to comment #72)
> Yay, I got it to run on the N9.
> 
> The only "error" I saw was this:
> 
> ERROR: [TextureMapper GL] Command failed: glBindFramebuffer(GL_FRAMEBUFFER, 0) (501)
> 
> ../../../Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp(725) : void WebCore::debugGLCommand(const char*, int)

Sounds familiar.
Some of the N9-specific stuff was not upstreamed yet, namely going to software mode when going to the background and other similar candies.

[Simon Hausmann]

Comment on attachment 111979 [details]
Patch

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

r=me, with two changes:

1) Revert the ItemHasContents piece to avoid the ASSERT
2) Fix the location of the d->paintingIsInitialized = true assignment

We plan on doing the next qt5 update tomorrow, which will require us to adapt to source incompatible changes (QSG* -> QQuick* renaming in qtdeclarative). So perhaps you can land this today your time? Otherwise the patch will need a rebase :)

> Source/WebKit2/UIProcess/API/qt/qtouchwebpage.cpp:160
> +float computeEffectiveOpacity(const QSGItem* item)

Missing 'static' keyword :)

[alan]

Landing soon would be awesome, so then we can go ahead with more cleanups around the qt wk2 classes. nice work btw.

[Simon Hausmann]

Committed r98706: <http://trac.webkit.org/changeset/98706>