Bug 110727

Summary: [chromium] Remove WebLayerTreeView::setViewportSize call
Product: WebKit Reporter: Alexandre Elias <aelias>
Component: New BugsAssignee: Alexandre Elias <aelias>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, buildbot, dglazkov, jamesr, jochen, piman, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Alexandre Elias 2013-02-24 23:29:23 PST
[chromium] Remove WebLayerTreeView::setViewportSize call
Comment 1 Alexandre Elias 2013-02-24 23:33:33 PST
Created attachment 190001 [details]
Patch
Comment 2 Alexandre Elias 2013-02-24 23:44:40 PST
https://codereview.chromium.org/12328080/ will land before this one.
Comment 3 Build Bot 2013-02-25 00:30:24 PST
Comment on attachment 190001 [details]
Patch

Attachment 190001 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16752009

New failing tests:
svg/as-image/img-preserveAspectRatio-support-2.html
Comment 4 WebKit Review Bot 2013-02-25 01:04:05 PST
Comment on attachment 190001 [details]
Patch

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

New failing tests:
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/nested-direct-image-compositing.html
compositing/direct-image-compositing.html
compositing/backface-visibility/backface-visibility-non3d.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/backface-visibility/backface-visibility-3d.html
compositing/clip-change.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/fixed-position-changed-in-composited-layer.html
compositing/self-painting-layers.html
compositing/text-on-scaled-layer.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/flat-with-transformed-child.html
compositing/scrollbar-painting.html
compositing/preserve-3d-toggle.html
compositing/fixed-position-scroll-offset-history-restore.html
compositing/backface-visibility/backface-visibility-image.html
compositing/compositing-visible-descendant.html
compositing/fixed-position-changed-within-composited-parent-layer.html
compositing/backface-visibility/backface-visibility-simple.html
compositing/backface-visibility/backface-visibility-webgl.html
compositing/text-on-scaled-surface.html
Comment 5 James Robinson 2013-02-25 18:02:15 PST
Comment on attachment 190001 [details]
Patch

This part looks fine, but you'll have to sort out the issues on the chromium-side patch and make sure that lands *and* rolls into Source/WebKit/chromium/DEPS before this part can land.
Comment 6 Alexandre Elias 2013-02-27 20:34:41 PST
Created attachment 190641 [details]
Patch

Make DumpRenderTree call setViewportSize
Comment 7 James Robinson 2013-02-27 20:51:21 PST
Comment on attachment 190641 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:940
> +    m_layerTreeView->setViewportSize(webWidget()->size(), deviceViewportSize);

Hmm, this is a bit of a bummer.  any way to avoid this?
Comment 8 Alexandre Elias 2013-02-27 21:09:12 PST
No way to avoid it comes to mind, since my patches are moving the responsibility to call this to the embedder.  DumpRenderTree is another embedder so it needs a call.

I could've left the responsibility to call it in WebKit, in which case it would need to be informed of the physical backing size via an API change.  But it doesn't need to know about this for anything else, and it's nice and clean for every coordinate given to WebKit to be in DIP pixels.
Comment 9 James Robinson 2013-02-28 11:00:27 PST
(In reply to comment #8)
> No way to avoid it comes to mind, since my patches are moving the responsibility to call this to the embedder.  DumpRenderTree is another embedder so it needs a call.
> 
> I could've left the responsibility to call it in WebKit, in which case it would need to be informed of the physical backing size via an API change.  But it doesn't need to know about this for anything else, and it's nice and clean for every coordinate given to WebKit to be in DIP pixels.

Does the viewport ever change in DRT?  If the answer is no (which I think it is) then we should pass this out as a construction-time parameter.  We shouldn't have exposed API in Platform that's only called from DRT
Comment 10 Alexandre Elias 2013-02-28 20:04:33 PST
I see, you prefer it only exposed in webkit_support.h.  Unfortunately, some layout tests do call window.resize and break if I try doing setting only at construction time, such as compositing/transitions/transform-on-large-layer.html
Comment 11 James Robinson 2013-02-28 22:07:17 PST
Yeah, I'd like to get rid of it from WebLayerTreeView completely if we don't want WebKit to be setting this generally.  But don't let that hold you up here.  We can fix it up later for DRT or just wait until DRT goes D-E-D.
Comment 12 Alexandre Elias 2013-03-03 14:12:57 PST
Comment on attachment 190641 [details]
Patch

Source/WebKit/chromium/DEPS rolled past 185621, landing this half.
Comment 13 WebKit Review Bot 2013-03-03 14:17:48 PST
Comment on attachment 190641 [details]
Patch

Clearing flags on attachment: 190641

Committed r144581: <http://trac.webkit.org/changeset/144581>
Comment 14 WebKit Review Bot 2013-03-03 14:17:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 jochen 2013-03-03 23:16:33 PST
This change regressed >500 tests on content_shell
Comment 16 Alexandre Elias 2013-03-03 23:32:47 PST
I have a follow-up fix https://codereview.chromium.org/12374078/ that should hopefully fix them.
Comment 17 Alexandre Elias 2013-03-03 23:51:15 PST
I confirmed https://codereview.chromium.org/12374078/ fixed the content_shell failures.
Comment 18 jochen 2013-03-03 23:58:53 PST
(In reply to comment #17)
> I confirmed https://codereview.chromium.org/12374078/ fixed the content_shell failures.

Thank you!