Bug 67857 – Provide a way to get WebArchive from a BundleFrame

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

[Ada Chan]

Implement WKBundleFrameCopyWebArchive().  

Need for <rdar://problem/9898848>

[Sam Weinig]

Do we really need this at the frame level?  Don't we already have a way to save WebArchives, can't we just re-use that?

[Ada Chan]

Created attachment 106890 [details]
Implement WKBundleFrameCopyWebArchive()

[Sam Weinig]

I missed that this was on the BundleFrame, and not the UIProcess frame.  Carry on.

[Sam Weinig]

Comment on attachment 106890 [details]
Implement WKBundleFrameCopyWebArchive()

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

We are trying to get in the habit of adding tests for new API, so please add a test in TestWebKitAPI.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:256
> +    RetainPtr<CFDataRef> data;
> +    if (RefPtr<LegacyWebArchive> archive = LegacyWebArchive::create(toImpl(frameRef)->coreFrame()->document())) {
> +        if ((data = archive->rawDataRepresentation()))
> +            return WKDataCreate(CFDataGetBytePtr(data.get()), CFDataGetLength(data.get()));
> +    }

The code to do this should be on the WebFrame itself.  In general, we try to keep the code in API files to a minimum, mainly casts.

[Ada Chan]

Created attachment 107348 [details]
Move logic to get webarchive to WebFrame and add test in TestWebKitAPI.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Last 3072 characters of output:
nts: set(['./'])
  
    name: net
    url: http://src.chromium.org/svn/trunk/src/net@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/net@100742
    should_process: True
    requirements: set(['./'])
  
    name: tools/data_pack
    url: http://src.chromium.org/svn/trunk/src/tools/data_pack@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/data_pack@100742
    should_process: True
    processed: True
    requirements: set(['./'])
  
    name: third_party/ffmpeg
    url: From('chromium_deps', 'src/third_party/ffmpeg')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/generate_stubs
    url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@100742
    should_process: True
    requirements: set(['./'])
  
    name: third_party/libjpeg_turbo
    url: From('chromium_deps', 'src/third_party/libjpeg_turbo')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/v8-i18n
    url: From('chromium_deps', 'src/third_party/v8-i18n')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/grit
    url: http://src.chromium.org/svn/trunk/src/tools/grit@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/grit@100742
    should_process: True
    requirements: set(['./'])
  
    name: base
    url: http://src.chromium.org/svn/trunk/src/base@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/base@100742
    should_process: True
    requirements: set(['./'])
  
    name: sql
    url: http://src.chromium.org/svn/trunk/src/sql@100742
    should_process: True
    requirements: set(['./'])
  
    name: v8
    url: From('chromium_deps', 'src/v8')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: testing/gtest
    url: From('chromium_deps', 'src/testing/gtest')
    should_process: True
    requirements: set(['testing', 'chromium_deps', './'])
  
    name: third_party/libwebp
    url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@100742
    should_process: True
    requirements: set(['third_party', './'])
  
    name: googleurl
    url: From('chromium_deps', 'src/googleurl')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: third_party/skia/include
    url: From('chromium_deps', 'src/third_party/skia/include')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/ots
    url: From('chromium_deps', 'src/third_party/ots')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/snappy/src
    url: From('chromium_deps', 'src/third_party/snappy/src')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])

Died at Tools/Scripts/update-webkit-chromium line 80.
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[Ada Chan]

Created attachment 107350 [details]
Small style error fixed.

[Eric Seidel (no email)]

Wow.  That's a nasty bug in EWS/update-webkit-chromium

[Jeff Miller]

Comment on attachment 107350 [details]
Small style error fixed.

I'm not sure PLATFORM(WIN) is a strict enough test when deciding whether CF data types are available. You may need to test USE(CF) instead, but I'm not sure.

[Eric Seidel (no email)]

PLATFORM(WIN) is poorly named and means "am I compiling the Apple Windows Port".  Assuming it still requires CF in all cases, than should be correct.  I don't think WIN-CAIRO uses PLATFORM(WIN), but Adam would know better than I.

[Ada Chan]

Sam looked over the patch also.

Committed:
http://trac.webkit.org/changeset/95122

[Brent Fulgham]

Whenever possible, it would be great if things that require CF simply use the 'USE(CF)' test.  In cases where Apple-specific behavior is needed (e.g., it is interacting with Safari-specific shims), then the PLATFORM(WIN) is probably better.

Ideally, the bits of code that rely on Apple-only libraries and support routines (e.g., the Windows port of Core Animation, etc.) would have a better name, such as "PLATFORM(WIN_APPLE)" or even just "PLATFORM(APPLE)".

But for now, "PLATFORM(WIN)" seems to be the definition with that meaning.

[Darin Adler]

Comment on attachment 107350 [details]
Small style error fixed.

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:245
> +WKDataRef WKBundleFrameCopyWebArchive(WKBundleFrameRef frameRef)

Do we need this for individual frames? Could we put this on WKBundlePage instead?

[Ada Chan]

(In reply to comment #13)
> (From update of attachment 107350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107350&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp:245
> > +WKDataRef WKBundleFrameCopyWebArchive(WKBundleFrameRef frameRef)
> 
> Do we need this for individual frames? Could we put this on WKBundlePage instead?

It's more flexible to put this on WKBundleFrame, in case caller wants to get webarchive of a subframe.