Bug 52712 – Add a compile-time option to disable WebArchive support

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jeremy Moskovich]

Allow ports such as Chromium & QT to compile out WebArchive support.

[Jeremy Moskovich]

Created attachment 80032 [details]
Patch

[Jeremy Moskovich]

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.

[Early Warning System Bot]

Attachment 80032 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7521301

[WebKit Review Bot]

Attachment 80032 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7500319

[Jeremy Moskovich]

Created attachment 80036 [details]
Patch

[Early Warning System Bot]

Attachment 80036 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7533350

[WebKit Review Bot]

Attachment 80036 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7558343

[Adam Barth]

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).

[Darin Adler]

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.

[Adam Barth]

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.

[Jeremy Moskovich]

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?

[Darin Adler]

(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.

[Darin Adler]

I believe this is also used to make image Copy and Paste work.

[Jeremy Moskovich]

Created attachment 80590 [details]
Patch

[Jeremy Moskovich]

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.

[Gustavo Noronha (kov)]

Attachment 80590 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7506778

[Early Warning System Bot]

Attachment 80590 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7584817

[Jeremy Moskovich]

Created attachment 80624 [details]
Patch

[Early Warning System Bot]

Attachment 80624 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7567958

[Jeremy Moskovich]

Created attachment 80628 [details]
Patch

[Early Warning System Bot]

Attachment 80628 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7597969

[Jeremy Moskovich]

Created attachment 80633 [details]
Patch

[Jeremy Moskovich]

Would be grateful if someone on the cc list could review, thanks!

[Adam Barth]

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.

[Jeremy Moskovich]

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.

[Jeremy Moskovich]

Created attachment 82251 [details]
Patch

[Jeremy Moskovich]

Created attachment 82252 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 82252 [details]
Patch

Clearing flags on attachment: 82252

Committed r78439: <http://trac.webkit.org/changeset/78439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.