Bug 71595 – [chromium] Add draw-time scale delta to compositor thread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Add draw-time scale delta to compositor thread

[Alexandre Elias]

Created attachment 113735 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Alexandre Elias]

Patch description:

Add draw-time scale delta to compositor thread.

- There are two magnify values:
1. The WebKit-side pageScale value corresponding to the resolution of
   the painted content.
2. The Impl-side-only scaleDelta value representing the additional
   degree of current magnification, applied as a draw transform.  WebKit is
   never explicitly aware of this value and the only way to change it
   currently is via pinch events (and in the future, double-tap/etc
   events).  At the end of a pinch zooms, we multiply pageScale *
   scaleDelta and tell WebKit to use it as the new pageScale.

- Scroll offsets and max-extents are maintained in pageScale space.
  When the scale changes, we need to take care to convert them to the
  new scale and avoid getting them clamped to the extents at the wrong
  scale.

- Remove maxScrollPosition on the non-impl side because it makes no
  sense to precompute it in the presence of an impl-side zoom.

- zoomAnimatorTransform is not used because it currently isn't in the
  branch, and mostly doesn't behave the way we want anyway (it does most
  things in WebKit thread).  We will need to unify these later.

- Also introduce deviceOrPageScaleFactorChanged() call which invalidates
  layers.

[Alexandre Elias]

Created attachment 113742 [details]
Patch

[Alexandre Elias]

Fixed bug with scrollbar maxScrollPosition (needed a clampNegativeToZero).

[Darin Fisher (:fishd, Google)]

Comment on attachment 113742 [details]
Patch

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

> Source/WebKit/chromium/public/WebLayerTreeViewClient.h:44
> +    virtual void applyScrollAndScale(const WebSize&, float) = 0;

nit: please give the float parameter a name.  i think "scaleFactor" would work.
the WebSize parameter could probably also benefit from a name:  "scrollDelta"

> Source/WebKit/chromium/public/WebView.h:212
> +    virtual void setScaleLimits(float minScale, float maxScale) = 0;

since this impacts "PageScaleFactor", it seems like the function name should include
that term.  how about calling this function setPageScaleFactorLimits?

[Alexandre Elias]

Created attachment 113803 [details]
Patch

[Alexandre Elias]

Comment on attachment 113742 [details]
Patch

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

>> Source/WebKit/chromium/public/WebLayerTreeViewClient.h:44
>> +    virtual void applyScrollAndScale(const WebSize&, float) = 0;
> 
> nit: please give the float parameter a name.  i think "scaleFactor" would work.
> the WebSize parameter could probably also benefit from a name:  "scrollDelta"

Done.

>> Source/WebKit/chromium/public/WebView.h:212
>> +    virtual void setScaleLimits(float minScale, float maxScale) = 0;
> 
> since this impacts "PageScaleFactor", it seems like the function name should include
> that term.  how about calling this function setPageScaleFactorLimits?

Sounds good, renamed everywhere.

[Adrienne Walker]

Comment on attachment 113803 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:214
> +    if (firstResize || m_pageScaleDirty)

Can you explain why the full-layer invalidation needs to happen in setBounds?

Also, I'm a little confused why you need to do this invalidation yourself.  Shouldn't WebKit invalidate everything when the scale changes? And, if it somehow doesn't, why not just call LayerChromium::setNeedsDisplay() instead of calling deviceOrPageScaleFactorChanged?

[James Robinson]

Comment on attachment 113803 [details]
Patch

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

Compositor stuff here looks great.  I left a few nits.

It looks like the bots weren't able to apply this patch to ToT - could you merge it up and post a new patch so the bots can double-check it?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:101
> +    void setPageMagnifyTransform(const TransformationMatrix& t) { m_pageMagnifyMatrix = t; }

we don't typically use one-letter variable names in WebKit even for dumb stuff like this

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:74
> +    bool pageScaleChanged = m_graphicsLayer->pageScaleFactor() != pageScale;
> +    if (pageScaleChanged)

i think the bool here is kind of unnecessary, just inline the check into the if()

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:219
> +    float m_minScale, m_maxScale;

nit: could you call these m_minPageScale / m_maxPageScale?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:175
>      bool changed = viewportSize != m_viewportSize;
>      m_viewportSize = viewportSize;
> -    if (changed)
> +    if (changed) {
> +        updateMaxScrollPosition();
>          m_layerRenderer->viewportChanged();

this function would be a little more idomatic if it had an early-return at the top if m_viewportSize == viewportSize and the rest was unguarded. i think it's structured this way for historical reasons (there used to be a lot more going on in here) that aren't relevant any more

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:208
> +    bool changed = delta != m_scaleDelta;
> +    m_scaleDelta = delta;
> +
> +    if (changed) {

also here - early return would be more in line with the rest of the code

[Darin Fisher (:fishd, Google)]

Comment on attachment 113803 [details]
Patch

WebKit API changes LGTM

[Alexandre Elias]

Created attachment 113985 [details]
Patch

[Alexandre Elias]

OK, all style nits applied.  I also synced up the client.  Note that this change conflicted with enne's clipping fix because I removed m_maxScrollPosition from the non-impl side.  I needed to introduce a "m_scrollable" boolean to allow the root layer to be discovered, and obtained the content bounds from a child layer.

(In reply to comment #9)
> (From update of attachment 113803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113803&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:214
> > +    if (firstResize || m_pageScaleDirty)
> 
> Can you explain why the full-layer invalidation needs to happen in setBounds?
> 
> Also, I'm a little confused why you need to do this invalidation yourself.  Shouldn't WebKit invalidate everything when the scale changes? And, if it somehow doesn't, why not just call LayerChromium::setNeedsDisplay() instead of calling deviceOrPageScaleFactorChanged?

It's because the invalidation is clipped to the bounds, and the bounds are multiplied by the scale, so we need to defer it until after they're changed.  There may be a cleaner way to do this, but it doesn't seem like too bad of a hack for now.

[Alexandre Elias]

Created attachment 114153 [details]
Patch

[James Robinson]

Comment on attachment 114153 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by James Robinson.

getting a bit ahead of yourself there I think :)

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:135
> +    bool scrollable() const { return m_scrollable; }
> +    void setScrollable(bool scrollable) { m_scrollable = true; }

can you update LayerChromiumTest.cpp's checkPropertyChangeCausesCorrectBehavior test for this property?

[WebKit Review Bot]

Comment on attachment 114153 [details]
Patch

Attachment 114153 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10374228

New failing tests:
accessibility/aria-checkbox-checked.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/animation-direction.html
canvas/philip/tests/2d.canvas.readonly.html
accessibility/aria-checkbox-text.html
http/tests/appcache/cyrillic-uri.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-add-events-in-handler.html
animations/animation-controller-drt-api.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.canvas.reference.html
accessibility/aria-describedby-on-input.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-direction-normal.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html

[WebKit Review Bot]

Created attachment 114169 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandre Elias]

Created attachment 114170 [details]
Patch

[Alexandre Elias]

Comment on attachment 114153 [details]
Patch

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

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by James Robinson.
> 
> getting a bit ahead of yourself there I think :)

Sorry, don't know the process, reverted.

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:135
>> +    void setScrollable(bool scrollable) { m_scrollable = true; }
> 
> can you update LayerChromiumTest.cpp's checkPropertyChangeCausesCorrectBehavior test for this property?

Done.

[James Robinson]

Comment on attachment 114170 [details]
Patch

R=me - holding off on cq for now to see if the cr-linux EWS bot goes red again.

[WebKit Review Bot]

Comment on attachment 114170 [details]
Patch

Attachment 114170 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10372099

New failing tests:
accessibility/aria-checkbox-checked.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/animation-direction.html
canvas/philip/tests/2d.canvas.readonly.html
accessibility/aria-checkbox-text.html
http/tests/appcache/cyrillic-uri.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-add-events-in-handler.html
animations/animation-controller-drt-api.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.canvas.reference.html
accessibility/aria-describedby-on-input.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-direction-normal.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html

[WebKit Review Bot]

Created attachment 114187 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Comment on attachment 114170 [details]
Patch

cr-linux EWS failure seems unrelated, can't repro locally.

[WebKit Review Bot]

Comment on attachment 114170 [details]
Patch

Rejecting attachment 114170 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

NOBODY found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10373124

[James Robinson]

Ah, the string you want is "NOBODY (OOPS!)."

[Alexandre Elias]

Created attachment 114299 [details]
Patch

[Alexandre Elias]

Ah, oops :).  Switched to "NOBODY (OOPS!)" and changed nothing else.

[WebKit Review Bot]

Comment on attachment 114299 [details]
Patch

Rejecting attachment 114299 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
bkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
tform/graphics/chromium/cc/CCSingleThreadProxy.cpp: In member function 'void WebCore::CCSingleThreadProxy::doCommit()':
Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:170: error: 'struct WebCore::CCScrollAndScaleSet' has no member named 'size'
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10370499

[Alexandre Elias]

Created attachment 114327 [details]
Patch

[Alexandre Elias]

That was an error hidden in a DEBUG-build #ifdef.  Fixed and checked that there aren't any others.

[WebKit Review Bot]

Comment on attachment 114327 [details]
Patch

Attachment 114327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10395047

New failing tests:
accessibility/aria-checkbox-checked.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/auth.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/animation-direction.html
canvas/philip/tests/2d.canvas.readonly.html
accessibility/aria-checkbox-text.html
http/tests/appcache/cyrillic-uri.html
http/tests/appcache/credential-url.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-add-events-in-handler.html
animations/animation-controller-drt-api.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.canvas.reference.html
accessibility/aria-describedby-on-input.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-direction-normal.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html

[WebKit Review Bot]

Created attachment 114339 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandre Elias]

Created attachment 114371 [details]
Patch

[Alexandre Elias]

Fixed updateLayerTreeViewport NULL pointer access causing the layout test failure.

[WebKit Review Bot]

Comment on attachment 114371 [details]
Patch

Clearing flags on attachment: 114371

Committed r99774: <http://trac.webkit.org/changeset/99774>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.