Bug 69151 - [Qt][WK2] Synchronize tiling with accelerated compositing
Summary: [Qt][WK2] Synchronize tiling with accelerated compositing
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: Noam Rosenthal
URL:
Keywords: Qt
Depends on: 70174 70176
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-30 09:51 PDT by Noam Rosenthal
Modified: 2011-10-28 01:00 PDT (History)
12 users (show)

See Also:


Attachments
Not for review. (122.98 KB, patch)
2011-09-30 10:50 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (24.58 KB, patch)
2011-10-06 16:52 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (24.30 KB, patch)
2011-10-10 12:46 PDT, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch (23.93 KB, patch)
2011-10-11 09:37 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Unbreak the minimal Qt bot (2.70 KB, patch)
2011-10-11 14:29 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (96.18 KB, patch)
2011-10-11 15:53 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (96.09 KB, patch)
2011-10-12 10:53 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (101.00 KB, patch)
2011-10-13 10:35 PDT, Noam Rosenthal
hausmann: review+
Details | Formatted Diff | Diff
Patch (101.25 KB, patch)
2011-10-13 10:53 PDT, Noam Rosenthal
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (101.32 KB, patch)
2011-10-13 17:09 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (101.67 KB, patch)
2011-10-14 08:56 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (101.43 KB, patch)
2011-10-14 12:26 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (101.43 KB, patch)
2011-10-17 09:21 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (101.45 KB, patch)
2011-10-17 11:42 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (23.16 KB, patch)
2011-10-19 18:19 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (19.50 KB, patch)
2011-10-21 10:18 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2011-10-21 10:19 PDT, Noam Rosenthal
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2011-09-30 09:51:51 PDT
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.
Comment 1 Noam Rosenthal 2011-09-30 10:50:33 PDT
Created attachment 109313 [details]
Not for review.

WIP patch.
Comment 2 Kenneth Rohde Christiansen 2011-09-30 15:45:45 PDT
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....
Comment 3 Noam Rosenthal 2011-10-06 16:52:55 PDT
Created attachment 110060 [details]
Patch
Comment 4 Noam Rosenthal 2011-10-06 16:54:05 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2011-10-07 03:34:26 PDT
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
Comment 6 Noam Rosenthal 2011-10-10 12:46:55 PDT
Created attachment 110383 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2011-10-11 03:19:58 PDT
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
Comment 8 Noam Rosenthal 2011-10-11 09:37:02 PDT
Created attachment 110525 [details]
Patch
Comment 9 WebKit Review Bot 2011-10-11 10:42:10 PDT
Comment on attachment 110525 [details]
Patch

Clearing flags on attachment: 110525

Committed r97163: <http://trac.webkit.org/changeset/97163>
Comment 10 WebKit Review Bot 2011-10-11 10:42:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Noam Rosenthal 2011-10-11 10:44:55 PDT
Not finished just yet.
Comment 12 Csaba Osztrogonác 2011-10-11 13:51:52 PDT
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?
Comment 13 Simon Hausmann 2011-10-11 14:02:34 PDT
(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).
Comment 14 Simon Hausmann 2011-10-11 14:13:06 PDT
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.
Comment 15 Noam Rosenthal 2011-10-11 14:14:35 PDT
(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.
Comment 16 Noam Rosenthal 2011-10-11 14:29:00 PDT
Created attachment 110576 [details]
Unbreak the minimal Qt bot
Comment 17 WebKit Review Bot 2011-10-11 14:42:02 PDT
Comment on attachment 110576 [details]
Unbreak the minimal Qt bot

Clearing flags on attachment: 110576

Committed r97186: <http://trac.webkit.org/changeset/97186>
Comment 18 WebKit Review Bot 2011-10-11 14:42:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Noam Rosenthal 2011-10-11 15:53:16 PDT
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.
Comment 20 Ryosuke Niwa 2011-10-11 18:26:25 PDT
This patch appear to have broken Qt minimal build. Fix attempted in http://trac.webkit.org/changeset/97210.
Comment 21 Ryosuke Niwa 2011-10-11 18:52:18 PDT
(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.
Comment 22 Kenneth Rohde Christiansen 2011-10-12 01:30:24 PDT
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
Comment 23 Noam Rosenthal 2011-10-12 10:21:59 PDT
 
> > 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.
Comment 24 Noam Rosenthal 2011-10-12 10:53:19 PDT
Created attachment 110705 [details]
Patch

Addressed Kenneth's comments, though some of them are addressed in the previous reply.
Comment 25 Simon Hausmann 2011-10-13 06:00:32 PDT
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?
Comment 26 Noam Rosenthal 2011-10-13 10:35:49 PDT
Created attachment 110873 [details]
Patch
Comment 27 WebKit Review Bot 2011-10-13 10:39:45 PDT
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.
Comment 28 Simon Hausmann 2011-10-13 10:50:55 PDT
Comment on attachment 110873 [details]
Patch

Great stuff :)
Comment 29 Simon Hausmann 2011-10-13 10:52:01 PDT
Comment on attachment 110873 [details]
Patch

r=me but please fix the style issues before landing :)
Comment 30 Noam Rosenthal 2011-10-13 10:53:24 PDT
Created attachment 110878 [details]
Patch

Fixed style issues and last comments from Simon.
Comment 31 Noam Rosenthal 2011-10-13 17:09:29 PDT
Created attachment 110938 [details]
Patch for landing
Comment 32 Noam Rosenthal 2011-10-13 21:30:24 PDT
Still some patches to go.
Comment 33 Collabora GTK+ EWS bot 2011-10-13 22:11:43 PDT
Comment on attachment 110878 [details]
Patch

Attachment 110878 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10068216
Comment 34 Simon Hausmann 2011-10-14 00:14:38 PDT
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.
Comment 35 Noam Rosenthal 2011-10-14 07:47:02 PDT
(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.
Comment 36 Noam Rosenthal 2011-10-14 08:56:35 PDT
Created attachment 111020 [details]
Patch

This should fix it, will let go through EWS and then land.
Comment 37 Noam Rosenthal 2011-10-14 12:26:48 PDT
Created attachment 111051 [details]
Patch
Comment 38 WebKit Review Bot 2011-10-15 00:31:45 PDT
Comment on attachment 111051 [details]
Patch

Clearing flags on attachment: 111051

Committed r97549: <http://trac.webkit.org/changeset/97549>
Comment 39 WebKit Review Bot 2011-10-15 00:31:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Csaba Osztrogonác 2011-10-15 02:32:31 PDT
(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.
Comment 41 Csaba Osztrogonác 2011-10-15 02:36:52 PDT
Rollout patch landed in http://trac.webkit.org/changeset/97554
Comment 42 Noam Rosenthal 2011-10-15 07:45:02 PDT
> 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.
Comment 43 Noam Rosenthal 2011-10-15 07:53:57 PDT
(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.
Comment 44 Noam Rosenthal 2011-10-15 07:55:01 PDT
Comment on attachment 111051 [details]
Patch

Re-committing, patch was rolled out because it was mistakenly blamed for flaky tests.
Comment 45 WebKit Review Bot 2011-10-15 09:00:56 PDT
Comment on attachment 111051 [details]
Patch

Clearing flags on attachment: 111051

Committed r97559: <http://trac.webkit.org/changeset/97559>
Comment 46 WebKit Review Bot 2011-10-15 09:01:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Noam Rosenthal 2011-10-15 09:12:27 PDT
Breaking stuff... more offensive #includes to move around.
Comment 48 Csaba Osztrogonác 2011-10-16 02:13:02 PDT
(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.
Comment 49 Noam Rosenthal 2011-10-17 09:21:27 PDT
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.
Comment 50 WebKit Review Bot 2011-10-17 11:23:53 PDT
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.
Comment 51 Noam Rosenthal 2011-10-17 11:42:38 PDT
Created attachment 111289 [details]
Patch
Comment 52 Noam Rosenthal 2011-10-17 13:02:56 PDT
Comment on attachment 111289 [details]
Patch

Clearing flags on attachment: 111289

Committed r97639: <http://trac.webkit.org/changeset/97639>
Comment 53 Noam Rosenthal 2011-10-17 13:03:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 54 Noam Rosenthal 2011-10-17 13:19:34 PDT
Still some patches to go.
Comment 55 Noam Rosenthal 2011-10-19 18:19:47 PDT
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.
Comment 56 Kenneth Rohde Christiansen 2011-10-20 02:28:55 PDT
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
Comment 57 Jocelyn Turcotte 2011-10-20 03:01:43 PDT
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.
Comment 58 Noam Rosenthal 2011-10-20 07:26:00 PDT
(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.
Comment 59 Noam Rosenthal 2011-10-20 07:28:40 PDT
> > 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.
Comment 60 Viatcheslav Ostapenko 2011-10-20 07:41:22 PDT
(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 .
Comment 61 Noam Rosenthal 2011-10-20 08:12:04 PDT
(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.
Comment 62 Noam Rosenthal 2011-10-21 10:18:13 PDT
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.
Comment 63 Noam Rosenthal 2011-10-21 10:19:44 PDT
Created attachment 111979 [details]
Patch
Comment 64 WebKit Review Bot 2011-10-21 10:21:29 PDT
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.
Comment 65 Noam Rosenthal 2011-10-21 10:28:27 PDT
(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.
Comment 66 Simon Hausmann 2011-10-25 06:24:54 PDT
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.
Comment 67 Noam Rosenthal 2011-10-25 08:22:18 PDT
(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?
Comment 68 Simon Hausmann 2011-10-26 01:00:08 PDT
(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?
Comment 69 Noam Rosenthal 2011-10-26 07:11:38 PDT
> 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
Comment 70 Simon Hausmann 2011-10-27 03:44:31 PDT
(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.
Comment 71 Simon Hausmann 2011-10-27 03:46:37 PDT
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.
Comment 72 Simon Hausmann 2011-10-27 07:12:11 PDT
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" :)
Comment 73 Noam Rosenthal 2011-10-27 07:18:55 PDT
(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.
Comment 74 Simon Hausmann 2011-10-27 07:21:06 PDT
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 :)
Comment 75 zalan 2011-10-27 07:34:28 PDT
Landing soon would be awesome, so then we can go ahead with more cleanups around the qt wk2 classes. nice work btw.
Comment 76 Simon Hausmann 2011-10-28 01:00:36 PDT
Committed r98706: <http://trac.webkit.org/changeset/98706>