Bug 79421

Summary: Need a WK1 Mac API to filter which subframes go into WebArchives as they are created
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: ASSIGNED ---    
Severity: Normal CC: commit-queue
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: All   
Attachments:
Description Flags
Patch v1
sam: review-
Patch v2 - Use a block sam: review+

Description Brady Eidson 2012-02-23 16:49:19 PST
Need a WK1 Mac API to filter which subframes go into WebArchives as they are created 

This API will use the WebCore code landed as part of https://bugs.webkit.org/show_bug.cgi?id=77766

In radar as <rdar://problem/10805709>
Comment 1 Brady Eidson 2012-02-23 16:55:09 PST
Created attachment 128606 [details]
Patch v1
Comment 2 Sam Weinig 2012-02-23 17:51:13 PST
Comment on attachment 128606 [details]
Patch v1

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

> Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:53
> +@interface DOMNode (WebDOMNodeOperationsPrivate)
> +- (WebArchive *)webArchiveWithWebArchiveCreationDelegate:(id)webArchiveCreationDelegate;
> +@end

This should take a block, not an arbitrary object.
Comment 3 Brady Eidson 2012-02-23 18:47:49 PST
Created attachment 128631 [details]
Patch v2 - Use a block
Comment 4 WebKit Commit Bot 2012-02-24 02:22:16 PST
Attachment 128631 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/mac/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Sam Weinig 2012-02-24 09:09:47 PST
Comment on attachment 128631 [details]
Patch v2 - Use a block

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

Feel free to add an API test as well.

> Source/WebKit/mac/DOM/WebDOMOperations.mm:89
> +class WebFrameFilter : public WebCore::FrameFilter {
> +public:
> +    WebFrameFilter(ShouldIncludeSubframeInWebArchiveBlock filterBlock);
> +        
> +private:
> +    virtual bool shouldIncludeSubframe(Frame*) const OVERRIDE;
> +
> +    ShouldIncludeSubframeInWebArchiveBlock m_filterBlock;
> +};

Though perhaps not strictly necessary due to the one place it is used, it seems safer to Block_copy/Block_release this in the c++ class.
Comment 6 Adam Roben (:aroben) 2012-02-24 09:19:35 PST
Comment on attachment 128631 [details]
Patch v2 - Use a block

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

> Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:54
> +- (WebArchive *)webArchiveFilteringSubframes:(ShouldIncludeSubframeInWebArchiveBlock)shouldIncludeSubframeInWebArchive;

Maybe webArchiveWithSubframeFilter: would be a better name? And the block typedef could be WebArchiveSubframeFilter. (I'm not sure the block typedef needs to include the word "block".)
Comment 7 Adam Roben (:aroben) 2012-02-24 09:20:12 PST
Comment on attachment 128631 [details]
Patch v2 - Use a block

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

>> Source/WebKit/mac/DOM/WebDOMOperationsPrivate.h:54
>> +- (WebArchive *)webArchiveFilteringSubframes:(ShouldIncludeSubframeInWebArchiveBlock)shouldIncludeSubframeInWebArchive;
> 
> Maybe webArchiveWithSubframeFilter: would be a better name? And the block typedef could be WebArchiveSubframeFilter. (I'm not sure the block typedef needs to include the word "block".)

After all, this block is just being used to perform a functional-style filter operation.