<rdar://problem/13032692>
Created attachment 184074 [details] patch (I'm going to test one more time)
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.
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.
Created attachment 194960 [details] patch
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.
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.
(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.
http://trac.webkit.org/changeset/146956