Bug 122604 – WebViews inside OS X screen savers have large caches, but should not

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Adam Roben (:aroben)]

WebViews inside OS X screen savers have large caches, but should not

[Adam Roben (:aroben)]

Created attachment 213892 [details]
Patch

[Alexey Proskuryakov]

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.

[Geoffrey Garen]

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.

[Adam Roben (:aroben)]

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?

[Adam Roben (:aroben)]

(Hi Geoff!)

[Geoffrey Garen]

> 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.

[Adam Roben (:aroben)]

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!

[Adam Roben (:aroben)]

Created attachment 213984 [details]
Patch

[Adam Roben (:aroben)]

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 {}.

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 213984 [details]
Patch

Clearing flags on attachment: 213984

Committed r157351: <http://trac.webkit.org/changeset/157351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.