Bug 78275 – [BlackBerry] Upstream BlackBerry WebCoreSupport FrameLoaderClientBlackBerry class

• Component: New Bugs
• Status: RESOLVED FIXED

[Jacky Jiang]

Upstream BlackBerry WebCoreSupport FrameLoaderClientBlackBerry class.

[Jacky Jiang]

Created attachment 126578 [details]
Patch

Note: Local self reviewed diff is here webkit/ad907cc8b91037c255883c2cf3e8c34ffe13aa3c

[Rob Buis]

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.

[Jacky Jiang]

Created attachment 126604 [details]
Patch

Update the patch based on Rob's comments.

[Rob Buis]

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.

[Jacky Jiang]

Created attachment 126818 [details]
Patch

Update the patch based on Rob's comments. Local diff is here webkit/0c604f46e7254a45d3c4217e5c3233565f50208d

[Rob Buis]

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.

[Jacky Jiang]

Created attachment 126996 [details]
Patch

Update the patch based on Rob's comments, filed a PR138431 for the CrossOrigin settings.

[Rob Buis]

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.

[Jacky Jiang]

Created attachment 127010 [details]
Patch

Update the patch based on Rob's comments above.

[Rob Buis]

Comment on attachment 127010 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 127010 [details]
Patch

Clearing flags on attachment: 127010

Committed r107734: <http://trac.webkit.org/changeset/107734>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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.