WK2 does this already. WK1 should match WK2 here and update the page visibility when the WebView is hidden / visible.
Created attachment 183899 [details] [PATCH] Update visibility state on window state changes I'll think about ways to test this. Maybe I can test this via TestWebKitAPI and minimize / restore the window containing the WebView. I'm open to suggestions.
Comment on attachment 183899 [details] [PATCH] Update visibility state on window state changes Attachment 183899 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16010918
Comment on attachment 183899 [details] [PATCH] Update visibility state on window state changes Attachment 183899 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16038485
So this fails on the mac bots because it requires a patch that hasn't landed yet (it just got reviewed). <http://webkit.org/b/107230> [Mac] Enable Page Visibility (PAGE_VISIBILITY_API)
Comment on attachment 183899 [details] [PATCH] Update visibility state on window state changes I have a better patch in the works with a test for WK1 and WK2. This also requires a patch from: <http://webkit.org/b/107364> Improve PageVisibility API with enums
Created attachment 184355 [details] [PATCH] Now With WK1 + WK2 Tests Still waiting on earlier patches to be reviewed for this to build on mac.
Comment on attachment 184355 [details] [PATCH] Now With WK1 + WK2 Tests View in context: https://bugs.webkit.org/attachment.cgi?id=184355&action=review So I guess when you switch tabs, we still have to programmatically change page visibility? What if someone move the view between windows? Let say I minimize Window 1, and some asshat move the view to Window 2? > Source/WebKit/mac/WebView/WebView.mm:3691 > +- (void)_windowDidMiniaturize:(NSNotification *)notification > +{ > + [self _updateVisibilityState]; > +} > + > +- (void)_windowDidDeminiaturize:(NSNotification *)notification > +{ > + [self _updateVisibilityState]; > +} > + > +- (void)_windowDidOrderOnScreen:(NSNotification *)notification > +{ > + [self _updateVisibilityState]; > +} > + > +- (void)_windowDidOrderOffScreen:(NSNotification *)notification > +{ > + [self _updateVisibilityState]; > +} Why not just make - (void)_windowVisibilityChanged: And register all notifications to that? > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1076 > + A57A34F016AF677200C2501F /* PageVisibilityStateWithWindowChanges.mm in Sources */, Build section could be sorted (my new favorite trick to r- patches) :) > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:34 > +static bool didGetPageSignal; The name is a little generic. > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:68 > + virtual NSURL *url() const { return [[NSBundle mainBundle] URLForResource:@"PageVisibilityStateWithWindowChanges" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]; } > + virtual void didLoadURL(WebView *webView) { runTest(webView); teardownView(webView); } > + virtual void didLoadURL(WKView *wkView) { runTest(wkView); teardownView(wkView); } Not a fan of inline implementation here. Missing OVERRIDE? > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:74 > + virtual void initializeView(WebView *) OVERRIDE; > + virtual void initializeView(WKView *) OVERRIDE; > + virtual void teardownView(WebView *); > + virtual void teardownView(WKView *); Style? Missing OVERRIDE? > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:79 > + webView.UIDelegate = [[PageVisibilityStateDelegate alloc] init]; This looks like a leak. > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:86 > + id uiDelegate = webView.UIDelegate; > + webView.UIDelegate = nil; > + [uiDelegate release]; ahahaha, okay. Maybe a RetainPtr as an attribute would makes things simpler?
(In reply to comment #7) > (From update of attachment 184355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184355&action=review > > So I guess when you switch tabs, we still have to programmatically change page visibility? Correct, that is a browser level feature, not a WebView level feature. > What if someone move the view between windows? Let say I minimize Window 1, and some asshat move the view to Window 2? When the WebView's superview / window hierarchy changes it will always get -[WebView viewDidMoveToWindow], which will update visibilityState. > > Source/WebKit/mac/WebView/WebView.mm:3691 > > +- (void)_windowDidMiniaturize:(NSNotification *)notification > > +{ > > + [self _updateVisibilityState]; > > +} > > + > > +- (void)_windowDidDeminiaturize:(NSNotification *)notification > > +{ > > + [self _updateVisibilityState]; > > +} > > + > > +- (void)_windowDidOrderOnScreen:(NSNotification *)notification > > +{ > > + [self _updateVisibilityState]; > > +} > > + > > +- (void)_windowDidOrderOffScreen:(NSNotification *)notification > > +{ > > + [self _updateVisibilityState]; > > +} > > Why not just make > - (void)_windowVisibilityChanged: > And register all notifications to that? I thought of that, but I wanted to be as explicit as possible. I'll make this change. > > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1076 > > + A57A34F016AF677200C2501F /* PageVisibilityStateWithWindowChanges.mm in Sources */, > > Build section could be sorted (my new favorite trick to r- patches) :) > > > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:34 > > +static bool didGetPageSignal; > > The name is a little generic. > > > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:68 > > + virtual NSURL *url() const { return [[NSBundle mainBundle] URLForResource:@"PageVisibilityStateWithWindowChanges" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]; } > > + virtual void didLoadURL(WebView *webView) { runTest(webView); teardownView(webView); } > > + virtual void didLoadURL(WKView *wkView) { runTest(wkView); teardownView(wkView); } > > Not a fan of inline implementation here. > Missing OVERRIDE? Inline is the convention here. All other didLoadURL implementations are inlined. I'll add OVERRIDe. > > > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:74 > > + virtual void initializeView(WebView *) OVERRIDE; > > + virtual void initializeView(WKView *) OVERRIDE; > > + virtual void teardownView(WebView *); > > + virtual void teardownView(WKView *); > > Style? Missing OVERRIDE? Nothing to override. This is new. I could upstream an empty impl to WebKitTestAgnostic and call it appropriately. Maybe "deinitializeView". > > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:79 > > + webView.UIDelegate = [[PageVisibilityStateDelegate alloc] init]; > > This looks like a leak. > > > Tools/TestWebKitAPI/Tests/mac/PageVisibilityStateWithWindowChanges.mm:86 > > + id uiDelegate = webView.UIDelegate; > > + webView.UIDelegate = nil; > > + [uiDelegate release]; > > ahahaha, okay. > Maybe a RetainPtr as an attribute would makes things simpler? This is exactly why I couldn't use RetainPtr and added teardownWebView. Because the WebView does not retain the delegate, I had to come up with something hacky. I could add a comment.
Created attachment 184636 [details] [PATCH] Addressed Review Comments
Comment on attachment 184636 [details] [PATCH] Addressed Review Comments View in context: https://bugs.webkit.org/attachment.cgi?id=184636&action=review > Tools/TestWebKitAPI/mac/WebKitAgnosticTest.h:62 > + virtual void deinitializeView(WebView *) { } > + virtual void deinitializeView(WKView *) { } I'm not a fan of this made up word. I would prefer cleanupView().
(In reply to comment #10) > (From update of attachment 184636 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184636&action=review > > > Tools/TestWebKitAPI/mac/WebKitAgnosticTest.h:62 > > + virtual void deinitializeView(WebView *) { } > > + virtual void deinitializeView(WKView *) { } > > I'm not a fan of this made up word. I would prefer cleanupView(). As discussed on IRC we went with teardownView since tests normally have setup/teardown naming conventions.
Landed r141011: <http://trac.webkit.org/changeset/141011> In my commit message I forgot to update OOPs to Sam. =( But the ChangeLogs are correct.