Bug 113326

Created attachment 195117 [details] Patch

Comment on attachment 195117 [details] Patch I'm ok with the coordinated graphics changes. Can you ask Kenneth/Jocelyn to also look before asking for a WK2 owner sign off?

(In reply to comment #2) > (From update of attachment 195117 [details]) > I'm ok with the coordinated graphics changes. Can you ask Kenneth/Jocelyn to also look before asking for a WK2 owner sign off? Thanks, Jocelyn could you take a look?

Comment on attachment 195117 [details] Patch Attachment 195117 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17236651

(In reply to comment #4) > (From update of attachment 195117 [details]) > Attachment 195117 [details] did not pass efl-ews (efl): > Output: http://webkit-commit-queue.appspot.com/results/17236651 Hmm, EFL seems to depend on the wrong behaviour. I'll have to check what's going on there.

Created attachment 195282 [details] Patch

(In reply to comment #6) > Created an attachment (id=195282) [details] > Patch Fixed the EFL build.

Comment on attachment 195282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195282&action=review LGTM but here is something that would be nice to verify twice: > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:140 > + m_drawingAreaProxy->page()->scalePage(scale, roundedIntPoint(rect.location())); > m_drawingAreaProxy->page()->process()->send(Messages::CoordinatedLayerTreeHost::SetVisibleContentsRect(rect, trajectoryVector), m_drawingAreaProxy->page()->pageID()); This might still create the issue that the scale change might trigger a layer flush with a visibleRect's size that is not matching the scale. For example, if you're zommed out and your page coord visible rect is 500x500 with a scale of 0.5 (a window size of 250x250), zooming in would trigger a tile update using a visibleRect of 125x125 with a scale of 2 (again 250x250). If the scale change does trigger a layer flush before the matching visibleRect update message is handled, you'll end up rendering a 500x500 rect with a scale of 2: 1000x1000 for the viewport. 2000x2000 if you take into account the TiledBackingStore 2x cover rect multiplier. Could you verify with some printf in TiledBackingStore::createTiles that the tilesToCreateCount isn't growing astronomically when zooming in? We might not hit that case by luck since a flush is scheduled back on the event loop, giving time to the visible rect update message to be handled, but I'm not sure.

The PageViewportController::didChangeContentsVisibility call still in the PageViewportControllerClientEfl::setViewportPosition PVC callback could cause problems with this patch. So hopefully somebody will be testing zooming with EFL.

(In reply to comment #9) > The PageViewportController::didChangeContentsVisibility call still in the PageViewportControllerClientEfl::setViewportPosition PVC callback could cause problems with this patch. So hopefully somebody will be testing zooming with EFL. Qt also has that call indirectlz through the contentX and contentY change notifications, but EFL needs to call it directly. I did not see any issues there but it would be good if someone could verify.

(In reply to comment #8) > (From update of attachment 195282 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195282&action=review > > LGTM but here is something that would be nice to verify twice: > > > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:140 > > + m_drawingAreaProxy->page()->scalePage(scale, roundedIntPoint(rect.location())); > > m_drawingAreaProxy->page()->process()->send(Messages::CoordinatedLayerTreeHost::SetVisibleContentsRect(rect, trajectoryVector), m_drawingAreaProxy->page()->pageID()); > > This might still create the issue that the scale change might trigger a layer flush with a visibleRect's size that is not matching the scale. > For example, if you're zommed out and your page coord visible rect is 500x500 with a scale of 0.5 (a window size of 250x250), zooming in would trigger a tile update using a visibleRect of 125x125 with a scale of 2 (again 250x250). > > If the scale change does trigger a layer flush before the matching visibleRect update message is handled, you'll end up rendering a 500x500 rect with a scale of 2: 1000x1000 for the viewport. 2000x2000 if you take into account the TiledBackingStore 2x cover rect multiplier. > > Could you verify with some printf in TiledBackingStore::createTiles that the tilesToCreateCount isn't growing astronomically when zooming in? We might not hit that case by luck since a flush is scheduled back on the event loop, giving time to the visible rect update message to be handled, but I'm not sure. I couldn't produce such a scenario. The tile creation always started after the visibleRect update was processed but never in between the scale setting and the visible rect update. I do not have a concept of astronomical in this context but the biggest number of tilesToCreateCount in TiledBackingStore::createTiles was 10 during double-tap zoom-in.

Comment on attachment 195282 [details] Patch LGTM for Qt, I'm still not sure for EFL so I think it would be best to get their blessing too.

(In reply to comment #11) > I couldn't produce such a scenario. The tile creation always started after the visibleRect update was processed but never in between the scale setting and the visible rect update. > > I do not have a concept of astronomical in this context but the biggest number of tilesToCreateCount in TiledBackingStore::createTiles was 10 during double-tap zoom-in. Ok in any case the patch is an improvement in that regard, thanks for checking.

(In reply to comment #12) > (From update of attachment 195282 [details]) > LGTM for Qt, I'm still not sure for EFL so I think it would be best to get their blessing too. Kenneth, could you check this for EFL with one of your magic manual tests? I ran a quick test with an EFL build and did not see suspicious behaviour.

Ping for review?

Created attachment 196645 [details] Patch

(In reply to comment #16) > Created an attachment (id=196645) [details] > Patch Updated to apply on ToT.

Comment on attachment 196645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196645&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:45 > + , m_lastSentScale(0) ah another scale :( but isn't it the same as pagescalefactor from page proxy?

Comment on attachment 196645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196645&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:45 >> + , m_lastSentScale(0) > > ah another scale :( but isn't it the same as pagescalefactor from page proxy? It is technically the same scale factor, but the represented state is different. In theory I could put a scale and scroll position equality check to WebPageProxy::scalePage, but WebPageProxy::m_pageScaleFactor has to be initialized to 1, since it is exposed, so an initial scale setting of 1 would not reach the web process causing behavior change and potential problems. I also think that the name makes the state this variable represents clear, but I could rename it to something like m_lastCommittedPageScaleFactor, if you think it confusing.

Created attachment 197573 [details] Patch

Comment on attachment 197573 [details] Patch r=me, just make sure you get an WK2 owner to look at it. (CC'd Benjamin)

Comment on attachment 197573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197573&action=review I have two little issues with this patch: I think the method is badly named. Something named "setVisibleContentsRect" should take a rect, not a rect, a scale and a "trajectoryVector" (I assume normalized). You should either split it in three method calls, or look for a more descriptive name. The scale of 1 passed for the page scale factor is really weird. I don't really understand why that would be correct. For example, a page has a scale of 0.3, the user resize the view, shouldn't the scale be re-computed for the viewport parameters? > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:102 > + float m_lastSentPageScaleFactor; float->double (the page scale factor is typed double).

(In reply to comment #22) > (From update of attachment 197573 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197573&action=review > > I have two little issues with this patch: > > I think the method is badly named. > Something named "setVisibleContentsRect" should take a rect, not a rect, a scale and a "trajectoryVector" (I assume normalized). It is not normalized at this point yet. Normalization happens in TiledBackingStore::setTrajectoryVector. > You should either split it in three method calls, or look for a more descriptive name. I'll try to come up with a more descriptive name. > > The scale of 1 passed for the page scale factor is really weird. I don't really understand why that would be correct. For example, a page has a scale of 0.3, the user resize the view, shouldn't the scale be re-computed for the viewport parameters? > We only set the scale to 1 for the "legacy" webview (no fixed layout, i.e. traditional desktop behaviour as well as the QRawWebView). For the QML WebView (with fixed layout) we recalculate each time something changes (see PageViewportController::syncVisibleContents). > > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:102 > > + float m_lastSentPageScaleFactor; > > float->double (the page scale factor is typed double). Thanks for catching this.

Created attachment 199021 [details] Patch

Comment on attachment 199021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199021&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:131 > +void CoordinatedLayerTreeHostProxy::updateVisibleContentsState(const FloatRect& visibleContetsRect, double pageScaleFactor, const FloatPoint& trajectoryVector) visibleConte*n*tsRect You could also name the method updateContentsVisibility to match PageViewportController::didChangeContentsVisibility

Created attachment 200707 [details] Patch

Comment on attachment 200707 [details] Patch Attachment 200707 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/431034

Created attachment 200710 [details] Patch

Comment on attachment 200710 [details] Patch lgtm. Benjamin, could you check again?

Created attachment 203331 [details] Patch

(In reply to comment #30) > Created an attachment (id=203331) [details] > Patch Updated to ToT.

Comment on attachment 203331 [details] Patch Attachment 203331 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/673339

Created attachment 203336 [details] Patch

Comment on attachment 203336 [details] Patch Qt has been removed, clearing review flags.

I don't see the flicker currently in qt5x2 branch. Is this patch still needed and should we apply it to branch?

=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.