Created attachment 126578 [details] Patch Note: Local self reviewed diff is here webkit/ad907cc8b91037c255883c2cf3e8c34ffe13aa3c

Comment on attachment 126578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126578&action=review Looks good, but I think it can be cleaned up some more. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:68 > +#include <SkBitmap.h> You probably do not need this. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:72 > +using WTF::String; I think this means that below you dont have to do WTF::String, but just String. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:180 > +void FrameLoaderClientBlackBerry::dispatchDecidePolicyForResponse(FramePolicyFunction function, const WebCore::ResourceResponse& response, const WebCore::ResourceRequest& request) Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:185 > + if (WebCore::contentDispositionType(response.httpHeaderField("Content-Disposition")) == WebCore::ContentDispositionAttachment Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:211 > + // Let the client have a chance to say whether this navigation should take take? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:213 > + No need for empty line > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:491 > +void FrameLoaderClientBlackBerry::dispatchDidReceiveResponse(WebCore::DocumentLoader*, unsigned long identifier, const WebCore::ResourceResponse& response) Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:723 > + if (WebCore::isBackForwardLoadType(m_frame->loader()->loadType())) { Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:804 > + return WebCore::ObjectContentOtherPlugin; Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:918 > + BackForwardListImpl* backForwardList = static_cast<WebCore::BackForwardListImpl*>(m_webPagePrivate->m_page->backForward()->client()); Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:997 > + if (WebCore::isBackForwardLoadType(loader->loadType())) { Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1209 > + Image* img = WebCore::iconDatabase().synchronousIconForPageURL(url, IntSize(10, 10)); Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1210 > + WTF::String iconUrl = WebCore::iconDatabase().synchronousIconURLForPageURL(url); Remove WebCore prefix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:26 > +#include "HTMLFormElement.h" Can be a forward reference. For all of the above please check if you can do the same. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:33 > +class WebPluginClient; Is this one needed? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:153 > + virtual WTF::PassRefPtr<Frame> createFrame(const KURL&, const String&, HTMLFrameOwnerElement*, const String&, bool, int, int); You can probably remove WTF:: from PassRefPtr everywhere in the header.

Created attachment 126604 [details] Patch Update the patch based on Rob's comments.

Comment on attachment 126604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126604&action=review Can still be cleaned up some more. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:537 > + SecurityPolicy::addOriginAccessWhitelistEntry(*securityOrigin, String("tel"), String(""), true); All of the above cases should use emptyString instead of "". > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:591 > + RefPtr<NodeList> nodeList = headElement->getElementsByTagName("meta"); Can you use HTMLNames::metaTag here? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:618 > + nodeList = headElement->getElementsByTagName("link"); Can you use HTMLNames::linkTag here? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:815 > + if (m_pluginView) { Please do early return here. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1208 > + Image* img = iconDatabase().synchronousIconForPageURL(url, IntSize(10, 10)); Better add early return here: if (!img || !img->data()) return; > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1209 > + String iconUrl = iconDatabase().synchronousIconURLForPageURL(url); This can be moved to below just before it is actually being used. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1211 > + NativeImageSkia* bitmap= img->nativeImageForCurrentFrame(); add space before '=' > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1224 > + RefPtr<NodeList> nodeList = m_frame->document()->getElementsByTagName("video"); Can you use HTMLNames::videoTag here? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1227 > + nodeList = m_frame->document()->getElementsByTagName("audio"); Can you use HTMLNames::audioTag here? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1233 > + nodeList = m_frame->document()->getElementsByTagName("img"); Can you use HTMLNames::imgTag here? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1247 > + // and then when the user navigates back, it will scroll to the right position. I would do s/and then/Then > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:121 > + virtual ResourceError interruptedForPolicyChangeError(const ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); } Please use emptyString for "". > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:122 > + virtual ResourceError cancelledError(const ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:123 > + virtual ResourceError blockedError(const ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:125 > + virtual ResourceError interruptForPolicyChangeError(const ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:126 > + virtual ResourceError cannotShowMIMETypeError(const ResourceResponse&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:127 > + virtual ResourceError fileDoesNotExistError(const ResourceResponse&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:128 > + virtual ResourceError pluginWillHandleLoadError(const ResourceResponse&) { notImplemented(); return ResourceError("", 0, "", ""); } Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:154 > + virtual PassRefPtr<Widget> createPlugin(const IntSize&, HTMLPlugInElement*, const KURL&, const WTF::Vector<String, 0u>&, const WTF::Vector<String, 0u>&, const String&, bool); You probably do not need the WTF prefix here. Is the 0u param needed at all? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:156 > + virtual PassRefPtr<Widget> createJavaAppletWidget(const IntSize&, HTMLAppletElement*, const KURL&, const WTF::Vector<String, 0u>&, const WTF::Vector<String, 0u>&) { notImplemented(); return 0; } Ditto.

Created attachment 126818 [details] Patch Update the patch based on Rob's comments. Local diff is here webkit/0c604f46e7254a45d3c4217e5c3233565f50208d

Comment on attachment 126818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126818&action=review Looks good, still a few things to fix. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:52 > +#include "NotImplemented.h" Not needed, included in header. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:56 > +#include "RenderEmbeddedObject.h" You don't need this one. Please check all other includes as well. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:329 > + return pluginView; No need for temp var pluginView. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:375 > + // received, even for the case of a document with no data (like about:blank) Add a period to the end of line to make it a proper sentence. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:386 > + // (unless it already has a token, in which case the request came from the client) Ditto. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:434 > + true); /*lock the mode*/ Might as well try to align the comments. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:523 > + // Whitelist all known protocols if BrowserField2 requires unrestricted requests. What is BrowserField2? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:553 > + // SubstituteData in dispatchDidFailProvisionalLoad) Add a period to the end of line to make it a proper sentence. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:887 > + // Provide the extension object first in case the client or others want to use it Add a period. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:23 > +#include "FormState.h" Maybe you don't need FormState include.

Created attachment 126996 [details] Patch Update the patch based on Rob's comments, filed a PR138431 for the CrossOrigin settings.

Comment on attachment 126996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126996&action=review Almost there, needs a bit more cleaning up. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:72 > +using namespace BlackBerry::WebKit; This means that below you can remove BlackBerry::WebKit usage. So BlackBerry::WebKit::WebSettings becomes just WebSettings for example. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:403 > + // In Frame::createView, Frame's FrameView object is set to 0 and recreated. one space too much between 'and' and 'recreated'. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:427 > + true); /*lock the mode*/ It is a bit cleaner to do this style /* lock the mode */ everywhere here. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:468 > + Remove one empty line here. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:616 > + Remove on empty line. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:691 > + Remove empty line here. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:26 > +#include "Widget.h" This include can probably be removed. Please check whether we need DocumentLoader and Frame includes. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:38 > +class HTMLFormElement; These can be removed since FrameLoaderClient does this as well.

Created attachment 127010 [details] Patch Update the patch based on Rob's comments above.

Comment on attachment 127010 [details] Patch Looks good.

Comment on attachment 127010 [details] Patch Clearing flags on attachment: 127010 Committed r107734: <http://trac.webkit.org/changeset/107734>

All reviewed patches have been landed. Closing bug.

Attachment 126996 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource M ChangeLog A LayoutTests/fast/css/style-scoped/registering-shadowroot-expected.txt A LayoutTests/fast/css/style-scoped/registering-shadowroot.html M LayoutTests/ChangeLog M Source/WebCore/WebCore.exp.in M Source/WebCore/testing/Internals.cpp M Source/WebCore/testing/Internals.h M Source/WebCore/testing/Internals.idl M Source/WebCore/dom/ShadowRootList.h M Source/WebCore/dom/ShadowRootList.cpp M Source/WebCore/dom/Element.cpp M Source/WebCore/dom/Node.h M Source/WebCore/dom/Element.h M Source/WebCore/dom/NodeRareData.h M Source/WebCore/dom/Node.cpp M Source/WebCore/dom/ElementRareData.h M Source/WebCore/ChangeLog M Source/WebCore/html/HTMLStyleElement.cpp M Source/autotools/symbols.filter M Source/WebKit2/ChangeLog M Source/WebKit2/win/WebKit2CFLite.def M Source/WebKit2/win/WebKit2.def r107793 = 2ce77d76874b9ee77991fecb540696485733202a (refs/remotes/origin/master) First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168755 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/wk2/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/css/CSSCalculationValue.cpp Auto-merging Source/WebCore/css/CSSCalculationValue.h Auto-merging Source/WebCore/css/CSSParser.cpp Auto-merging Source/WebKit/mac/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.