WebViews inside OS X screen savers have large caches, but should not
Created attachment 213892 [details] Patch
This whole mechanism with hardcoded bundle IDs is so icky that I can't make myself r+ the patch. But I don't see anything wrong with the patch per se, it seems likely indeed that screen saver modules don't use WebViews to create a browser with back/forward navigation.
Comment on attachment 213892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213892&action=review Hi Adam! > Source/WebKit/mac/ChangeLog:10 > + against legacy WebKit. ScreenSaverEngine.app doesn't link against > + WebKit at all, and thus falls into the legacy case by accident because > + NSVersionOfLinkTimeLibrary("WebKit") returns -1. I don't think that was the original intent. Also, the new behavior has been around long enough that backwards-compatibility is less of a concern. I think we should give the new WebCacheModelDocumentViewer behavior to any app that does not link directly against WebKit.
Comment on attachment 213892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213892&action=review >> Source/WebKit/mac/ChangeLog:10 >> + NSVersionOfLinkTimeLibrary("WebKit") returns -1. > > I don't think that was the original intent. Also, the new behavior has been around long enough that backwards-compatibility is less of a concern. I think we should give the new WebCacheModelDocumentViewer behavior to any app that does not link directly against WebKit. Are you also saying we should give WebCacheModelDocumentViewer behavior to apps linked against very old versions of WebKit?
(Hi Geoff!)
> Are you also saying we should give WebCacheModelDocumentViewer behavior to apps linked against very old versions of WebKit? Let's see... I don't think so. I guess I'm saying this: (1) Very old apps that link to WebKit should get the backwards-compatible thing (i.e., WebCacheModelDocumentBrowser, unless they're in the specific list of exceptions detailed by cacheModelForMainBundle). (2) Old and/or new apps that don't link to WebKit at all should get the new thing (i.e., WebCacheModelDocumentViewer). It's reasonable to assume that, if you don't link to WebKit, you probably aren't a browser. (3) New apps that do link to WebKit should get the new thing (i.e., WebCacheModelDocumentViewer). I could be convinced either way on (1), since the use case is rare at this point. (2) and (3) seem essential. We don't want to surprise unsuspecting apps with high memory footprint.
OK, thanks for the clarification. I wanted to make sure I was interpreting your "backwards-compatibility" comment correctly, and I wasn't. New patch coming up!
Created attachment 213984 [details] Patch
Created attachment 213985 [details] Patch ignoring whitespace Here's a copy of the patch with whitespace changes ignored, to make it easier to see the real changes without the indentation forced by @autoreleasepool {}.
Comment on attachment 213984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213984&action=review > Source/WebKit/mac/WebView/WebPreferences.mm:134 > + if (contains(documentViewerIDs, sizeof(documentViewerIDs) / sizeof(documentViewerIDs[0]), bundleID)) > + return WebCacheModelDocumentViewer; > + if (contains(documentBrowserIDs, sizeof(documentBrowserIDs) / sizeof(documentBrowserIDs[0]), bundleID)) > + return WebCacheModelDocumentBrowser; > + if (contains(primaryWebBrowserIDs, sizeof(primaryWebBrowserIDs) / sizeof(primaryWebBrowserIDs[0]), bundleID)) > + return WebCacheModelPrimaryWebBrowser; As long as you were modernizing, you could have used WTF_ARRAY_LENGTH here.
Comment on attachment 213984 [details] Patch Clearing flags on attachment: 213984 Committed r157351: <http://trac.webkit.org/changeset/157351>
All reviewed patches have been landed. Closing bug.