Bug 107539 – [ca] Tell CA to clean up unused resources if a given WebProcess won't be drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

<rdar://problem/13032692>

[Tim Horton]

Created attachment 184074 [details]
patch (I'm going to test one more time)

[WebKit Review Bot]

Attachment 184074 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebProcess.cpp:1106:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 184074 [details]
patch (I'm going to test one more time)

Disregard, it's not having the intended effect. I'll try again.

[Tim Horton]

Created attachment 194960 [details]
patch

[WebKit Review Bot]

Attachment 194960 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/platform/mac/WebCoreSystemInterface.h', u'Source/WebCore/platform/mac/WebCoreSystemInterface.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebSystemInterface.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebSystemInterface.mm', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebProcess.cpp', u'Source/WebKit2/WebProcess/WebProcess.h', u'WebKitLibraries/libWebKitSystemInterfaceLion.a', u'WebKitLibraries/libWebKitSystemInterfaceMountainLion.a']" exit_code: 1
Source/WebKit2/WebProcess/WebProcess.cpp:1121:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 194960 [details]
patch

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

> Source/WebCore/platform/mac/WebCoreSystemInterface.mm:195
> +void(*wkCABackingStoreCollectBlocking)(void);

I think we should change this name to something less mysterious.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:373
> +    if (!parameters.isInWindow) {
> +        // Page defaults to on-screen, but setIsInWindow depends on it being a valid indicator of actually having been put into a window.
> +        m_page->setCanStartMedia(false);
> +        m_page->willMoveOffscreen();
> +    }

Is this behavior change detectable at all? Will it break anything?

> Source/WebKit2/WebProcess/WebProcess.cpp:1078
> +    m_inWindowPageCount--;

I think you should assert that m_inWindowPageCount is > 0 here. Consider making it signed for slightly less bad behavior if miscounting occurs in a release build.

[Tim Horton]

(In reply to comment #6)
> (From update of attachment 194960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194960&action=review
> 
> > Source/WebCore/platform/mac/WebCoreSystemInterface.mm:195
> > +void(*wkCABackingStoreCollectBlocking)(void);
> 
> I think we should change this name to something less mysterious.

OK.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:373
> > +    if (!parameters.isInWindow) {
> > +        // Page defaults to on-screen, but setIsInWindow depends on it being a valid indicator of actually having been put into a window.
> > +        m_page->setCanStartMedia(false);
> > +        m_page->willMoveOffscreen();
> > +    }
> 
> Is this behavior change detectable at all? Will it break anything?

I don't think so (on both counts) because it is immediately rectified by setIsInWindow. I do, however, think that we don't need to setCanStartMedia here, because there is no time between when this code runs and when setIsInWindow sets it to the right value, so I'm going to remove that.

Also, I'm glad you pointed this out; this patch is from before the isInWindow vs. willMoveOffscreen disambiguation; it really wants to be doing setIsInWindow(false). I'll fix that.

> > Source/WebKit2/WebProcess/WebProcess.cpp:1078
> > +    m_inWindowPageCount--;
> 
> I think you should assert that m_inWindowPageCount is > 0 here. Consider making it signed for slightly less bad behavior if miscounting occurs in a release build.

Sure.

[Tim Horton]

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