[Webkit-unassigned] [Bug 47354] Need WKPage API for serializing and restoring a page's state

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47354


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70121|review?                     |review+
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2010-10-08 16:44:58 PST ---
(From update of attachment 70121)
View in context: https://bugs.webkit.org/attachment.cgi?id=70121&action=review

> WebKit2/UIProcess/WebBackForwardList.h:93
> +// FIXME - <rdar://problem/8261624> and https://bugs.webkit.org/show_bug.cgi?id=47355 - 
> +//         When we have a solution for restoring the full back/forward list 
> +//         then causing a load of the current item, we will no longer need this.

Strange formatting here. We normally don't use "-" like this and line up multi-line comments like this.

> WebKit2/UIProcess/WebPageProxy.h:163
> +    typedef bool (*WebPageProxySessionStateFilterCallback)(WKPageRef page, WKStringRef type, WKTypeRef object, void*);

No need for the argument name "page" here.

> WebKit2/UIProcess/API/C/WKPage.cpp:157
> +    static RefPtr<WebString> sessionHistoryURLValueType = WebString::create("SessionHistoryURL");

This should not be a RefPtr. That will cause us to have a static destructor. Instead we just want to leak this string and store it in a WebString* or a WKStringRef global.

> WebKit2/UIProcess/API/C/WKPage.h:191
> +WK_EXPORT WKStringRef WKPageGetSessionHistoryURLValueType();

If these are supposed to be C header files, then these should say (void), not ().

> WebKit2/UIProcess/API/C/WKPage.h:193
> +typedef bool (*WKPageSessionStateFilterCallback)(WKPageRef page, WKStringRef valueType, WKTypeRef value, void*);

If this is a C header file, then the last argument needs an argument name.

> WebKit2/UIProcess/API/C/WKPage.h:194
> +WK_EXPORT WKDataRef WKPageCopySessionState(WKPageRef page, void *context, WKPageSessionStateFilterCallback urlAllowedCallback);

Not sure why you said “void *context” here and “void*” on the line above.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:40
> +    RetainPtr<CFMutableDictionaryRef> dictionary(AdoptCF, CFDictionaryCreateMutable(0, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Since this dictionary always has exactly two values, I think we could just use CFDictionaryCreate on it instead of making it mutable.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:42
> +    RetainPtr<CFNumberRef> currentIndex(AdoptCF, CFNumberCreate(0, kCFNumberSInt32Type, &m_current));

Since m_current is not an SInt32 or UInt32, but rather an unsigned, the right CFNumberType to use is kCFNumberIntType.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:51
> +        RetainPtr<CFMutableDictionaryRef> entryDictionary(AdoptCF, CFDictionaryCreateMutable(0, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Since this dictionary always has exactly two values, I think we could just use CFDictionaryCreate on it instead of making it mutable.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:64
> +    return dictionary.releaseRef();

Please use the new name leakRef rather than the old name releaseRef.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:70
> +    if (!cfIndex || CFGetTypeID(cfIndex) != CFNumberGetTypeID() || CFNumberIsFloatType(cfIndex)) {

Why the CFNumberIsFloat type check? That doesn’t seem right.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:76
> +    int32_t currentIndex;
> +    if (!CFNumberGetValue(cfIndex, kCFNumberSInt32Type, &currentIndex)) {

It would be more elegant to use int and kCFNumberIntType or SInt32 and kCFNumberSInt32Type.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:110
> +        // FIXME <rdar://problem/8261624> and https://bugs.webkit.org/show_bug.cgi?id=47355 - 
> +        //          The data for the above entry needs to be added to the full back/forward list.
> +        //          When we have a solution that restores the full back/forwardlist then causes a load of the current item,
> +        //          we will no longer need this.

Same comment-formatting thought as above.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:47
> +    RetainPtr<CFMutableDictionaryRef> stateDictionary(AdoptCF, CFDictionaryCreateMutable(0, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +    CFDictionarySetValue(stateDictionary.get(), SessionHistoryKey(), sessionHistoryDictionary.get());

Since this dictionary always has exactly one value, I think we could just use CFDictionaryCreate on it instead of making it mutable.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:55
> +    RefPtr<WebData> stateWebData = WebData::create(stateVector);
> +    return stateWebData.release();

No need for the local variable here.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:93
> +    // FIXME: When we have a solution for restoring the full back/forward list then causing a load of the current item,
> +    //        we will trigger that load here.  Until then, we use the "restored current URL" which can later be removed.

Same FIXME formatting comment.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.