Bug 145493 - PDFs always think they're visible on iOS.
Summary: PDFs always think they're visible on iOS.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-30 14:16 PDT by Gavin Barraclough
Modified: 2015-06-02 15:03 PDT (History)
3 users (show)

See Also:


Attachments
Fix (3.89 KB, patch)
2015-05-30 14:26 PDT, Gavin Barraclough
kling: review+
Details | Formatted Diff | Diff
With fix for WebKitTestRunner (4.51 KB, patch)
2015-06-02 14:56 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2015-05-30 14:16:29 PDT
The problem here is that WKContentView is currently responsible for notifying the WebPageProxy that the visibility may have changed, but when a PDF isn't showing the WKContentView isn't in the view hierarchy and doesn't receive the didMoveToWindow notifications – and the WKPDFView (which is in the view hierarchy) does not listen for these events.

Visibility of the page should really just track the visibility of the WKWebView (and when actually assessing the visibility it largely does - the page client checks the web view's visibility, bar a FIXME, and the foreground/background check, which needs to change). So notifications should really just come from the WKWebView.

The WKWebView already listens for the didMoveToWindow notification, it just was only updating the IsInWindow flag. Instead just update all flags, and the call to viewStateDidChange from WKContentView can just be removed.

There is one problem with this in that it would reverse the order of the calls to viewStateDidChange & _updateForScreen:, which would mean the the view would become visible before updating the screen pixel density. To fix this, moving the call to _updateForScreen: to willMoveToWindow:, to ensure it occurs before the page becomes visible.

This will also change behavior on Mac for WKWebView clients, in coalescing all view state changes within the didMoveToWindow call. This is the direction we intended to go in anyway (the plan is to remove the mayHaveChanged argument from viewStateDidChange - we're currently adding unnecessary IPC traffic).
Comment 1 Gavin Barraclough 2015-05-30 14:26:52 PDT
Created attachment 253964 [details]
Fix
Comment 2 Andreas Kling 2015-05-30 20:59:01 PDT
Comment on attachment 253964 [details]
Fix

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

r=me

> Source/WebKit2/ChangeLog:25
> +        updating the screen pixel density. To fix this, moving the call to _updateForScreen:

s/moving/move/
Comment 3 Gavin Barraclough 2015-05-31 16:17:39 PDT
Committed revision 185046.
Comment 4 Daniel Bates 2015-06-01 10:56:02 PDT
(In reply to comment #3)
> Committed revision 185046.

This change causes many animation tests to time out on iOS WebKit2.
Comment 5 Gavin Barraclough 2015-06-01 11:41:48 PDT
Temporary roll-out in r185068...
Comment 6 Gavin Barraclough 2015-06-02 14:56:50 PDT
Created attachment 254107 [details]
With fix for WebKitTestRunner
Comment 7 Gavin Barraclough 2015-06-02 15:02:13 PDT
Comment on attachment 254107 [details]
With fix for WebKitTestRunner

Reviewed by Sam.
Comment 8 Gavin Barraclough 2015-06-02 15:03:09 PDT
Committed revision 185127.