Bug 88399

Created attachment 145961 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 145961 [details] Patch Attachment 145961 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12908131 New failing tests: canvas/philip/tests/2d.gradient.radial.cone.top.html canvas/philip/tests/2d.gradient.radial.cone.shape2.html

Created attachment 146162 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 146217 [details] Patch Remove bundle API and bump the WKPageLoaderClient version.

Comment on attachment 146217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146217&action=review > Source/WebKit2/Shared/APIClientTraits.h:57 > template<> struct APIClientTraits<WKPageLoaderClient> { > - static const size_t interfaceSizesByVersion[2]; > + static const size_t interfaceSizesByVersion[3]; > }; I don't know about this versioning, maybe someone else can have a look > Source/WebKit2/Shared/IntentServiceInfo.h:14 > + * * Neither the name of Intel Corporation nor the names of its Is the Intel name the only thing that differs here from what is commonly used in WebKit? > Source/WebKit2/Shared/IntentServiceInfo.h:53 > + WebCore::KURL href; Any reason for using href instead of url ? > Source/WebKit2/UIProcess/WebIntentServiceInfo.cpp:45 > +namespace WebKit { > + > +WebIntentServiceInfo::WebIntentServiceInfo(const IntentServiceInfo& store) > + : m_store(store) > +{ > +} > + > +WebIntentServiceInfo::~WebIntentServiceInfo() > +{ > +} Any reason why not just doing this in the header file? > Source/WebKit2/UIProcess/WebLoaderClient.cpp:167 > +#if ENABLE(WEB_INTENTS_TAG) Given the current discussion around WebIntents and WebActivities, I am not sure this will remain a tag, so why not just use WEB_INTENTS ? > Source/WebKit2/UIProcess/WebLoaderClient.cpp:168 > +void WebLoaderClient::registerIntentServiceForFrame(WebPageProxy* page, WebFrameProxy* frame, const IntentServiceInfo& serviceInfo) IntentsService? or just registerIntentForFrame ? > Source/WebKit2/UIProcess/WebLoaderClient.h:52 > class WebPageProxy; > class WebProtectionSpace; > > +#if ENABLE(WEB_INTENTS_TAG) > +struct IntentServiceInfo; > +#endif WebIntentsServiceInfo for consistency? > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1563 > + notImplemented(); This seems wrong

Comment on attachment 146217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146217&action=review >> Source/WebKit2/Shared/APIClientTraits.h:57 >> }; > > I don't know about this versioning, maybe someone else can have a look The version bump was requested by andersca. >> Source/WebKit2/Shared/IntentServiceInfo.h:14 >> + * * Neither the name of Intel Corporation nor the names of its > > Is the Intel name the only thing that differs here from what is commonly used in WebKit? This is the standard BSD header we are asked to use at Intel. Is this a problem? I guess I could use the same one as in the other files if this is a problem. >> Source/WebKit2/Shared/IntentServiceInfo.h:53 >> + WebCore::KURL href; > > Any reason for using href instead of url ? Just trying to match the spec for consistency: http://dvcs.w3.org/hg/web-intents/raw-file/tip/spec/Overview.html#registration-markup >> Source/WebKit2/UIProcess/WebIntentServiceInfo.cpp:45 >> +} > > Any reason why not just doing this in the header file? I could. I followed what was done for WebNavigationData class in same folder. WebNavigationData.cpp was written and contains pretty much the same thing. >> Source/WebKit2/UIProcess/WebLoaderClient.cpp:167 >> +#if ENABLE(WEB_INTENTS_TAG) > > Given the current discussion around WebIntents and WebActivities, I am not sure this will remain a tag, so why not just use WEB_INTENTS ? I don't really understand why we would use WEB_INTENTS here. WEB_INTENTS can be enabled while WEB_INTENTS_TAG is disabled at the moment. I'm matching what is in WebCore. If the 2 merge in the future, then we can replace it then. >> Source/WebKit2/UIProcess/WebLoaderClient.cpp:168 >> +void WebLoaderClient::registerIntentServiceForFrame(WebPageProxy* page, WebFrameProxy* frame, const IntentServiceInfo& serviceInfo) > > IntentsService? or just registerIntentForFrame ? This is really for registering an intent service, not an intent. "registerIntentService" is what is used in FrameLoaderClient. >> Source/WebKit2/UIProcess/WebLoaderClient.h:52 >> +#endif > > WebIntentsServiceInfo for consistency? You mean that you want registerIntentServiceForFrame() in this file to take a WebIntentServiceInfo in argument instead of an IntentServiceInfo? If so, Ok, I can fix this. >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1563 >> + notImplemented(); > > This seems wrong Yes it does :) I'll remove it.

Created attachment 147271 [details] Patch Take Kenneth's feedback into consideration: - Change license headers - Remove the extra notImplemented() in WebFrameLoaderClient.cpp - Move empty destructor from implementation to header (WebIntentServiceInfo) - Use WebIntentServiceInfo instead of IntentServiceInfo in WebLoaderClient I kept the rest as is for now (please see my previous comments).

Comment on attachment 147271 [details] Patch Attachment 147271 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12939975

Created attachment 147274 [details] Patch Tiny Changelog fix.

Comment on attachment 147274 [details] Patch Attachment 147274 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12954016

Created attachment 147323 [details] Patch Rebase on master and fix the wk2-qt build.

Created attachment 147330 [details] Patch

Created attachment 147482 [details] Patch for landing Rebase on master.

Comment on attachment 147482 [details] Patch for landing Clearing flags on attachment: 147482 Committed r120301: <http://trac.webkit.org/changeset/120301>

All reviewed patches have been landed. Closing bug.