Bug 132857 – [iOS] Page scale update messages for media controls should only fire at the end of zooming

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

[iOS] Page scale update messages for media controls should only fire at the end of zooming

[Dean Jackson]

Created attachment 231370 [details]
Patch

[Benjamin Poulain]

Comment on attachment 231370 [details]
Patch

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

I'll have a look.

My first take is: I don't understand why would this be related to the pinch gesture. Why wouldn't you update the media control on programmatic zoom (e.g. double tap) and scale changes originating from the WebProcess (e.g. Content Width changes, and is rescaled to fit into view).

> Source/WebCore/page/Page.cpp:788
> +            frame->document()->pageScaleFactorChanged();

The method pageScaleFactorChanged() should probably be renamed to something specific to media, and to a name that make it clear that it can be out of sync with the page scale factor.

As it is with the patch, it counter intuitive that you would call pageScaleFactorChanged() only when a user triggered zoom ends.

It is also unclear why would one call pageScaleFactorChanged() on every document when only the scale factor of the main frame changed.

[Dean Jackson]

(In reply to comment #2)
> (From update of attachment 231370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231370&action=review
> 
> I'll have a look.
> 
> My first take is: I don't understand why would this be related to the pinch gesture. Why wouldn't you update the media control on programmatic zoom (e.g. double tap) and scale changes originating from the WebProcess (e.g. Content Width changes, and is rescaled to fit into view).

Yeah, that's a mistake. This should happen any time zoom changes. The idea is just to stop it happening during a pinch... so I should move the functionality back into the setPageScaleFactor method.

> 
> > Source/WebCore/page/Page.cpp:788
> > +            frame->document()->pageScaleFactorChanged();
> 
> The method pageScaleFactorChanged() should probably be renamed to something specific to media, and to a name that make it clear that it can be out of sync with the page scale factor.

I think I should do half of that. I think I should remove the guards around the code that mention MEDIA, because this could really apply to anything.

Then, yeah, I need to think of a better name.... pageScaleFactorCommitted?

> 
> As it is with the patch, it counter intuitive that you would call pageScaleFactorChanged() only when a user triggered zoom ends.
> 
> It is also unclear why would one call pageScaleFactorChanged() on every document when only the scale factor of the main frame changed.

[Dean Jackson]

Created attachment 231407 [details]
Patch

[Dean Jackson]

From Ben's comments:

- renamed the method to make it clear it's only about media controls
- call the "commit change for media" method during page scale changes unless we're in the middle of a zoom (covering double tap and other things)
- only calling the method on the main frame

[Simon Fraser (smfr)]

Comment on attachment 231407 [details]
Patch

VisibleContentRectUpdateInfo already has inStableState(); you should use this instead of plumbing through more ways to know if a zoom is in progress.

[Benjamin Poulain]

(In reply to comment #6)
> (From update of attachment 231407 [details])
> VisibleContentRectUpdateInfo already has inStableState(); you should use this instead of plumbing through more ways to know if a zoom is in progress.

The problem with that is if you start pinching, then lift a single finger and continue scrolling.

That is not a stable update, yet we should update anything that depends on a stable scale factor.

At the moment, WKWebView does not differentiate. It seems Dean will need to break stable state in two: "stableState" and "scaleFactorIsStable".

[Dean Jackson]

Created attachment 231413 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231413 [details]
Patch

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.

[Dean Jackson]

Created attachment 231416 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 231416 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2162
> +    bool toldPageThatScaleHasChanged = false;

Could be just hasSetPageScale I think

[Dean Jackson]

Committed r168761: <http://trac.webkit.org/changeset/168761>