Summary: | make the code in MemoryPressureHandler::respondToMemoryPressure shareable. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yongjun Zhang <yongjun_zhang> | ||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, ddkilzer, joepeck, msaboff, sam, webkit.review.bot, yongjun_zhang | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Yongjun Zhang
2012-01-10 13:52:33 PST
Created attachment 121913 [details]
Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS.
Comment on attachment 121913 [details] Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS. View in context: https://bugs.webkit.org/attachment.cgi?id=121913&action=review > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:118 > + releaseMemory(false); This boolean should ideally be an enum: releaseMemory(Normal) releaseMemory(Critical) > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:123 > +void MemoryPressureHandler::releaseMemory(bool critical) > +{ What do you think about moving this to a separate common file? As it is, 90% of this file is blacklisted for iOS. Separate files are easier to maintain that #ifdef imho (although the file would be very simple in this case). If you choose the #ifdef, do not forget to add || PLATFORM(IOS) to MemoryPressureHandler.cpp. > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:125 > - pageCache()->setCapacity(pageCache()->pageCount()/2); > + pageCache()->setCapacity(critical ? 0 : pageCache()->pageCount()/2); Spaces around operator. > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:131 > + [nsurlCache setMemoryCapacity:critical ? 0 : [nsurlCache currentMemoryUsage]/2]; Spaces around operator. (In reply to comment #3) > (From update of attachment 121913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121913&action=review > > > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:118 > > + releaseMemory(false); > > This boolean should ideally be an enum: > releaseMemory(Normal) > releaseMemory(Critical) > Agree. Currently we only have two levels, we can consider changing this to enum when new levels are introduced. > > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:123 > > +void MemoryPressureHandler::releaseMemory(bool critical) > > +{ > > What do you think about moving this to a separate common file? As it is, 90% of this file is blacklisted for iOS. Separate files are easier to maintain that #ifdef imho (although the file would be very simple in this case). > I thought about that, but didn't like the idea of having a separate file with just this one function. > If you choose the #ifdef, do not forget to add || PLATFORM(IOS) to MemoryPressureHandler.cpp. Ok, looks like we already had that for iOS. > > > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:125 > > - pageCache()->setCapacity(pageCache()->pageCount()/2); > > + pageCache()->setCapacity(critical ? 0 : pageCache()->pageCount()/2); > > Spaces around operator. > > > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:131 > > + [nsurlCache setMemoryCapacity:critical ? 0 : [nsurlCache currentMemoryUsage]/2]; > > Spaces around operator. Created attachment 122275 [details]
Address review comments on previous patch.
Address Benjamin's review comments, adding || PLATFORM(IOS) to MemoryPressureHander.cpp.
Comment on attachment 122275 [details] Address review comments on previous patch. View in context: https://bugs.webkit.org/attachment.cgi?id=122275&action=review > Source/WebCore/platform/MemoryPressureHandler.h:48 > + void releaseMemory(bool critical); I still feel the enum is better, even for only two values here. Comment on attachment 122275 [details] Address review comments on previous patch. Clearing flags on attachment: 122275 Committed r104873: <http://trac.webkit.org/changeset/104873> All reviewed patches have been landed. Closing bug. |