Allow ports such as Chromium & QT to compile out WebArchive support.
Created attachment 80032 [details] Patch
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.
Attachment 80032 [details] did not build on qt: Build output: http://queues.webkit.org/results/7521301
Attachment 80032 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7500319
Created attachment 80036 [details] Patch
Attachment 80036 [details] did not build on qt: Build output: http://queues.webkit.org/results/7533350
Attachment 80036 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7558343
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 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 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.
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?
(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.
I believe this is also used to make image Copy and Paste work.
Created attachment 80590 [details] Patch
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.
Attachment 80590 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7506778
Attachment 80590 [details] did not build on qt: Build output: http://queues.webkit.org/results/7584817
Created attachment 80624 [details] Patch
Attachment 80624 [details] did not build on qt: Build output: http://queues.webkit.org/results/7567958
Created attachment 80628 [details] Patch
Attachment 80628 [details] did not build on qt: Build output: http://queues.webkit.org/results/7597969
Created attachment 80633 [details] Patch
Would be grateful if someone on the cc list could review, thanks!
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.
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.
Created attachment 82251 [details] Patch
Created attachment 82252 [details] Patch
Comment on attachment 82252 [details] Patch Clearing flags on attachment: 82252 Committed r78439: <http://trac.webkit.org/changeset/78439>
All reviewed patches have been landed. Closing bug.