Bug 107509

Summary: [Mac] Update PageVisibilityState when WebView is hidden / visible
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, buildbot, cmuppala, ddkilzer, joepeck, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Update visibility state on window state changes
joepeck: review-, joepeck: commit-queue-
[PATCH] Now With WK1 + WK2 Tests
joepeck: commit-queue-
[PATCH] Addressed Review Comments sam: review+

Description Joseph Pecoraro 2013-01-21 23:28:17 PST
WK2 does this already. WK1 should match WK2 here and update the page visibility when the WebView is hidden / visible.
Comment 1 Joseph Pecoraro 2013-01-21 23:33:03 PST
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 2 Build Bot 2013-01-22 02:43:03 PST
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 3 Build Bot 2013-01-22 03:44:54 PST
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
Comment 4 Joseph Pecoraro 2013-01-22 10:14:39 PST
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 5 Joseph Pecoraro 2013-01-23 16:39:32 PST
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
Comment 6 Joseph Pecoraro 2013-01-23 17:00:39 PST
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 7 Benjamin Poulain 2013-01-24 14:22:36 PST
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?
Comment 8 Joseph Pecoraro 2013-01-24 14:50:03 PST
(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.
Comment 9 Joseph Pecoraro 2013-01-24 18:49:49 PST
Created attachment 184636 [details]
[PATCH] Addressed Review Comments
Comment 10 Sam Weinig 2013-01-26 11:53:09 PST
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().
Comment 11 Joseph Pecoraro 2013-01-28 14:46:12 PST
(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.
Comment 12 Joseph Pecoraro 2013-01-28 14:47:02 PST
Landed r141011: <http://trac.webkit.org/changeset/141011>

In my commit message I forgot to update OOPs to Sam. =( But the ChangeLogs are correct.