Bug 105978

Created attachment 181120 [details] WIP Patch

Comment on attachment 181120 [details] WIP Patch Post WIP patch to clarify the direction. I'm not sure yet this code is correct in each platform specific code: webview, pageClient, pageViewportControllerClient. And I need more test. I worked based on kenneth's xmaxpatch :)

Comment on attachment 181120 [details] WIP Patch Attachment 181120 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15639622

Comment on attachment 181120 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181120&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:114 > + WebKit::WebPageProxy* page() const { return m_pageProxy.get(); } this method returns mutable pointer, so should not be const

(In reply to comment #0) > Curruntly, Coordinated Graphics manages a duplicated scale. Remove code related to a duplicated scale. > > There are two major changes. > 1. Don't keep scale. Instead query the scale from WebPage[Proxy]. > 2. CoordinatedLayerTreeHost::SetVisibleContentsRect message does not send a scale anymore. Instead reuse IPC communication of WebPage, and callback mechanism of GraphicsLayer. > > FYI, note what is effectiveScaleFactor. > effectiveScaleFactor is pageScaleFactor * deviceScaleFactor. > platform webview and pageViewportController know both pageScaleFactor and deviceScaleFactor. > But CoordinatedGraphicsLayer and TiledBackingStore need to know only effectiveScaleFactor. > CoordinatedGraphicsLayer would snap pixel alignment using effectiveScaleFactor, and TiledBackingStore paints contents using effectiveScaleFactor. > > However, GraphicsLayerCA snaps pixel alignment using pageScaleFactor. I think it is a potential bug, but apple uses only 2 as a deviceScaleFactor, so this potential bug does not appear yet. > In conclusion, CoordinatedGraphicsLayer and TiledBackingStore need to know final scale for device, and it is effectiveScaleFactor. I think this patch should be aligned with the work I do for Qt on retina display. Since the coordinates in Qt are always logical and the device scale factor is applied on the backing store and during painting in QtWebPageSGNode, the device scale factor logic in PageViewportController is redundant and will be removed. My patch for that also still needs testing, and I have to align the change with the way EFL is handling dpxr. I'm back from vacation on monday, please do not land this before next week.

Comment on attachment 181120 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181120&action=review > Source/WebKit2/ChangeLog:8 > + Curruntly, Coordinated Graphics manages a duplicated scale. Remove code about a Spelling. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:507 > + return page()->pageScaleFactor() * page()->deviceScaleFactor(); Are you sure 'page()->pageScaleFactor()' is initialized properly here? > Source/WebKit2/UIProcess/PageViewportController.cpp:50 > + , m_effectiveScaleFactor(1) maybe renaming worth creating a separate patch? > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:55 > + virtual void setContentsScale(float) { } could this method be removed?

(In reply to comment #6) > (From update of attachment 181120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181120&action=review Thank Mikhail for review. I rewrite the second WIP patch. I will apply your review :) (In reply to comment #5) > (In reply to comment #0) > I think this patch should be aligned with the work I do for Qt on retina display. What a coincidence! great. > Since the coordinates in Qt are always logical and the device scale factor is applied on the backing store and during painting in QtWebPageSGNode, the device scale factor logic in PageViewportController is redundant and will be removed. Do you means TiledBackingStore must know both pageScale and deviceScale? However TileCache in Apple knows only effectiveScale ( = pageScale * deviceScale). And I think PageViewportController needs to know effectiveScale because PageViewportController must adjust scroll position to avoid noncompositing layer blurring. > My patch for that also still needs testing, and I have to align the change with the way EFL is handling dpxr. > > I'm back from vacation on monday, please do not land this before next week. Ok, I wait for you. I found a lot of problems as made this bug. I need to cowork and discuss in depth. :) As I mentioned, I encountered many problems. I list some of them. 1. delegating scroll : we sometimes change scrollPosition of ScrollView (i.e. select text using mouse) and sometimes ignore (most cases). However, changing scrollPosition of WebCore does not make anything except for calling PageDidRequestScroll callback. I think we will remove scroll delegating so that WebCore draws scroll bar and we use Scrolling thread. Am I right? Page::setPageScaleFactor(float scale, const IntPoint& origin) requires scroll position, but different scroll position arguments do not make difference on Coordinated Graphics. 2. page scale : Currently we never change page scale when using Minibrowser, but we can change via WK2 API. CoordiantedGraphicsLayer and TiledBackingStore seems to assume pageScale is always 1. We need to change CoordiantedGraphicsLayer and TiledBackingStore to get along with pageScale in WebCore. 3. There are complex and slow scroll position and scale communication routine related to PageViewportController. For example, we need 4 times IPC communication to run wheel event. UI Process Message Web Process WebPageProxy (wheel ->) WebPage PageViewportController (<- PageDidRequestScroll) WebPage PageViewportController (setVisibleContentsRect ->) CoordinatedLayerTree PageViewportController (<- didRenderFrame) CoordinatedLayerTree LayerTreeRenderer::paint Scale handling ditto. We implemented PageViewportController runs drawing after receiving callback from Web Process. We need to change.

> 2. page scale : Currently we never change page scale when using Minibrowser, but we can change via WK2 API. CoordiantedGraphicsLayer and TiledBackingStore seems to assume pageScale is always 1. We need to change CoordiantedGraphicsLayer and TiledBackingStore to get along with pageScale in WebCore. > https://bugs.webkit.org/show_bug.cgi?id=104374 is targeted to solve it. However the solution would also require fix of the current bug.

(In reply to comment #8) > https://bugs.webkit.org/show_bug.cgi?id=104374 is targeted to solve it. > However the solution would also require fix of the current bug. Aha, this bug was already created! I think both are duplicated.

*** Bug 104374 has been marked as a duplicate of this bug. ***

Created attachment 181873 [details] WIP Patch2

Comment on attachment 181873 [details] WIP Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=181873&action=review This is the first working patch, that means the first patch work abnormally :) I post this patch to explain the direction. We need prerequisite refactoring before this patch. 1. Decide whether transform include deviceScale as well as pageScale. 2. The size of WebPage (a.k.a nonCompositingLayer) is on viewport coordinated system, not contents coordinated system although the size of all GrahpicsLayer is on contents coordinated system. TODO: As I mentioned, sending scroll and scall to Web Process is not efficient. We need to refactor further. > Source/WebCore/css/StyleResolver.cpp:1456 > +// documentStyle->setPageScaleTransform(frame ? frame->frameScaleFactor() : 1); If we don't comment out, coordinated graphics draws with pageScale^2. It means if pageScale is 2, we can see 4 times scaled contents. In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale. IMO current implementation is ambiguous. > Source/WebKit2/UIProcess/PageViewportController.h:112 > + float m_contentsScale; // Should always be cssScale * devicePixelRatio. Define m_contentsScale that is cssScale * devicePixelRatio because GraphicsLayerCA and TiledBackingStore also use the contentsScale term.

Comment on attachment 181873 [details] WIP Patch2 Attachment 181873 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15763457 New failing tests: fast/frames/frame-set-scaling-hit.html fast/dom/window-scroll-scaling.html compositing/layer-creation/fixed-position-out-of-view-scaled.html fast/dom/Range/scale-page-client-rects.html fast/dom/elementFromPoint-scaled-scrolled.html inspector/elements/highlight-node-scaled.html fast/frames/frame-set-rotation-hit.html compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html fast/dom/Element/scale-page-bounding-client-rect.html fast/dom/Element/scale-page-client-rects.html fast/events/page-scaled-mouse-click-iframe.html fast/dom/Range/scale-page-bounding-client-rect.html fast/transforms/selection-bounds-in-transformed-view.html svg/as-image/image-respects-pageScaleFactor.html compositing/tiling/tile-cache-zoomed.html

(In reply to comment #12) > In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale. Can you explain this a bit more - I don't really understand the comment. What I do know is that it's beneficial for the graphics context to know the scale factor.

(In reply to comment #14) > (In reply to comment #12) > > > In addition, if we really want transform includes scale info, we should make transform include deviceRatioFactor and we should make GraphicsLayer not know about scale. > > Can you explain this a bit more - I don't really understand the comment. What I do know is that it's beneficial for the graphics context to know the scale factor. That's good question! I should have explained well. Sorry for lazy comment. GraphicsLayer should use final effective scale to draw contents on the device screen. The effective scale is page scale * device scale. Current WebCore slightly apply page scale to local transform of GraphicsLayer. But RenderLayerBacking notifies page scale and device scale to GraphicsLayer already. I think it is a bit confused that local transform include page scale. We must remember page scale and local transform are not orthogonal. And if we really want local transform to include scale info, local transform should include device scale also. I prefer local transform not to include scale info. In addition, Page::pageScale() method causes style recalc. IMO it is only for applying page scale to local transform. If it is true, the style recalc can be avoidable as we don't change local transform. And I don't fully understand why Page::pageScale() causes layout yet. Now, changing page scale causes layout after this patch. I'm not sure whether it is acceptable or not.

Are you going to update this patch?

Created attachment 184193 [details] Patch

After this patch, we need to reset the results of many layout tests because we now change a page scale factor in WebCore. I tested pinch zoom using QtMiniBrowser and device scale factor using -r option of EFLMiniBrowser. I think this patch is fine. However, I'm sure this patch makes new bugs that are not identified yet. We need to fix them gradually.

Comment on attachment 184193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184193&action=review > Source/WebKit2/ChangeLog:11 > + Currently, PageViewportController sends a page scale factor to Coordinated > + Graphics System regardless of the page scale factor in WebCore. This patch makes > + Coordinated Graphics System use the page scale factor in WebCore to match other > + ports. Exactly what we want, to be more inline with the other ports > Source/WebKit2/ChangeLog:15 > + WebCore is changed, CoordinatedGraphicsLayer can be notified via gets? instead of can be > Source/WebKit2/ChangeLog:23 > + We set true to applyDeviceScaleFactorInCompositor and > + ApplyPageScaleFactorInCompositor in Settings like chromium, because > + TiledBackingStore that is a backing store of each GraphicsLayer applies the > + scale to our raster graphics engines instead of applying the scale to the local > + transform of each render object. that sounds right. Then you dont need to relayout after changing scale > Source/WebKit2/ChangeLog:43 > + * UIProcess/API/efl/EwkViewImpl.cpp: > + (EwkViewImpl::EwkViewImpl): > + * UIProcess/API/qt/qquickwebview.cpp: > + (QQuickWebViewPrivate::initialize): > + (QQuickWebViewLegacyPrivate::updateViewportSize): > + * UIProcess/API/qt/raw/qrawwebview.cpp: > + (QRawWebView::setSize): > + * UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp: > + (WebKit::CoordinatedLayerTreeHostProxy::CoordinatedLayerTreeHostProxy): > + (WebKit::CoordinatedLayerTreeHostProxy::setVisibleContentsRect): > + * UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h: > + (CoordinatedLayerTreeHostProxy): > + * UIProcess/DrawingAreaProxy.h: > + (WebKit::DrawingAreaProxy::setVisibleContentsRect): > + * UIProcess/DrawingAreaProxyImpl.cpp: a bit of detailed info here would be useful > Source/WebKit2/UIProcess/PageViewportController.cpp:202 > + // FIXME : the contents position does not affect anything when delegates scrolling is enabled. > + m_webPageProxy->scalePage(m_pageScaleFactor, roundedIntPoint(m_contentsPosition)); Why is that a FIXME? Should it? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218 > > +#if USE(ACCELERATED_COMPOSITING) > + if (m_drawingArea->layerTreeHost()) > + m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged(); > +#endif smfr should probably look at this change > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1254 > +#if USE(ACCELERATED_COMPOSITING) > + if (m_drawingArea->layerTreeHost()) > + m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged(); > +#endif diddo > Source/WebKit2/WebProcess/WebPage/mac/LayerTreeHostMac.mm:165 > -void LayerTreeHostMac::deviceScaleFactorDidChange() > +void LayerTreeHostCA::deviceOrPageScaleFactorChanged() diddo

Comment on attachment 184193 [details] Patch Attachment 184193 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16080219

Comment on attachment 184193 [details] Patch Attachment 184193 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16063687

Comment on attachment 184193 [details] Patch Attachment 184193 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16065585

Comment on attachment 184193 [details] Patch Attachment 184193 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16067468

(In reply to comment #19) > (From update of attachment 184193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184193&action=review Thank you for review! > > Source/WebKit2/ChangeLog:15 > > + WebCore is changed, CoordinatedGraphicsLayer can be notified via > > gets? instead of can be yes > > Source/WebKit2/ChangeLog:23 > > + We set true to applyDeviceScaleFactorInCompositor and > > + ApplyPageScaleFactorInCompositor in Settings like chromium, because > > + TiledBackingStore that is a backing store of each GraphicsLayer applies the > > + scale to our raster graphics engines instead of applying the scale to the local > > + transform of each render object. > > that sounds right. Then you dont need to relayout after changing scale But we need to change Page::setPageScaleFactor to no relayout. I think putting "if (ApplyPageScaleFactorInCompositor)" in relevant place. > > Source/WebKit2/ChangeLog:43 > > + (WebKit::DrawingAreaProxy::setVisibleContentsRect): > > + * UIProcess/DrawingAreaProxyImpl.cpp: > > a bit of detailed info here would be useful I'll update. > > > Source/WebKit2/UIProcess/PageViewportController.cpp:202 > > + // FIXME : the contents position does not affect anything when delegates scrolling is enabled. > > + m_webPageProxy->scalePage(m_pageScaleFactor, roundedIntPoint(m_contentsPosition)); > > Why is that a FIXME? Should it? I'll recomment without FIXME. The comment is a bit irrelevant. What I really want to say is that Page::setPageScaleFactor is somewhat weird. I don't understand we really need updateLayout in Page::setPageScaleFactor when delegatesScrolling is enabled. void Page::setPageScaleFactor(float scale, const IntPoint& origin) { ... if (scale == m_pageScaleFactor) { if (view && (view->scrollPosition() != origin || view->delegatesScrolling())) { document->updateLayoutIgnorePendingStylesheets(); view->setScrollPosition(origin); } return; } ... } > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1218 > > > > +#if USE(ACCELERATED_COMPOSITING) > > + if (m_drawingArea->layerTreeHost()) > > + m_drawingArea->layerTreeHost()->deviceOrPageScaleFactorChanged(); > > +#endif > > smfr should probably look at this change I'll separate this part in another bug so that smfr review conveniently :)

Build fail due to Bug 107742. I fixed in Bug 107760

Created attachment 184438 [details] Patch

Comment on attachment 184438 [details] Patch Attachment 184438 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16076681

Created attachment 184445 [details] Patch

Created attachment 185449 [details] Patch

Comment on attachment 185449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1276 > m_page->settings()->setFixedPositionCreatesStackingContext(fixed); > + m_page->settings()->setApplyDeviceScaleFactorInCompositor(fixed); > + m_page->settings()->setApplyPageScaleFactorInCompositor(fixed); > #endif I tihnk we are already setting these when creating our EwkView (or we should be)

(In reply to comment #30) > (From update of attachment 185449 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1276 > > m_page->settings()->setFixedPositionCreatesStackingContext(fixed); > > + m_page->settings()->setApplyDeviceScaleFactorInCompositor(fixed); > > + m_page->settings()->setApplyPageScaleFactorInCompositor(fixed); > > #endif > > I tihnk we are already setting these when creating our EwkView (or we should be) we can not set in UI process because these api are not open via WebPreference.

LGTM, please have a WebKit2 owner review

(In reply to comment #32) > LGTM, please have a WebKit2 owner review LGTM as well. A step forward towards having less home-brew code around...

Comment on attachment 185449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:201 > + // FIXME : Page::setPageScaleFactor performs layout even if the scale is not changed. Does that layout actually do any work though?

(In reply to comment #34) > (From update of attachment 185449 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185449&action=review > > > Source/WebKit2/UIProcess/PageViewportController.cpp:201 > > + // FIXME : Page::setPageScaleFactor performs layout even if the scale is not changed. > > Does that layout actually do any work though? Thank you for review! after r141053 (Bug 107424) the comment became stale. I mean that there is no layout with applyPageScaleInCompositor. I'll get rid of the comment and the cq+.

Created attachment 185556 [details] Patch for landing

Thank mikhail for cq+ :)

Comment on attachment 185556 [details] Patch for landing Clearing flags on attachment: 185556 Committed r141310: <http://trac.webkit.org/changeset/141310>

All reviewed patches have been landed. Closing bug.