|Summary:||[chromium] Remove WebLayerTreeView::setViewportSize call|
|Product:||WebKit||Reporter:||Alexandre Elias <aelias>|
|Component:||New Bugs||Assignee:||Alexandre Elias <aelias>|
|Severity:||Normal||CC:||aelias, buildbot, dglazkov, jamesr, jochen, piman, rniwa, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Alexandre Elias 2013-02-24 23:29:23 PST
[chromium] Remove WebLayerTreeView::setViewportSize call
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.