Bug 105787 - [Texmap] Refactor code related to debug border and repaint count.
Summary: [Texmap] Refactor code related to debug border and repaint count.
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: Dongseong Hwang
URL:
Keywords:
Depends on: 105781 107196
Blocks: 108779 107073 107099 107198 107910 107942 108401
  Show dependency treegraph
 
Reported: 2012-12-27 02:41 PST by Dongseong Hwang
Modified: 2013-02-03 17:11 PST (History)
19 users (show)

See Also:


Attachments
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] (25.69 KB, patch)
2012-12-27 02:47 PST, Dongseong Hwang
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
WIP Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2] (57.21 KB, patch)
2012-12-27 02:56 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2] (57.68 KB, patch)
2012-12-27 03:17 PST, Dongseong Hwang
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] (32.85 KB, patch)
2012-12-28 01:09 PST, Dongseong Hwang
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Not for Review(WIP) : [TexMap] Refactor code related to debug border and repaint count. [2/2] (57.40 KB, patch)
2012-12-28 01:48 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] (27.84 KB, patch)
2013-01-02 22:22 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] (27.65 KB, patch)
2013-01-10 22:12 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2] (57.97 KB, patch)
2013-01-11 01:17 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2] (61.06 KB, patch)
2013-01-11 03:35 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : [Qt][EFL][GTK][TexMap] Refactor code related to debug border and repaint count. (66.97 KB, patch)
2013-01-14 16:26 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (67.48 KB, patch)
2013-01-14 18:44 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (61.39 KB, patch)
2013-01-15 18:19 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (64.08 KB, patch)
2013-01-15 19:05 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (66.17 KB, patch)
2013-01-16 00:19 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (66.67 KB, patch)
2013-01-16 01:08 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (57.39 KB, patch)
2013-01-17 16:32 PST, Dongseong Hwang
benjamin: review-
Details | Formatted Diff | Diff
Patch (57.63 KB, patch)
2013-01-18 20:03 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (36.03 KB, patch)
2013-01-24 21:28 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (35.56 KB, patch)
2013-01-25 01:48 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch : It is a follow-up patch not to change layout test results. (2.32 KB, patch)
2013-01-28 16:51 PST, Dongseong Hwang
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Patch : It is a follow-up patch not to change layout test results. (2.25 KB, patch)
2013-01-28 22:27 PST, Dongseong Hwang
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff
Patch for landing : It is a follow-up patch not to change layout test results. (2.26 KB, patch)
2013-01-28 22:44 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-12-27 02:41:16 PST
Currently, TextureMapperBackingStore, CoordinatedBackingStore and
GraphicsLayerTextureMapper have duplicated code to draw debug border or repaint
count. This patch refactors that all platform layers draw debug border and repaint
count in the consistent way.
Comment 1 Dongseong Hwang 2012-12-27 02:47:58 PST
Created attachment 180783 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment 2 Dongseong Hwang 2012-12-27 02:56:40 PST
Created attachment 180784 [details]
WIP Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment 3 Dongseong Hwang 2012-12-27 03:07:43 PST
Not for review: I need to test more.

I need feedback, especially how to deal with [QT_]WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS
Comment 4 Dongseong Hwang 2012-12-27 03:17:34 PST
Created attachment 180785 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment 5 Noam Rosenthal 2012-12-27 07:29:24 PST
Comment on attachment 180783 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]

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

I wanted to do this for ages but was lazy :)
However I have some comments.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:121
> +    TextureMapperPaintParameters parameters;
> +    parameters.modelViewMatrix = transform;
> +    parameters.opacity = options.opacity;
> +    parameters.mask = options.mask.get();

Parameters and options are colliding here.
We should move TextureMapperPaintOptions to be defined in TextureMapperLayer.cpp, and have an internal member of TextureMapperPaintParameters.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:32
> +class TextureMapperPaintParameters {

This should be in TextureMapper.h, maybe later we'd want to change the signature of TextureMapper functions to use it.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:34
> +    TransformationMatrix modelViewMatrix;

transform instead of modelViewMatrix.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:36
> +    BitmapTexture* mask;

RefPtr

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:38
> +    TextureMapperPaintParameters()

Any reason why targetRect is left out?
Comment 6 Noam Rosenthal 2012-12-27 07:42:52 PST
Comment on attachment 180785 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]

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

This is actually a big change to how we use repaint counters, which would make them slow and unusable since we draw text and upload it for every paint even if there is no update.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:402
> +    updateDebugBorderAndRepaintCount();

This is wrong :)
repaint counter should be updated when the backing store is actually updated, not when the layer is flushed.
Comment 7 Dongseong Hwang 2012-12-27 18:48:23 PST
(In reply to comment #6)
> (From update of attachment 180785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180785&action=review
> 
> This is actually a big change to how we use repaint counters, which would make them slow and unusable since we draw text and upload it for every paint even if there is no update.

Very valid concern. Currently CoordinatedBackingStore draws repaint count in this slow way. I apply this way to all platform layers. I think we can refactor later. And IMHO it would be not bad to draw debug visual slowly.

My later improvement idea is to prepare repaint count texture in TextureMapperLayer::flushCompositingStateForThisLayerOnly()

if (changeMask & RepaintCountChange) {
    m_state.repaintCount = graphicsLayer->repaintCount();
    prepare texture!!
}


> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:402
> > +    updateDebugBorderAndRepaintCount();
> 
> This is wrong :)
> repaint counter should be updated when the backing store is actually updated, not when the layer is flushed.

Yes, so I checked several conditions in updateDebugBorderAndRepaintCount().

// only WK1 increases repaint count at this point.
if (!m_hasOwnBackingStore)
    return;

// If the layer has a backing store and dirty rect, we increases repaint count.
bool needsToRepaint = shouldHaveBackingStore() && (m_needsDisplay || !m_needsDisplayRect.isEmpty())
Comment 8 Dongseong Hwang 2012-12-27 18:56:33 PST
(In reply to comment #5)
> (From update of attachment 180783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180783&action=review
> 
> I wanted to do this for ages but was lazy :)
> However I have some comments.

While you improve overall performance of AC like background color, I can contribute like this readability refactoring :)

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:121
> > +    TextureMapperPaintParameters parameters;
> > +    parameters.modelViewMatrix = transform;
> > +    parameters.opacity = options.opacity;
> > +    parameters.mask = options.mask.get();
> 
> Parameters and options are colliding here.
> We should move TextureMapperPaintOptions to be defined in TextureMapperLayer.cpp, and have an internal member of TextureMapperPaintParameters.

It is something like what I want to hear from you.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:38
> > +    TextureMapperPaintParameters()
> 
> Any reason why targetRect is left out?

For example, CoordinatedBackingStore needs to make new TextureMapperPaintParameters per tile if CoordinatedBackingStore stores targetRect.
However, we can store targetRect in TextureMapperPaintParameters if we want :)

And there is no reason TextureMapper is left out.

Do you think we should store all parameters including TextureMapper and targetRect in TextureMapperPaintParameters?



btw, could you review Bug 105760 and Bug 105781 firstly. This bug depends on both physically.
Comment 9 Dongseong Hwang 2012-12-28 01:09:56 PST
Created attachment 180852 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment 10 Dongseong Hwang 2012-12-28 01:43:38 PST
Comment on attachment 180852 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] 

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33
> +    TextureMapperPaintParameters parameters;

I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier.

btw, I encounter new problem. I will add new member into TextureMapperPaintParameters:
    bool showDebugBorders;
    Color debugBorderColor;
    float debugBorderWidth;
    bool showRepaintCounter;
    int repaintCount;

But it is weird for TextureMapperPaintOptions to have those new members.

IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different.
How do you think?
Comment 11 Dongseong Hwang 2012-12-28 01:48:47 PST
Created attachment 180856 [details]
Not for Review(WIP) : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment 12 Dongseong Hwang 2012-12-28 01:51:54 PST
Comment on attachment 180856 [details]
Not for Review(WIP) : [TexMap] Refactor code related to debug border and repaint count. [2/2] 

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

To clarify my problem, I submitted this WIP second patch.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:66
> +    int repaintCount;

5 members is not needed by TextureMapperPaintOptions although TextureMapperPaintParameters is a member of TextureMapperPaintOptions.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:117
> +    parameters.repaintCount = m_state.repaintCount;

This ugly code is still needed.
Comment 13 Noam Rosenthal 2012-12-28 01:59:48 PST
Comment on attachment 180852 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] 

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

I'm doing some work on mask code, which would make it so that we actually remove the "mask" parameter from textureMapperPlatformLayer.
I'm thinking overloads of TextureMapperLayer should *always* support only transform and opacity, and for everything else they should provide a BitmapTexture and let TextureMapperLayer handle it.

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33
>> +    TextureMapperPaintParameters parameters;
> 
> I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier.
> 
> btw, I encounter new problem. I will add new member into TextureMapperPaintParameters:
>     bool showDebugBorders;
>     Color debugBorderColor;
>     float debugBorderWidth;
>     bool showRepaintCounter;
>     int repaintCount;
> 
> But it is weird for TextureMapperPaintOptions to have those new members.
> 
> IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different.
> How do you think?

I think it's ok to have duplicated members. I meant they collide in a way where it's hard for a person reading the code to understand which is which.
Regarding the repaint counters - it should not be part of the paint parameters. It should be part of the "updateContents" process.
Comment 14 Dongseong Hwang 2013-01-02 21:01:07 PST
Comment on attachment 180852 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] 

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

>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:33
>>> +    TextureMapperPaintParameters parameters;
>> 
>> I don't store targetRect in TextureMapperPaintParameters because it is weird for TextureMapperPaintOptions to have targetRect as well as the tile related reason as I mentioned earlier.
>> 
>> btw, I encounter new problem. I will add new member into TextureMapperPaintParameters:
>>     bool showDebugBorders;
>>     Color debugBorderColor;
>>     float debugBorderWidth;
>>     bool showRepaintCounter;
>>     int repaintCount;
>> 
>> But it is weird for TextureMapperPaintOptions to have those new members.
>> 
>> IMO, TextureMapperPaintOptions has duplicated members although it collides TextureMapperPaintParameters because both class's purpose is different.
>> How do you think?
> 
> I think it's ok to have duplicated members. I meant they collide in a way where it's hard for a person reading the code to understand which is which.
> Regarding the repaint counters - it should not be part of the paint parameters. It should be part of the "updateContents" process.

How about renaming from TextureMapperPaintParameters to TextureMapperPlatformLayerPaintOptions.

IMO it is natural for repaintCount to belong to the paint parameters because we count repaintCount based on GraphicsLayer, not backing store.
So TextureMapperLayer holds repaintCount and pass to backing store at painting time.
And then TextureMapperLayer draws repaintCount using TextureMapperGL.
As I mentioned earlier, we can prepare repaintCount texture at TextureMapperLayer::flushCompositingLayer process.

I think it is reasonable for backing store to count repaintCount at updateContents process but we don't have a single point which applies repaintCount logic to all platform layers. And it needs another huge refactoring.
If needed, we can make it in further refactoring.
Comment 15 Dongseong Hwang 2013-01-02 22:22:09 PST
Created attachment 181151 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment 16 Dongseong Hwang 2013-01-02 22:27:59 PST
Comment on attachment 181151 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] 

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:122
> +    parameters.mask = options.mask.get();

TextureMapperPaintOptions has duplicated members to TextureMapperPlatformLayerPaintParameters because both classes have different purpose as we discussed.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:34
> +class TextureMapperPlatformLayerPaintParameters {

Rename TextureMapperPlatformLayerPaintParameters to distinguish with TextureMapperPaintOptions.
Replace it in TextureMapperPlatformLayer.h because TextureMapperPlatformLayerPaintParameters is part of TextureMapperPlatformLayer API.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
> +    BitmapTexture* mask;

Use BitmapTexture* instead of RefPtr<BitmapTexture> because I don't want "#include <BitmapTexture.h>"
I think after noamr refactors mask, he will remove the mask member.
Comment 17 Dongseong Hwang 2013-01-10 22:12:55 PST
Created attachment 182266 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2]
Comment 18 Dongseong Hwang 2013-01-11 01:17:09 PST
Created attachment 182291 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment 19 Dongseong Hwang 2013-01-11 03:35:01 PST
Created attachment 182317 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [2/2]
Comment 20 Dongseong Hwang 2013-01-11 03:37:20 PST
ping.

IMO patch [1/2] has no problem. could you review the patch.

And I need more feedback for patch [2/2].
Comment 21 Noam Rosenthal 2013-01-13 07:42:40 PST
Comment on attachment 182266 [details]
Patch : [TexMap] Refactor code related to debug border and repaint count. [1/2] 

In second thought, I think this is not a good direction.
By making it easy to add parameters to the paint functions, we make things easy to silently regress in subclasses of TextureMapperPlatformLayer if we add more parameters.

I prefer to even remove the mask parameters, and have that paint function always be paintToTextureMapper(tm, rect, transform, opacity), giving the reset of the control to the subclass, and add a TextureMapper::setMask/setDebugBorder etc.
Comment 22 Dongseong Hwang 2013-01-14 16:26:15 PST
Created attachment 182652 [details]
Patch : [Qt][EFL][GTK][TexMap] Refactor code related to debug border and repaint count.
Comment 23 Dongseong Hwang 2013-01-14 16:32:35 PST
(In reply to comment #21)
> (From update of attachment 182266 [details])
> In second thought, I think this is not a good direction.
> By making it easy to add parameters to the paint functions, we make things easy to silently regress in subclasses of TextureMapperPlatformLayer if we add more parameters.
> 
> I prefer to even remove the mask parameters, and have that paint function always be paintToTextureMapper(tm, rect, transform, opacity), giving the reset of the control to the subclass, and add a TextureMapper::setMask/setDebugBorder etc.

I agree. I felt some weirdness when making. It seems to be because of your explanation.

I gave up drawing debug visual via piggy-backing paintToTextureMapper(). Now I explicitly calls drawDebugVisuals() from TextureMapperLayer.

I made this patch as prerequisite of actor model, because debug visuals make GraphicsLayerTextureMapper and TextureMapperLayer code complex currently. IMO this patch also helps you make actor model implementation.
Comment 24 Dongseong Hwang 2013-01-14 16:36:59 PST
Comment on attachment 182652 [details]
Patch : [Qt][EFL][GTK][TexMap] Refactor code related to debug border and repaint count.

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

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:149
> +void TextureMapper::drawDebugVisuals(const FloatRect& targetRect, const TransformationMatrix& transform, bool showDebugBorders, const Color& borderColor, float borderWidth, bool showRepaintCounter, int repaintCount)

I will optimize this method later (in follow-up bug) via reusing repaint count texture. I will pass BitmapImage for repaintCount instead of repaintCount.
The BitmapImage will be created in TextureMapperLayer::flushCompositingStateForThisLayerOnly()
void TextureMapperLayer::flushCompositingStateForThisLayerOnly(GraphicsLayerTextureMapper* graphicsLayer)
{
    ....
    if (changeMask & RepaintCountChange)
        m_state.repaintCount = graphicsLayer->repaintCount();       <-- HERE
    ....
}
Comment 25 Dongseong Hwang 2013-01-14 18:44:35 PST
Created attachment 182677 [details]
Patch
Comment 26 Noam Rosenthal 2013-01-15 01:18:39 PST
Comment on attachment 182677 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:369
> +        if (isShowingRepaintCounter()) {
> +            incrementRepaintCount();
> +            m_changeMask |= TextureMapperLayer::RepaintCountChange;
> +        }

This change seems a bit different; Maybe it should come in a different patch?

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:427
> +    // When this has its own backing store (i.e. Qt WK1), update the repaint count before calling TextureMapperLayer::flushCompositingStateForThisLayerOnly().

(i.e. Qt WK1) -> you mean e.g.; GTK also uses ownBackingStore.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:136
> +    void drawDebugVisuals(const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount);

bool showDebugBorders seems unnecessary; use a color with no alpha or borderWidth of zero. Same with repaintCount; if it's 0 there's no point in painting it.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:149
> +        if (m_state.showDebugBorders || m_state.showRepaintCounter)
> +            m_backingStore->drawDebugVisuals(options.textureMapper, layerRect(), transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount);
>      }
>  
>      if (m_contentsLayer) {
>          ASSERT(!layerRect().isEmpty());
>          m_contentsLayer->paintToTextureMapper(options.textureMapper, m_state.contentsRect, transform, opacity, mask.get());
> +        if (m_state.showDebugBorders || m_state.showRepaintCounter)
> +            m_contentsLayer->drawDebugVisuals(options.textureMapper, m_state.contentsRect, transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount);

The API here seems a bit messy; Any particular reason to have drawDebugVisuals as one function with booleans instead of two functions?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.cpp:38
> +void TextureMapperPlatformLayer::drawDebugVisuals(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& transform, bool showDebugBorders, const Color& borderColor, float borderWidth, bool showRepaintCounter, int repaintCount)
> +{
> +    textureMapper->drawDebugVisuals(targetRect, transform, showDebugBorders, borderColor, borderWidth, showRepaintCounter, repaintCount);
> +}

This can be done in the header file.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:39
> +    virtual void drawDebugVisuals(TextureMapper*, const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount);

You really need it as a virtual function only for directly composited images; Otherwise you can use the layer transform normally. 
I think all this virtualization should be removed; We should draw the repaint counters only for the main backing store of the layer.
Comment 27 Philippe Normand 2013-01-15 01:28:40 PST
Wouldn't [TexMap][WK2] be enough tags for this bug? is it REALLY necessary to mention all the ports using texmap? :)
Comment 28 Dongseong Hwang 2013-01-15 04:17:02 PST
Comment on attachment 182677 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:369
>> +        }
> 
> This change seems a bit different; Maybe it should come in a different patch?

you're right. follow-up patch will contain it.

>> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:427
>> +    // When this has its own backing store (i.e. Qt WK1), update the repaint count before calling TextureMapperLayer::flushCompositingStateForThisLayerOnly().
> 
> (i.e. Qt WK1) -> you mean e.g.; GTK also uses ownBackingStore.

you're right. I was confused i.e. with e.g.

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:149
>> +            m_contentsLayer->drawDebugVisuals(options.textureMapper, m_state.contentsRect, transform, m_state.showDebugBorders, m_state.debugBorderColor, m_state.debugBorderWidth, m_state.showRepaintCounter, m_state.repaintCount);
> 
> The API here seems a bit messy; Any particular reason to have drawDebugVisuals as one function with booleans instead of two functions?

I wanted to handle it using only one kind of function.
TextureMapper::drawDebugVisuals() and TextureMapperPlatformLayer::drawDebugVisuals().

I agree this is a bit messy. I separate like
TextureMapper::drawDebugBorder() and TextureMapperPlatformLayer::drawDebugBorder().
TextureMapper::drawRepaintCounter() and TextureMapperPlatformLayer::drawRepaintCounter().

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.cpp:38
>> +}
> 
> This can be done in the header file.

I think so. But I encountered build fail due to incomplete declaration of TextureMapper.
I'll try again.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:39
>> +    virtual void drawDebugVisuals(TextureMapper*, const FloatRect&, const TransformationMatrix&, bool showDebugBorders, const Color&, float borderWidth, bool showRepaintCounter, int repaintCount);
> 
> You really need it as a virtual function only for directly composited images; Otherwise you can use the layer transform normally. 
> I think all this virtualization should be removed; We should draw the repaint counters only for the main backing store of the layer.

I need it as a virtual function only for tiled platformLayer: CoordinatedBackingStore and TextureMapperTiledBackingStore.
I agree that only the main backing store of the layer should have repaint counter, but debug border is helpful for canvas, video, directed image and background color.

I need to rethink repaint count, and then I'll submit a patch again.
Comment 29 Dongseong Hwang 2013-01-15 18:19:16 PST
Created attachment 182890 [details]
Patch
Comment 30 Early Warning System Bot 2013-01-15 18:24:44 PST
Comment on attachment 182890 [details]
Patch

Attachment 182890 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15902210
Comment 31 Early Warning System Bot 2013-01-15 18:25:44 PST
Comment on attachment 182890 [details]
Patch

Attachment 182890 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15905181
Comment 32 Dongseong Hwang 2013-01-15 18:27:39 PST
Comment on attachment 182890 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420
> +        incrementRepaintCount();

increase repaint count for a backing store. see CoordinatedGraphicsLayer.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:145
> +            m_backingStore->drawRepaintCounter(options.textureMapper, m_state.repaintCount, m_state.debugBorderColor, layerRect(), transform);

We draw the repaint count for only backing store. It means we draw only border for background color, webgl, composited image and video.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:-386
> -    graphicsLayer->updateDebugIndicators();

This patch removes this line. After this patch, remained dependency on GraphicsLayer is only calling getter of GraphicsLayer.
It is why I want to land this patch before actor model.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:738
> +        m_coordinator->setRepaintCount(id(), incrementRepaintCount());

increase repaint count for a backing store.
Currently we keep repaint count in GraphicsLayer, not each tile.
Final goal would make Tile manage repaint count by itself, but it needs huge refactoring.
So I need to postpone refactoring of this as well as optimization.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2338
> +#if PLATFORM(QT)

It is a bit messy. Is it ok?
Comment 33 Dongseong Hwang 2013-01-15 19:05:45 PST
Created attachment 182903 [details]
Patch
Comment 34 WebKit Review Bot 2013-01-15 19:07:18 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 35 Dongseong Hwang 2013-01-15 19:07:52 PST
(In reply to comment #33)
> Created an attachment (id=182903) [details]
> Patch

Build fix
Don't change WebPage in WK2 not to be WK2 patch. Set preferences in each port file.
Comment 36 Noam Rosenthal 2013-01-15 23:37:13 PST
Comment on attachment 182903 [details]
Patch

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

Getting there :)

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:147
> +    TransformationMatrix adjustedTransform = transform;
> +    adjustedTransform.multiply(TransformationMatrix::rectToRect(rect(), targetRect));

This needs to move to its own helper function.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:143
> +        // Draw the repaint count of only a backing store.

// Only draw repaint count for the main backing store.

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:162
> +    FloatRect rectOnContents(FloatPoint::zero(), m_size);
> +    TransformationMatrix adjustedTransform = transform;
> +    adjustedTransform.multiply(TransformationMatrix::rectToRect(rectOnContents, targetRect));

This needs to move to a helper function; Probably in TextureMapperBackingStore.

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:245
> +void CoordinatedLayerTreeHostProxy::setRepaintCount(CoordinatedLayerID id, int value)

setLayerRepaintCount
Comment 37 Dongseong Hwang 2013-01-16 00:19:09 PST
Created attachment 182926 [details]
Patch
Comment 38 Dongseong Hwang 2013-01-16 00:21:56 PST
(In reply to comment #36)
> (From update of attachment 182903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182903&action=review
> 
> Getting there :)

Wow! Thank you for your review :)

> This needs to move to its own helper function.
> // Only draw repaint count for the main backing store.
> This needs to move to a helper function; Probably in TextureMapperBackingStore.
> setLayerRepaintCount

all done.
Comment 39 Noam Rosenthal 2013-01-16 00:29:30 PST
Comment on attachment 182926 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
> +TransformationMatrix TextureMapperBackingStore::rectToRect(const TransformationMatrix& transform, const FloatRect& from, const FloatRect& to)

I was thinking of calling it adjustedTransformForRect(const FloatRect& targetRect), not pass the "from" rect since it's a member anyway.
And then at the calling site do something like adjustedTransform  = transform * adjustedTransformForRect(targetRect).
Comment 40 Dongseong Hwang 2013-01-16 01:08:36 PST
Created attachment 182933 [details]
Patch
Comment 41 Dongseong Hwang 2013-01-16 01:09:35 PST
(In reply to comment #39)
> (From update of attachment 182926 [details])
> I was thinking of calling it adjustedTransformForRect(const FloatRect& targetRect), not pass the "from" rect since it's a member anyway.
> And then at the calling site do something like adjustedTransform  = transform * adjustedTransformForRect(targetRect).

aha, good idea. done.
Comment 42 Noam Rosenthal 2013-01-17 10:21:29 PST
Comment on attachment 182933 [details]
Patch

Can you separate the change to several patches? It's pretty big; Seems like at least the settings part is rather isolated.
Comment 43 Dongseong Hwang 2013-01-17 16:32:05 PST
Created attachment 183313 [details]
Patch
Comment 44 Dongseong Hwang 2013-01-17 16:33:33 PST
(In reply to comment #42)
> (From update of attachment 182933 [details])
> Can you separate the change to several patches? It's pretty big; Seems like at least the settings part is rather isolated.

I extract two parts.
1. settings in Bug 107198
2. changes of CoordinatedTile in Bug 107196

I'm looking forward review :)
Comment 45 Noam Rosenthal 2013-01-18 05:42:07 PST
Comment on attachment 183313 [details]
Patch

This looks fine to me; Please ask a WebKit2 owner for review.
Comment 46 Benjamin Poulain 2013-01-18 14:14:20 PST
Comment on attachment 183313 [details]
Patch

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

That's a lot of code for a debugging helper! :)

Here are my nitpicks:

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407
> +    if (m_hasOwnBackingStore)
> +        updateDebugBorderAndRepaintCount();
>      m_layer->flushCompositingStateForThisLayerOnly(this);
> -    updateBackingStore();
> +    if (m_hasOwnBackingStore)
> +        updateBackingStore();
>      didFlushCompositingState();

Why did you move the test for m_hasOwnBackingStore out of updateBackingStore() and replace it by an assertion?
This makes the code more fragile and is not explained in the ChangeLog.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430
> +    // The default values for GraphicsLayer debug borders are a little
> +    // hard to see (some less than one pixel wide), so we double their size here.
> +    m_debugBorderColor = color;
> +    m_debugBorderWidth = width * 2;

This is odd. The minimum width of GraphicsLayer::getDebugBorderInfo() is 2. Why do you say some are less than one pixel wide?

What you should do instead of doubling the width here, is override GraphicsLayer::getDebugBorderInfo().

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:315
> +static inline RGBA32 convertABGRToARGB(RGBA32 pixel)
>  {
> +    return ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel & 0xff00ff00);
> +}

This looks like type abuse.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:319
> +    static const int pointSize = 8;

Why is this static?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:336
> -    painter.fillRect(sourceRect, Qt::blue); // Since we won't swap R+B for speed, this will paint red.
> +    painter.fillRect(sourceRect, convertABGRToARGB(color.rgb())); // Since we won't swap R+B for speed, this will paint the given color.

I don't get any of this.
There is Color->QColor implicit conversion. Why use the RGBA32 value?

The comment is not helping.

It also looks like the defaultFormat thingy is premultiplied. While the input color is not. Why?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
> +    virtual void drawBorder(TextureMapper* textureMapper, const Color& color, float borderWidth, const FloatRect& targetRect, const TransformationMatrix& transform)
> +    {
> +        textureMapper->drawBorder(color, borderWidth, targetRect, transform);
> +    }

Virtual functions should not be implemented in headers.
Comment 47 Dongseong Hwang 2013-01-18 19:26:30 PST
(In reply to comment #46)
> (From update of attachment 183313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183313&action=review
> 
> That's a lot of code for a debugging helper! :)
Thank you for review!!

> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407
> > +    if (m_hasOwnBackingStore)
> > +        updateDebugBorderAndRepaintCount();
> >      m_layer->flushCompositingStateForThisLayerOnly(this);
> > -    updateBackingStore();
> > +    if (m_hasOwnBackingStore)
> > +        updateBackingStore();
> >      didFlushCompositingState();
> 
> Why did you move the test for m_hasOwnBackingStore out of updateBackingStore() and replace it by an assertion?
> This makes the code more fragile and is not explained in the ChangeLog.

m_hasOwnBackingStore is WK1 legacy and GraphicsLayerTextureMapper::flushCompositingStateForThisLayerOnly() is important function, so I want to highlight updateDebugBorderAndRepaintCount() and updateBackingStore() are only used in WK1 legacy.
There is no behavior change. I'll explain in the ChangeLog.

> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430
> > +    // The default values for GraphicsLayer debug borders are a little
> > +    // hard to see (some less than one pixel wide), so we double their size here.
> > +    m_debugBorderColor = color;
> > +    m_debugBorderWidth = width * 2;
> 
> This is odd. The minimum width of GraphicsLayer::getDebugBorderInfo() is 2. Why do you say some are less than one pixel wide?
> What you should do instead of doubling the width here, is override GraphicsLayer::getDebugBorderInfo().

I agree on you. It is existing code, not my code. But I correct this: remove comment as well as "*2"

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:315
> > +static inline RGBA32 convertABGRToARGB(RGBA32 pixel)
> >  {
> > +    return ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel & 0xff00ff00);
> > +}
> 
> This looks like type abuse.

Do you have good idea?

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:319
> > +    static const int pointSize = 8;
> 
> Why is this static?

static is not necessary. I think it is a bit faster than allocating stack in every time. I'll remove static.

> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:336
> > -    painter.fillRect(sourceRect, Qt::blue); // Since we won't swap R+B for speed, this will paint red.
> > +    painter.fillRect(sourceRect, convertABGRToARGB(color.rgb())); // Since we won't swap R+B for speed, this will paint the given color.
> 
> I don't get any of this.
> There is Color->QColor implicit conversion. Why use the RGBA32 value?
> 
> The comment is not helping.

This code is hack to upload buffer to texture without swizzling.
the buffer format of qpainter is ARGB but texture format of opengl es is ABGR.
So we consciously draw blue color into qpainter if we want red color. After we upload texture, we can see red color.

> It also looks like the defaultFormat thingy is premultiplied. While the input color is not. Why?
Should Input color be converted to premultiplied when defaultFormat is premultiplied? I did not know that. I'll change.

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
> > +    virtual void drawBorder(TextureMapper* textureMapper, const Color& color, float borderWidth, const FloatRect& targetRect, const TransformationMatrix& transform)
> > +    {
> > +        textureMapper->drawBorder(color, borderWidth, targetRect, transform);
> > +    }
> 
> Virtual functions should not be implemented in headers.

Yes, I know. inline virtual does not make sense. but today compiler well handles it as just virtual. And we don't have TextureMapperPlatformLayer.cpp and we don't need to make TextureMapperPlatformLayer.cpp as noam mentioned in #c26
Comment 48 Dongseong Hwang 2013-01-18 20:03:40 PST
Created attachment 183594 [details]
Patch
Comment 49 Dongseong Hwang 2013-01-18 20:07:23 PST
Comment on attachment 183594 [details]
Patch

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

> Source/WebCore/ChangeLog:33
> +            and updateBackingStore() are only used in WK1.

Add comment.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:430
> +}

Remove "*2"

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:313
> +static inline unsigned convertARGBToABGR(unsigned pixel)

replace RGBA32 to unsigned
rename from convertABGRToARGB to convertARGBToABGR

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:338
> +    painter.fillRect(sourceRect, convertARGBToABGR(premultipliedARGBFromColor(color))); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color.

convert to premultiplied
rephrase comment

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
> +    }

Remain this code not to make TextureMapperPlatformLayer.cpp
Comment 50 Bruno Abinader (history only) 2013-01-21 09:30:37 PST
Since this patch is about a refactory, IMO it would be nice to unify the envvar name (ie. unprefix QT_ from WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS), what do you guys think?
Comment 51 Dongseong Hwang 2013-01-21 14:51:40 PST
(In reply to comment #50)
> Since this patch is about a refactory, IMO it would be nice to unify the envvar name (ie. unprefix QT_ from WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS), what do you guys think?

I agree on you. We can unprefix QT_ in Bug 107198.
Comment 52 Dongseong Hwang 2013-01-22 18:11:39 PST
Benjamin: ping

Could you review this bug? Thank you in advance.
Comment 53 Noam Rosenthal 2013-01-24 00:04:18 PST
Comment on attachment 183594 [details]
Patch

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

I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information.

I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message.

>> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:40
>> +    }
> 
> Remain this code not to make TextureMapperPlatformLayer.cpp

I think it's OK if the default implementation did nothing. The tiled backing store itself should deal with drawing the border.
Comment 54 Dongseong Hwang 2013-01-24 16:45:12 PST
(In reply to comment #53)
> (From update of attachment 183594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review
> 
> I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information.

Ok, I agree.

> I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message.

Yes, right direction. Many bugs wait this bug. I'll separate soon!

> I think it's OK if the default implementation did nothing. The tiled backing store itself should deal with drawing the border.

I don't think it is good idea for tiled backing store to draw the border. I'm sure it is one of solutions.
But I feel that "Compositor" has responsibility to draw border. Currently, TBS does not care of border. I'm wonder if it is necessary for TBS to deal with border. I want only compositor to deal with border.

In addition, it is not efficient for TBS to draw borders. For example, if users toggle "showDebugBorder" via web inspector, we must update whole tiles.
On the other hand, it is cheap for TexMap to draw border.
Comment 55 Dongseong Hwang 2013-01-24 17:10:08 PST
(In reply to comment #53)
> (From update of attachment 183594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183594&action=review
> 
> I think this is an overkill for repaint counting. I don't want to add extra IPC messages for every repaint just for a debug feature. We already know of a repaint in the UI process, when UpdateTile is called, and we should use that information.
> I think we should review the WK1 part separately (it looks quite OK to me), and then in WK2 call GraphicsLayerTextureMapper->incrementRepaintCount() when we get a createTile/updateTile message.

After rethinking, it is not good for WK2 to use GraphicsLayerTextureMapper->incrementRepaintCount(). It conflicts against removing GraphicsLayerTextureMapper from WK2.
In the short term, I need to make new overkill message to use [Coordinated]GraphicsLayer::incrementRepaintCount().

In the long term, we will get rid of code related to dealing with repaint count in GraphicsLayer. As we know, each tile should deal with each repaint count because each tile has different repaint count.
At that time, we will remove the overkill message.
Comment 56 Dongseong Hwang 2013-01-24 21:28:13 PST
Created attachment 184656 [details]
Patch
Comment 57 Dongseong Hwang 2013-01-24 21:38:03 PST
Comment on attachment 184656 [details]
Patch

This patch can be compiled and run well without Bug 107910 (separated WK2 part).

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

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:350
> +    painter.fillRect(sourceRect, convertARGBToABGR(color.rgb())); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color.

It is correct to send unmultiplied color.
Comment 58 Noam Rosenthal 2013-01-24 23:51:05 PST
Comment on attachment 184656 [details]
Patch

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

Almost there.

> Source/WebCore/ChangeLog:17
> +        2. We use the same color and width to Mac port because we get both info using

both info -> that info

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:406
> +    if (m_hasOwnBackingStore)
> +        updateDebugBorderAndRepaintCount();

I think you should always call updateDebugBorderAndRepaintCountIfNeeded(), and return early if it doesn't have own backing store.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:409
> +    if (m_hasOwnBackingStore)
> +        updateBackingStore();

Ditto (updateBackingStoreIfNeeded)

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:98
> +

Remove line

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:45
> +        ASSERT_NOT_REACHED();

I don't think it's necessary to have an ASSERT like this to a debug feature.

>> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:350
>> +    painter.fillRect(sourceRect, convertARGBToABGR(color.rgb())); // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color.
> 
> It is correct to send unmultiplied color.

I would prefer to call
Color::createUnchecked(color.blue(), color.green(), color.red())
Comment 59 Noam Rosenthal 2013-01-25 01:38:13 PST
Comment on attachment 184656 [details]
Patch

See comments
Comment 60 Dongseong Hwang 2013-01-25 01:48:32 PST
Created attachment 184700 [details]
Patch
Comment 61 Dongseong Hwang 2013-01-25 01:49:40 PST
(In reply to comment #58)
> (From update of attachment 184656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184656&action=review

Thank you for review! all Done!
Comment 62 Dongseong Hwang 2013-01-25 04:50:51 PST
Thank you!

Could you cq+? I don't have a committer authority yet.
Comment 63 WebKit Review Bot 2013-01-25 05:52:27 PST
Comment on attachment 184700 [details]
Patch

Clearing flags on attachment: 184700

Committed r140821: <http://trac.webkit.org/changeset/140821>
Comment 64 WebKit Review Bot 2013-01-25 05:52:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Zoltan Arvai 2013-01-28 05:08:14 PST
After the patch committed in r140821 it seems the Qt5-WK1 and Qt5-WK2 layout test results are different with a

(usingTiledLayer 1)

http://build.webkit.org/results/Qt%20Linux%20Release/r140821%20%2856761%29/results.html

Can you help me find out what can cause this difference, please?
Comment 66 Dongseong Hwang 2013-01-28 14:00:18 PST
(In reply to comment #65)
> After the patch committed in r140821 it seems the Qt5-WK1 and Qt5-WK2 layout test results are different with a
> 
> (usingTiledLayer 1)
> 
> http://build.webkit.org/results/Qt%20Linux%20Release/r140821%20%2856761%29/results.html
> 
> Can you help me find out what can cause this difference, please?

Thanks for reporting. I missed it.

In this patch, I set GraphicsLayer::m_usingTiledLayer to 1 when the layer has tiled backing.
So Qt1 layout test results were changed.

But Bug 107910 will apply the same thing to WK2, so now WK1 and WK2 results are different.

Are you ok if I update test results after Bug 107910 landing?
Comment 67 Dongseong Hwang 2013-01-28 16:51:47 PST
Reopening to attach new patch.
Comment 68 Dongseong Hwang 2013-01-28 16:51:59 PST
Created attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.
Comment 69 Dongseong Hwang 2013-01-28 16:54:01 PST
Comment on attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.

After re-surveying, only mac uses usingTiledLayer, so default expected results don't expect (usingTiledLayer 1).
So we (EFL GTK QT) don't set usingTiledLayer to true.
Comment 70 Noam Rosenthal 2013-01-28 21:57:27 PST
Comment on attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.

Since we use tiled layers for everything, I think we should simply rebase our tests.
Comment 71 Noam Rosenthal 2013-01-28 22:01:00 PST
(In reply to comment #70)
> (From update of attachment 185105 [details])
> Since we use tiled layers for everything, I think we should simply rebase our tests.

btw why do we set m_usingTiledLayer to true in the first place?
Comment 72 Dongseong Hwang 2013-01-28 22:14:44 PST
Comment on attachment 185105 [details]
Patch : It is a follow-up patch not to change layout test results.

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:420
> +        m_usingTiledLayer = false;

(In reply to comment #71)
> (In reply to comment #70)
> > (From update of attachment 185105 [details] [details])
> > Since we use tiled layers for everything, I think we should simply rebase our tests.

Yes, we can just rebase, but I'm afraid that it would be burden for gardening Qt and EFL port.

As I mentioned, default expected txt does not expect (usingTiledLayer 1).
For example, LayoutTest/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt does not include (usingTiledLayer 1).
If we use m_usingTiledLayer, we must manage LayoutTest/platform/qt/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt like LayoutTests/platform/mac/compositing/tiling/rotated-tiled-preserve3d-clamped-expected.txt
I think other maintainers don't like it.

> btw why do we set m_usingTiledLayer to true in the first place?

updateDebugIndicators() return different color for a tiled backing. I want to match the color of texture mapper to safari.

I agree that my solution is a bit weird. I want to listen your opinion. :)
Comment 73 Noam Rosenthal 2013-01-28 22:17:05 PST
> > btw why do we set m_usingTiledLayer to true in the first place?
> 
> updateDebugIndicators() return different color for a tiled backing. I want to match the color of texture mapper to safari.
> 
> I agree that my solution is a bit weird. I want to listen your opinion. :)
Remove the useTiling flag for now. We can mess with the debug colors to match Safari in a different patch.
Comment 74 Dongseong Hwang 2013-01-28 22:19:20 PST
(In reply to comment #73)
> Remove the useTiling flag for now. We can mess with the debug colors to match Safari in a different patch.

ok. so simple and succinct solution :)
Comment 75 Dongseong Hwang 2013-01-28 22:27:58 PST
Created attachment 185156 [details]
Patch : It is a follow-up patch not to change layout test results.
Comment 76 Noam Rosenthal 2013-01-28 22:37:21 PST
Comment on attachment 185156 [details]
Patch : It is a follow-up patch not to change layout test results.

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

> Source/WebCore/ChangeLog:10
> +        The previous patch set GraphicsLayer::m_usingTiledLayer to true when using a

set -> sets

> Source/WebCore/ChangeLog:18
> +        No new tests. Covered by existing tests.

Debug feature, not covered in tests.
Comment 77 Dongseong Hwang 2013-01-28 22:44:49 PST
Created attachment 185163 [details]
Patch for landing : It is a follow-up patch not to change layout test results.
Comment 78 WebKit Review Bot 2013-01-28 23:50:19 PST
Comment on attachment 185163 [details]
Patch for landing : It is a follow-up patch not to change layout test results.

Clearing flags on attachment: 185163

Committed r141067: <http://trac.webkit.org/changeset/141067>
Comment 79 WebKit Review Bot 2013-01-28 23:50:30 PST
All reviewed patches have been landed.  Closing bug.