Bug 73632 – Upstream 7 files into WebCore/platform/blackberry

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mary Wu]

Upstream following 7 files into WebCore/platform/blackberry:
PopupMenuBlackBerry.cpp  ScrollbarBlackBerry.cpp       ScrollViewBlackBerry.cpp       SearchPopupMenuBlackBerry.h
PopupMenuBlackBerry.h    ScrollbarThemeBlackBerry.cpp  SearchPopupMenuBlackBerry.cpp

[Mary Wu]

Created attachment 117582 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 117582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117582&action=review

> Source/WebCore/platform/blackberry/PopupMenuBlackBerry.cpp:31
> +    // no-op

These comments don't add anythign.

> Source/WebCore/platform/blackberry/SearchPopupMenuBlackBerry.cpp:52
> +    // FIXME: remove 0 template parameter when this is implemented

Why is the 0 template parameter there in the first place?

[Mary Wu]

(In reply to comment #2)
> (From update of attachment 117582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117582&action=review
> 
> > Source/WebCore/platform/blackberry/PopupMenuBlackBerry.cpp:31
> > +    // no-op
> 
> These comments don't add anythign.
> 
> > Source/WebCore/platform/blackberry/SearchPopupMenuBlackBerry.cpp:52
> > +    // FIXME: remove 0 template parameter when this is implemented
> 
> Why is the 0 template parameter there in the first place?

ok, thanks. Will update...

[Mary Wu]

Created attachment 117822 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117822 [details]
Patch

Attachment 117822 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10734542

New failing tests:
svg/custom/linking-uri-01-b.svg

[Mary Wu]

(In reply to comment #5)
> (From update of attachment 117822 [details])
> Attachment 117822 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10734542
> 
> New failing tests:
> svg/custom/linking-uri-01-b.svg

it's blackberry specific porting, shouldn't impact chrome test/build

[Daniel Bates]

Comment on attachment 117822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117822&action=review

> Source/WebCore/platform/blackberry/PopupMenuBlackBerry.cpp:33
> +void PopupMenuBlackBerry::show(const IntRect& r, FrameView* v, int index)

All of these parameters are unused. Either we need to use UNUSED_PARAM() or we need to omit them from the signature here or this may cause unused variable warnings depending on the compiler warning level.

> Source/WebCore/platform/blackberry/ScrollViewBlackBerry.cpp:22
> +namespace WebCore {

Why do we need to commit this almost empty file?

> Source/WebCore/platform/blackberry/ScrollbarBlackBerry.cpp:22
> +namespace WebCore {

Ditto.

[Mary Wu]

Created attachment 117854 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117854 [details]
Patch

Attachment 117854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10703592

New failing tests:
svg/custom/linking-uri-01-b.svg

[Rob Buis]

Comment on attachment 117854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117854&action=review

Looks good, nearly there, could be improved a bit, so r-.

> Source/WebCore/platform/blackberry/PopupMenuBlackBerry.h:23
> +#include "PopupMenuClient.h"

I think a forward reference for PopupMenuClient should do.

> Source/WebCore/platform/blackberry/ScrollbarThemeBlackBerry.cpp:29
> +    static ScrollbarTheme theme;

Can DEFINE_STATIC_LOCAL be used here?

[Mary Wu]

Created attachment 117979 [details]
Patch

[Rob Buis]

Comment on attachment 117979 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 117979 [details]
Patch

Clearing flags on attachment: 117979

Committed r102249: <http://trac.webkit.org/changeset/102249>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.