Bug 47354 – Need WKPage API for serializing and restoring a page's state

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Need WKPage API for serializing and restoring a page's state, including its session state, such as back/forward list data.

[Brady Eidson]

Patch coming shortly, and will refer to https://bugs.webkit.org/show_bug.cgi?id=47355 for follow up work.

[Brady Eidson]

Created attachment 70096 [details]
Patch

[WebKit Review Bot]

Attachment 70096 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/Logging.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:63:  Use 0 instead of NULL.  [readability/null] [5]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:65:  Use 0 instead of NULL.  [readability/null] [5]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:67:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:67:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 7 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Created attachment 70098 [details]
Patch v2 

Fixed NULL -> 0 style errors, but the config.h stuff is bogus (we don't use it in WK2)

[WebKit Review Bot]

Attachment 70098 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Platform/Logging.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Per Sam's in person request and review, landed Logging channel stuff separately in http://trac.webkit.org/changeset/69323

[Brady Eidson]

Created attachment 70121 [details]
Patch v3 (without Logging.h/cpp additions)

[WebKit Review Bot]

Attachment 70121 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 70121 [details]
Patch v3 (without Logging.h/cpp additions)

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.

[Brady Eidson]

Created attachment 70476 [details]
Patch v4 - All of Darin's feedback, plus a versioning header Sam and I discussed.

[WebKit Review Bot]

Attachment 70476 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 70476 [details]
Patch v4 - All of Darin's feedback, plus a versioning header Sam and I discussed.

Whoops different version for windows coming soon

[Brady Eidson]

Created attachment 70477 [details]
Patch v5 - Darin's feedback + a cross platform version header.

[Brady Eidson]

Comment on attachment 70477 [details]
Patch v5 - Darin's feedback + a cross platform version header.

Sorry, not quite ready

[Brady Eidson]

Created attachment 70478 [details]
Patch v6 - The same, with debugging info, etc removed

[WebKit Review Bot]

Attachment 70477 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 70478 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/cf/WebPageProxyCF.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 70478 [details]
Patch v6 - The same, with debugging info, etc removed

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

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

Probably should name the context argument here.

> WebKit2/UIProcess/API/C/WKPage.cpp:164
> +    RefPtr<WebData> state = toImpl(pageRef)->sessionStateData(filter, context);
>      return toAPI(state.release().releaseRef());

No obvious need for a local variable here. Might read better as a single long line.

> WebKit2/UIProcess/API/C/WKPage.h:211
> +typedef bool (*WKPageSessionStateFilterCallback)(WKPageRef page, WKStringRef valueType, WKTypeRef value, void* context);
> +WK_EXPORT WKDataRef WKPageCopySessionState(WKPageRef page, void *context, WKPageSessionStateFilterCallback urlAllowedCallback);

Inconsistent void* vs. void * on these two lines.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:46
> +    const void* keys[2] = { SessionHistoryCurrentIndexKey(), SessionHistoryEntriesKey() };
> +    const void* values[2] = { currentIndex.get(), entries.get() };
> +
> +    RetainPtr<CFDictionaryRef> dictionary(AdoptCF, CFDictionaryCreate(0, keys, values, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

That constant 2 is repeated three times. Would be bad if these didn’t agree. No concrete idea for making it better.

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:59
> +        const void* keys[2] = { SessionHistoryEntryURLKey(), SessionHistoryEntryTitleKey() };
> +        const void* values[2] = { url.get(), title.get() };
> +
> +        RetainPtr<CFDictionaryRef> entryDictionary(AdoptCF, CFDictionaryCreate(0, keys, values, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Ditto.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:51
> +    const void* keys[1] = { SessionHistoryKey() };
> +    const void* values[1] = { sessionHistoryDictionary.get() };
> +    
> +    RetainPtr<CFDictionaryRef> stateDictionary(AdoptCF, CFDictionaryCreate(0, keys, values, 1, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

For a single entry you don’t really need to use an array.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:56
> +    Vector<unsigned char> stateVector(length + 4);

All these explicit "4" constants are slightly annoying. Maybe sizeof(UInt32)?

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:72
> +    if (!webData || webData->size() < 4)

Here’s another 4.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:83
> +    RetainPtr<CFDataRef> data(AdoptCF, CFDataCreate(0, webData->bytes() + 4, webData->size() - 4));

And two more 4’s.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:87
> +    if (propertyListError) {

Don’t you have to CFRelease this?

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:98
> +    if (CFGetTypeID(propertyList.get()) != CFDictionaryGetTypeID()) {
> +        LOG(SessionState, "SessionState property list is not a CFDictionaryRef (%i) - its CFTypeID is %i", (int)CFDictionaryGetTypeID(), (int)CFGetTypeID(propertyList.get()));
> +        return;
> +    }

I don’t think CFPropertyListCreateWithData can return a non-dictionary.

> WebKit2/WebKit2.xcodeproj/project.pbxproj:653
> -		BC111B0B112F5E4F00337BAB /* WebPageProxy.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WebPageProxy.cpp; sourceTree = "<group>"; };
> +		BC111B0B112F5E4F00337BAB /* WebPageProxy.cpp */ = {isa = PBXFileReference; explicitFileType = sourcecode.cpp.cpp; fileEncoding = 4; path = WebPageProxy.cpp; sourceTree = "<group>"; };

Why this change?

[Brady Eidson]

Fixing the windows build right now.

[Brady Eidson]

Created attachment 70510 [details]
Windows build fix (would like a review)

[Brady Eidson]

I commited http://trac.webkit.org/changeset/69538 before Darin's comments came through.  I'm going to follow up with a couple of his comments with his implicit review.  :)

[Brady Eidson]

http://trac.webkit.org/changeset/69556 was a build fix for Windows.

http://trac.webkit.org/changeset/69559 addresses some of Darin's second review feedback.

[Brady Eidson]

Brian also checked in http://trac.webkit.org/changeset/69542 as a Windows build fix.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69556 might have broken GTK Linux 64-bit Debug

[Darin Adler]

Comment on attachment 70510 [details]
Windows build fix (would like a review)

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

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:63
> +    if (!CFPropertyListWriteToStream(stateDictionary.get(), writeStream.get(), kCFPropertyListBinaryFormat_v1_0, 0))
> +        return 0;

I believe in this failure case you should call CFWriteStreamClose.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:65
> +    RetainPtr<CFDataRef> stateCFData(AdoptCF, (CFDataRef)CFWriteStreamCopyProperty(writeStream.get(), kCFStreamPropertyDataWritten));

I believe that after doing this you should call CFWriteStreamClose.