Bug 67857 - Provide a way to get WebArchive from a BundleFrame
Summary: Provide a way to get WebArchive from a BundleFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-09 11:03 PDT by Ada Chan
Modified: 2011-09-16 12:18 PDT (History)
8 users (show)

See Also:


Attachments
Implement WKBundleFrameCopyWebArchive() (2.62 KB, patch)
2011-09-09 11:23 PDT, Ada Chan
sam: review-
Details | Formatted Diff | Diff
Move logic to get webarchive to WebFrame and add test in TestWebKitAPI. (21.35 KB, patch)
2011-09-14 10:22 PDT, Ada Chan
no flags Details | Formatted Diff | Diff
Small style error fixed. (21.35 KB, patch)
2011-09-14 10:36 PDT, Ada Chan
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2011-09-09 11:03:03 PDT
Implement WKBundleFrameCopyWebArchive().  

Need for <rdar://problem/9898848>
Comment 1 Sam Weinig 2011-09-09 11:06:07 PDT
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?
Comment 2 Ada Chan 2011-09-09 11:23:02 PDT
Created attachment 106890 [details]
Implement WKBundleFrameCopyWebArchive()
Comment 3 Sam Weinig 2011-09-09 11:26:28 PDT
I missed that this was on the BundleFrame, and not the UIProcess frame.  Carry on.
Comment 4 Sam Weinig 2011-09-09 11:28:07 PDT
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.
Comment 5 Ada Chan 2011-09-14 10:22:14 PDT
Created attachment 107348 [details]
Move logic to get webarchive to WebFrame and add test in TestWebKitAPI.
Comment 6 WebKit Review Bot 2011-09-14 10:25:10 PDT
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.
Comment 7 Ada Chan 2011-09-14 10:36:57 PDT
Created attachment 107350 [details]
Small style error fixed.
Comment 8 Eric Seidel (no email) 2011-09-14 12:55:55 PDT
Wow.  That's a nasty bug in EWS/update-webkit-chromium
Comment 9 Jeff Miller 2011-09-14 13:17:46 PDT
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.
Comment 10 Eric Seidel (no email) 2011-09-14 13:35:54 PDT
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.
Comment 11 Ada Chan 2011-09-14 14:30:50 PDT
Sam looked over the patch also.

Committed:
http://trac.webkit.org/changeset/95122
Comment 12 Brent Fulgham 2011-09-16 11:09:21 PDT
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.
Comment 13 Darin Adler 2011-09-16 11:50:02 PDT
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?
Comment 14 Ada Chan 2011-09-16 12:18:23 PDT
(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.