Bug 141009

Summary: Clean up / modernize PageCache class
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, japhet, kling, koivisto, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-01-28 13:39:25 PST 
Clean up / modernize PageCache class a bit.
Comment 1 Chris Dumez 2015-01-28 16:33:37 PST 
Created attachment 245578 [details]
Patch
Comment 2 Chris Dumez 2015-01-28 16:59:13 PST 
Created attachment 245585 [details]
Patch
Comment 3 Chris Dumez 2015-01-28 17:20:07 PST 
Created attachment 245587 [details]
Patch
Comment 4 Chris Dumez 2015-01-28 17:25:50 PST 
Created attachment 245588 [details]
Patch
Comment 5 Chris Dumez 2015-01-28 22:12:27 PST 
Created attachment 245609 [details]
Patch
Comment 6 Darin Adler 2015-01-29 00:10:19 PST 
Comment on attachment 245609 [details]
Patch

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

> Source/WebCore/history/BackForwardController.cpp:75
> -    if (!item)
> -        return;
> -
> -    m_page.goToItem(item, FrameLoadType::IndexedBackForward);
> +    if (item)
> +        m_page.goToItem(*item, FrameLoadType::IndexedBackForward);

I like the early return style better.

> Source/WebCore/history/BackForwardController.cpp:84
> -    HistoryItem* item = backItem();
> -    if (!item)
> -        return false;
> -
> -    m_page.goToItem(item, FrameLoadType::Back);
> -    return true;
> +    if (HistoryItem* item = backItem()) {
> +        m_page.goToItem(*item, FrameLoadType::Back);
> +        return true;
> +    }
> +    return false;

I like the early return style better.

> Source/WebCore/history/BackForwardController.cpp:93
> -    HistoryItem* item = forwardItem();
> -    if (!item)
> -        return false;
> -
> -    m_page.goToItem(item, FrameLoadType::Forward);
> -    return true;
> +    if (HistoryItem* item = forwardItem()) {
> +        m_page.goToItem(*item, FrameLoadType::Forward);
> +        return true;
> +    }
> +    return false;

I like the early return style better.

> Source/WebCore/history/PageCache.cpp:433
> -    case PruningReason::ReachedCapacity:
> +    case PruningReason::ReachedMaxSize:
>          return DiagnosticLoggingKeys::prunedDueToCapacityReached();

Difference in terminology is a little annoying.
Comment 7 Chris Dumez 2015-01-29 09:53:15 PST 
Created attachment 245627 [details]
Patch
Comment 8 Chris Dumez 2015-01-29 09:54:21 PST 
Comment on attachment 245609 [details]
Patch

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

>> Source/WebCore/history/BackForwardController.cpp:75
>> +        m_page.goToItem(*item, FrameLoadType::IndexedBackForward);
> 
> I like the early return style better.

Done.

>> Source/WebCore/history/BackForwardController.cpp:84
>> +    return false;
> 
> I like the early return style better.

Done.

>> Source/WebCore/history/BackForwardController.cpp:93
>> +    return false;
> 
> I like the early return style better.

Done.

>> Source/WebCore/history/PageCache.cpp:433
>>          return DiagnosticLoggingKeys::prunedDueToCapacityReached();
> 
> Difference in terminology is a little annoying.

Done.
Comment 9 WebKit Commit Bot 2015-01-29 10:38:51 PST 
Comment on attachment 245627 [details]
Patch

Clearing flags on attachment: 245627

Committed r179347: <http://trac.webkit.org/changeset/179347>
Comment 10 WebKit Commit Bot 2015-01-29 10:38:56 PST 
All reviewed patches have been landed.  Closing bug.
Comment 11 Tim Horton 2015-01-29 13:33:05 PST 
iOS build fix in http://trac.webkit.org/changeset/179360