RESOLVED FIXED 52712
Add a compile-time option to disable WebArchive support
https://bugs.webkit.org/show_bug.cgi?id=52712
Summary Add a compile-time option to disable WebArchive support
Jeremy Moskovich
Reported 2011-01-19 05:03:46 PST
Allow ports such as Chromium & QT to compile out WebArchive support.
Attachments
Patch (19.71 KB, patch)
2011-01-25 00:49 PST, Jeremy Moskovich
no flags
Patch (27.28 KB, patch)
2011-01-25 02:09 PST, Jeremy Moskovich
no flags
Patch (18.58 KB, patch)
2011-01-30 06:23 PST, Jeremy Moskovich
no flags
Patch (19.49 KB, patch)
2011-01-31 01:25 PST, Jeremy Moskovich
no flags
Patch (20.75 KB, patch)
2011-01-31 02:07 PST, Jeremy Moskovich
no flags
Patch (22.54 KB, patch)
2011-01-31 04:32 PST, Jeremy Moskovich
no flags
Patch (22.58 KB, patch)
2011-02-12 23:53 PST, Jeremy Moskovich
no flags
Patch (22.57 KB, patch)
2011-02-12 23:57 PST, Jeremy Moskovich
no flags
Jeremy Moskovich
Comment 1 2011-01-25 00:49:45 PST
Jeremy Moskovich
Comment 2 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.
Early Warning System Bot
Comment 3 2011-01-25 01:19:58 PST
WebKit Review Bot
Comment 4 2011-01-25 01:38:00 PST
Jeremy Moskovich
Comment 5 2011-01-25 02:09:42 PST
Early Warning System Bot
Comment 6 2011-01-25 02:38:58 PST
WebKit Review Bot
Comment 7 2011-01-25 02:59:44 PST
Adam Barth
Comment 8 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).
Darin Adler
Comment 9 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.
Adam Barth
Comment 10 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.
Jeremy Moskovich
Comment 11 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?
Darin Adler
Comment 12 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.
Darin Adler
Comment 13 2011-01-26 11:00:24 PST
I believe this is also used to make image Copy and Paste work.
Jeremy Moskovich
Comment 14 2011-01-30 06:23:23 PST
Jeremy Moskovich
Comment 15 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.
Gustavo Noronha (kov)
Comment 16 2011-01-30 06:50:47 PST
Early Warning System Bot
Comment 17 2011-01-30 08:59:20 PST
Jeremy Moskovich
Comment 18 2011-01-31 01:25:15 PST
Early Warning System Bot
Comment 19 2011-01-31 01:54:20 PST
Jeremy Moskovich
Comment 20 2011-01-31 02:07:36 PST
Early Warning System Bot
Comment 21 2011-01-31 03:07:21 PST
Jeremy Moskovich
Comment 22 2011-01-31 04:32:52 PST
Jeremy Moskovich
Comment 23 2011-02-06 02:14:01 PST
Would be grateful if someone on the cc list could review, thanks!
Adam Barth
Comment 24 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.
Jeremy Moskovich
Comment 25 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.
Jeremy Moskovich
Comment 26 2011-02-12 23:53:38 PST
Jeremy Moskovich
Comment 27 2011-02-12 23:57:33 PST
WebKit Commit Bot
Comment 28 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>
WebKit Commit Bot
Comment 29 2011-02-13 12:29:03 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.