Bug 52712 - Add a compile-time option to disable WebArchive support
Summary: Add a compile-time option to disable WebArchive support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-19 05:03 PST by Jeremy Moskovich
Modified: 2011-02-13 12:29 PST (History)
12 users (show)

See Also:


Attachments
Patch (19.71 KB, patch)
2011-01-25 00:49 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (27.28 KB, patch)
2011-01-25 02:09 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (18.58 KB, patch)
2011-01-30 06:23 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (19.49 KB, patch)
2011-01-31 01:25 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (20.75 KB, patch)
2011-01-31 02:07 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (22.54 KB, patch)
2011-01-31 04:32 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (22.58 KB, patch)
2011-02-12 23:53 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Patch (22.57 KB, patch)
2011-02-12 23:57 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2011-01-19 05:03:46 PST
Allow ports such as Chromium & QT to compile out WebArchive support.
Comment 1 Jeremy Moskovich 2011-01-25 00:49:45 PST
Created attachment 80032 [details]
Patch
Comment 2 Jeremy Moskovich 2011-01-25 00:53:53 PST
I added a
#endif // ENABLE(WEB_ARCHIVE)
comment to the closing #endif in cases where the #ifdef was far enough away for the scope of the #endif to be confusing.

Reviewers: If you'd like me to change this, please let me know.
Comment 3 Early Warning System Bot 2011-01-25 01:19:58 PST
Attachment 80032 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7521301
Comment 4 WebKit Review Bot 2011-01-25 01:38:00 PST
Attachment 80032 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7500319
Comment 5 Jeremy Moskovich 2011-01-25 02:09:42 PST
Created attachment 80036 [details]
Patch
Comment 6 Early Warning System Bot 2011-01-25 02:38:58 PST
Attachment 80036 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7533350
Comment 7 WebKit Review Bot 2011-01-25 02:59:44 PST
Attachment 80036 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7558343
Comment 8 Adam Barth 2011-01-25 10:58:49 PST
Comment on attachment 80036 [details]
Patch

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

Looks reasonable to me.  Certainly if WebArchive were added today, it would have an ifdef.  I can't verify that all the placement details are correct, but it's plausible.

> Source/WebCore/ChangeLog:3
> +

You've got an extra line here.

> Source/WebCore/WebCore.gypi:-2096
> -            'loader/archive/cf/LegacyWebArchive.cpp',
> -            'loader/archive/cf/LegacyWebArchive.h',
> -            'loader/archive/cf/LegacyWebArchiveMac.mm',
> -            'loader/archive/Archive.h',
> -            'loader/archive/ArchiveFactory.cpp',
> -            'loader/archive/ArchiveFactory.h',
> -            'loader/archive/ArchiveResource.cpp',
> -            'loader/archive/ArchiveResource.h',
> -            'loader/archive/ArchiveResourceCollection.cpp',
> -            'loader/archive/ArchiveResourceCollection.h',

The gypi should list all files in the project, regardless of whether they're used by Chromium.

> Source/WebKit2/UIProcess/WebFrameProxy.h:113
>      void getWebArchive(PassRefPtr<DataCallback>);
>      void getMainResourceData(PassRefPtr<DataCallback>);
>      void getResourceData(WebURL*, PassRefPtr<DataCallback>);

This functions are mis-named (not the fault of this patch).
Comment 9 Darin Adler 2011-01-25 11:52:41 PST
Comment on attachment 80036 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebFrameProxy.h:113
>>      void getResourceData(WebURL*, PassRefPtr<DataCallback>);
> 
> This functions are mis-named (not the fault of this patch).

They are not. These are calls that asynchronously get data. They have the word get in their names to emphasize that their results are conveyed in a way that is different from a function return value, analogous to how functions with out arguments use the word get.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:991
>  void WebPageProxy::getResourceDataFromFrame(WebFrameProxy* frame, WebURL* resourceURL, PassRefPtr<DataCallback> prpCallback)

This is not a web archive function. I think the conditional is wrong.

> Source/WebKit2/UIProcess/WebPageProxy.h:276
> +#if ENABLE(WEB_ARCHIVE)
>      void getResourceDataFromFrame(WebFrameProxy*, WebURL*, PassRefPtr<DataCallback>);
> +    void getWebArchiveOfFrame(WebFrameProxy*, PassRefPtr<DataCallback>);
> +#endif

Removing these functions will break compilation of WKFrame.cpp for platforms that turn off WEB_ARCHIVE, so this is incomplete.

We won’t conditionalize WKFrame.h since it is an API header intended to be used outside WebKit, so we’ll need to solve that problem.

Perhaps this is why the Qt and GTK builds are broken.
Comment 10 Adam Barth 2011-01-25 12:00:00 PST
Comment on attachment 80036 [details]
Patch

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

>>> Source/WebKit2/UIProcess/WebFrameProxy.h:113
>>>      void getWebArchive(PassRefPtr<DataCallback>);
>>>      void getMainResourceData(PassRefPtr<DataCallback>);
>>>      void getResourceData(WebURL*, PassRefPtr<DataCallback>);
>> 
>> This functions are mis-named (not the fault of this patch).
> 
> They are not. These are calls that asynchronously get data. They have the word get in their names to emphasize that their results are conveyed in a way that is different from a function return value, analogous to how functions with out arguments use the word get.

Thanks!  Good to know.
Comment 11 Jeremy Moskovich 2011-01-26 04:40:34 PST
Darin: Thanks for looking this over!

The call tree for the function you mentioned seems to be:
WebPageProxy:: getResourceDataFromFrame()
     WebPage::getResourceDataFromFrame()
          DocumentLoader::subresource()

DocumentLoader::subresource() returns an ArchiveResource object.

Are ArchiveResource objects used for anything other than .webarchive support in WebKit?
Comment 12 Darin Adler 2011-01-26 10:47:43 PST
(In reply to comment #11)
> The call tree for the function you mentioned seems to be:
> WebPageProxy:: getResourceDataFromFrame()
>      WebPage::getResourceDataFromFrame()
>           DocumentLoader::subresource()
> 
> DocumentLoader::subresource() returns an ArchiveResource object.
> 
> Are ArchiveResource objects used for anything other than .webarchive support in WebKit?

This feature is used for commands like Save Image As in the Safari context menu. Commands that have nothing to do with .webarchive files. I don’t know if that answers your question.
Comment 13 Darin Adler 2011-01-26 11:00:24 PST
I believe this is also used to make image Copy and Paste work.
Comment 14 Jeremy Moskovich 2011-01-30 06:23:23 PST
Created attachment 80590 [details]
Patch
Comment 15 Jeremy Moskovich 2011-01-30 06:39:06 PST
New patch uploaded taking a more conservative approach:
* Don't remove DocumentLoader::subresource(), Archive & ArchiveResource since they're needed for non-webArchive functionality.
* Don't touch build files for non-chromium ports.
Comment 16 Gustavo Noronha (kov) 2011-01-30 06:50:47 PST
Attachment 80590 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7506778
Comment 17 Early Warning System Bot 2011-01-30 08:59:20 PST
Attachment 80590 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7584817
Comment 18 Jeremy Moskovich 2011-01-31 01:25:15 PST
Created attachment 80624 [details]
Patch
Comment 19 Early Warning System Bot 2011-01-31 01:54:20 PST
Attachment 80624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7567958
Comment 20 Jeremy Moskovich 2011-01-31 02:07:36 PST
Created attachment 80628 [details]
Patch
Comment 21 Early Warning System Bot 2011-01-31 03:07:21 PST
Attachment 80628 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7597969
Comment 22 Jeremy Moskovich 2011-01-31 04:32:52 PST
Created attachment 80633 [details]
Patch
Comment 23 Jeremy Moskovich 2011-02-06 02:14:01 PST
Would be grateful if someone on the cc list could review, thanks!
Comment 24 Adam Barth 2011-02-09 14:33:28 PST
Comment on attachment 80633 [details]
Patch

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

This looks good to me, but I'm not a WebArchive expert.  I've been avoiding reviewing this patch because folks pointed out errors in my previous review.  However, no one else seems to be reviewing this patch...  Maybe give folks some time to take another look before landing?

> Source/WebCore/ChangeLog:15
> +            WebArchive support is currently enabled for all ports that define PLATFORM(CF) apart from QT.

QT => Qt

> Source/WebCore/WebCore.gyp/WebCore.gyp:1348
> +        # Don't build files needed for WebArchive support. Since we disable
> +        # this feature.

The second sentence here isn't a complete sentence.
Comment 25 Jeremy Moskovich 2011-02-10 01:50:42 PST
Thanks Adam,  I'll wait until next week to land (with fixes for your latest comments).

I want to make sure everyone else has time to take a look.
Comment 26 Jeremy Moskovich 2011-02-12 23:53:38 PST
Created attachment 82251 [details]
Patch
Comment 27 Jeremy Moskovich 2011-02-12 23:57:33 PST
Created attachment 82252 [details]
Patch
Comment 28 WebKit Commit Bot 2011-02-13 12:28:58 PST
Comment on attachment 82252 [details]
Patch

Clearing flags on attachment: 82252

Committed r78439: <http://trac.webkit.org/changeset/78439>
Comment 29 WebKit Commit Bot 2011-02-13 12:29:03 PST
All reviewed patches have been landed.  Closing bug.