[webkit-reviews] review granted: [Bug 47354] Need WKPage API for serializing and restoring a page's state : [Attachment 70121] Patch v3 (without Logging.h/cpp additions)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 47354: Need WKPage API for serializing and restoring a page's state
https://bugs.webkit.org/show_bug.cgi?id=47354

Attachment 70121: Patch v3 (without Logging.h/cpp additions)
https://bugs.webkit.org/attachment.cgi?id=70121&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.