[webkit-reviews] review denied: [Bug 132857] [iOS] Page scale update messages for media controls should only fire at the end of zooming : [Attachment 231413] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dean Jackson
<dino at apple.com>'s request for review:
Bug 132857: [iOS] Page scale update messages for media controls should only
fire at the end of zooming
https://bugs.webkit.org/show_bug.cgi?id=132857

Attachment 231413: Patch
https://bugs.webkit.org/attachment.cgi?id=231413&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231413&action=review


> Source/WebCore/dom/Document.h:1045
> -    void pageScaleFactorChanged();
> +    void pageScaleFactorCommittedForMedia();

I would prefer this not have "ForMedia" in the name. We may need to hook other
things up to stable scale changes. Maybe pageScaleFactorChangeToStableValue or
something. gotStableScaleChange?

> Source/WebCore/page/Page.cpp:765
> -    for (Frame* frame = &mainFrame(); frame; frame =
frame->tree().traverseNext())
> -	   frame->document()->pageScaleFactorChanged();
> +    if (inStableState)
> +	   mainFrame().document()->pageScaleFactorCommittedForMedia();

You've lost going through subframes in this patch.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2162
> +    m_page->setPageScaleFactor(floatBoundedScale, scrollPosition,
visibleContentRectUpdateInfo.inStableState());

This changes means that we'll be changing the scale factor without checking
floatBoundedScale != m_page->pageScaleFactor(), which I suspect will cause some
bad things to happen (maybe when pinching?)

I think you should only call m_page->setPageScaleFactor() here if the state is
stable.