WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Jeremy Moskovich
Comment 1
2011-01-25 00:49:45 PST
Created
attachment 80032
[details]
Patch
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
Attachment 80032
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7521301
WebKit Review Bot
Comment 4
2011-01-25 01:38:00 PST
Attachment 80032
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/7500319
Jeremy Moskovich
Comment 5
2011-01-25 02:09:42 PST
Created
attachment 80036
[details]
Patch
Early Warning System Bot
Comment 6
2011-01-25 02:38:58 PST
Attachment 80036
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7533350
WebKit Review Bot
Comment 7
2011-01-25 02:59:44 PST
Attachment 80036
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/7558343
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
Created
attachment 80590
[details]
Patch
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
Attachment 80590
[details]
did not build on gtk: Build output:
http://queues.webkit.org/results/7506778
Early Warning System Bot
Comment 17
2011-01-30 08:59:20 PST
Attachment 80590
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7584817
Jeremy Moskovich
Comment 18
2011-01-31 01:25:15 PST
Created
attachment 80624
[details]
Patch
Early Warning System Bot
Comment 19
2011-01-31 01:54:20 PST
Attachment 80624
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7567958
Jeremy Moskovich
Comment 20
2011-01-31 02:07:36 PST
Created
attachment 80628
[details]
Patch
Early Warning System Bot
Comment 21
2011-01-31 03:07:21 PST
Attachment 80628
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/7597969
Jeremy Moskovich
Comment 22
2011-01-31 04:32:52 PST
Created
attachment 80633
[details]
Patch
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
Created
attachment 82251
[details]
Patch
Jeremy Moskovich
Comment 27
2011-02-12 23:57:33 PST
Created
attachment 82252
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug