Bug 103290 – Ensure that WebPageProxy's notion of pageScaleFactor is reset on didCommitLoad

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Tim Horton]

When a standard main frame load is committed, we reset the WebPage's pageScaleFactor, if it's not the default. However, if the previous page had a full-main-frame plugin that was handling page scaling itself, WebPageProxy can have a cached pageScaleFactor != 1, while WebPage now (since the plugin that was overriding (set)pageScaleFactor is gone) has a pageScaleFactor of 1, causing us to skip the call to reset the WebPage's pageScaleFactor. We need to notify WebPageProxy of the change in this case.

[Tim Horton]

This causes pinch-to-zooming in PDFPlugin to break initial pinch-to-zooming in the next page loaded.

[Tim Horton]

<rdar://problem/12752467>

[Tim Horton]

Created attachment 176077 [details]
patch

[Tim Horton]

Comment on attachment 176077 [details]
patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:471
> +        float pageScaleFactor = webPage->pageScaleFactor();

Don't need this temporary, and it should be a double anyway.

[Darin Adler]

Comment on attachment 176077 [details]
patch

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

Can you make a test case?

I’m OK with this change, but it seems to be an indirect way to solve the problem, and I suspect there’s a better way.

> Source/WebKit2/ChangeLog:19
> +        instead of reaching into WebCore immediately.

immediately -> directly

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:479
> +            webPage->send(Messages::WebPageProxy::PageScaleFactorDidChange(1));

Is it really appropriate to send this message after every single load? These redundant messages don’t seem to be conveying information. Can’t the other process determine this without having to receive a cross-process message? There must be a better way to solve this. It seems bogus to just send extra scale factor did change messages when the scale factor did not change.

[Tim Horton]

Hmm, you're right, there's probably somewhere that the UIProcess can do this itself already.

A test case might be tricky, but I'll try (I'd like to get more of this PDFPlugin stuff tested, but that's difficult for obvious reasons).

[Tim Horton]

Created attachment 176446 [details]
patch

How about this instead?

[Tim Horton]

I don't think there's any way to test this at the moment (without building a plugin that takes over page scale that can be built on all platforms, etc. etc.).

[Eric Seidel (no email)]

Comment on attachment 176077 [details]
patch

Cleared Darin Adler's review+ from obsolete attachment 176077 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Darin Adler]

Comment on attachment 176446 [details]
patch

Seems a bit inelegant to send the load type across, but I don’t have an immediate better idea and this is internal to the code so it doesn’t affect API or anything like that.

[Tim Horton]

http://trac.webkit.org/changeset/139341