Bug 71595 - [chromium] Add draw-time scale delta to compositor thread
Summary: [chromium] Add draw-time scale delta to compositor thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-04 16:58 PDT by Alexandre Elias
Modified: 2011-11-09 15:48 PST (History)
8 users (show)

See Also:


Attachments
Patch (46.12 KB, patch)
2011-11-04 16:59 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (46.33 KB, patch)
2011-11-04 17:52 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (46.52 KB, patch)
2011-11-06 14:32 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (50.03 KB, patch)
2011-11-07 21:36 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (50.40 KB, patch)
2011-11-08 14:22 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (42.30 KB, application/zip)
2011-11-08 16:07 PST, WebKit Review Bot
no flags Details
Patch (51.43 KB, patch)
2011-11-08 16:15 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (42.29 KB, application/zip)
2011-11-08 18:07 PST, WebKit Review Bot
no flags Details
Patch (51.44 KB, patch)
2011-11-09 09:06 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (54.25 KB, patch)
2011-11-09 11:33 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (42.34 KB, application/zip)
2011-11-09 12:17 PST, WebKit Review Bot
no flags Details
Patch (54.42 KB, patch)
2011-11-09 14:37 PST, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2011-11-04 16:58:56 PDT
[chromium] Add draw-time scale delta to compositor thread
Comment 1 Alexandre Elias 2011-11-04 16:59:55 PDT
Created attachment 113735 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-04 17:02:17 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Alexandre Elias 2011-11-04 17:05:27 PDT
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.
Comment 4 Alexandre Elias 2011-11-04 17:52:34 PDT
Created attachment 113742 [details]
Patch
Comment 5 Alexandre Elias 2011-11-04 17:53:56 PDT
Fixed bug with scrollbar maxScrollPosition (needed a clampNegativeToZero).
Comment 6 Darin Fisher (:fishd, Google) 2011-11-05 15:01:52 PDT
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?
Comment 7 Alexandre Elias 2011-11-06 14:32:46 PST
Created attachment 113803 [details]
Patch
Comment 8 Alexandre Elias 2011-11-06 14:33:53 PST
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.
Comment 9 Adrienne Walker 2011-11-07 12:28:45 PST
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?
Comment 10 James Robinson 2011-11-07 14:27:10 PST
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
Comment 11 Darin Fisher (:fishd, Google) 2011-11-07 15:26:37 PST
Comment on attachment 113803 [details]
Patch

WebKit API changes LGTM
Comment 12 Alexandre Elias 2011-11-07 21:36:49 PST
Created attachment 113985 [details]
Patch
Comment 13 Alexandre Elias 2011-11-07 21:43:45 PST
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.
Comment 14 Alexandre Elias 2011-11-08 14:22:04 PST
Created attachment 114153 [details]
Patch
Comment 15 James Robinson 2011-11-08 15:27:43 PST
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?
Comment 16 WebKit Review Bot 2011-11-08 16:07:55 PST
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
Comment 17 WebKit Review Bot 2011-11-08 16:07:59 PST
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
Comment 18 Alexandre Elias 2011-11-08 16:15:56 PST
Created attachment 114170 [details]
Patch
Comment 19 Alexandre Elias 2011-11-08 16:17:55 PST
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.
Comment 20 James Robinson 2011-11-08 17:11:04 PST
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.
Comment 21 WebKit Review Bot 2011-11-08 18:07:54 PST
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
Comment 22 WebKit Review Bot 2011-11-08 18:07:58 PST
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
Comment 23 James Robinson 2011-11-08 21:35:44 PST
Comment on attachment 114170 [details]
Patch

cr-linux EWS failure seems unrelated, can't repro locally.
Comment 24 WebKit Review Bot 2011-11-08 21:38:30 PST
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
Comment 25 James Robinson 2011-11-08 21:42:32 PST
Ah, the string you want is "NOBODY (OOPS!)."
Comment 26 Alexandre Elias 2011-11-09 09:06:33 PST
Created attachment 114299 [details]
Patch
Comment 27 Alexandre Elias 2011-11-09 09:19:31 PST
Ah, oops :).  Switched to "NOBODY (OOPS!)" and changed nothing else.
Comment 28 WebKit Review Bot 2011-11-09 10:03:18 PST
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
Comment 29 Alexandre Elias 2011-11-09 11:33:49 PST
Created attachment 114327 [details]
Patch
Comment 30 Alexandre Elias 2011-11-09 11:35:33 PST
That was an error hidden in a DEBUG-build #ifdef.  Fixed and checked that there aren't any others.
Comment 31 WebKit Review Bot 2011-11-09 12:17:07 PST
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
Comment 32 WebKit Review Bot 2011-11-09 12:17:11 PST
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
Comment 33 Alexandre Elias 2011-11-09 14:37:00 PST
Created attachment 114371 [details]
Patch
Comment 34 Alexandre Elias 2011-11-09 14:38:02 PST
Fixed updateLayerTreeViewport NULL pointer access causing the layout test failure.
Comment 35 WebKit Review Bot 2011-11-09 15:48:01 PST
Comment on attachment 114371 [details]
Patch

Clearing flags on attachment: 114371

Committed r99774: <http://trac.webkit.org/changeset/99774>
Comment 36 WebKit Review Bot 2011-11-09 15:48:08 PST
All reviewed patches have been landed.  Closing bug.