Bug 73632 - Upstream 7 files into WebCore/platform/blackberry
Summary: Upstream 7 files into WebCore/platform/blackberry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-12-01 23:58 PST by Mary Wu
Modified: 2011-12-07 10:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.04 KB, patch)
2011-12-02 00:02 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (12.87 KB, patch)
2011-12-04 18:42 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (11.07 KB, patch)
2011-12-05 01:08 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (11.12 KB, patch)
2011-12-05 19:27 PST, Mary Wu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mary Wu 2011-12-01 23:58:46 PST
Upstream following 7 files into WebCore/platform/blackberry:
PopupMenuBlackBerry.cpp  ScrollbarBlackBerry.cpp       ScrollViewBlackBerry.cpp       SearchPopupMenuBlackBerry.h
PopupMenuBlackBerry.h    ScrollbarThemeBlackBerry.cpp  SearchPopupMenuBlackBerry.cpp
Comment 1 Mary Wu 2011-12-02 00:02:01 PST
Created attachment 117582 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-12-03 14:20:50 PST
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?
Comment 3 Mary Wu 2011-12-04 18:42:00 PST
(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...
Comment 4 Mary Wu 2011-12-04 18:42:43 PST
Created attachment 117822 [details]
Patch
Comment 5 WebKit Review Bot 2011-12-04 19:31:43 PST
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
Comment 6 Mary Wu 2011-12-04 21:26:59 PST
(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
Comment 7 Daniel Bates 2011-12-05 00:18:37 PST
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.
Comment 8 Mary Wu 2011-12-05 01:08:02 PST
Created attachment 117854 [details]
Patch
Comment 9 WebKit Review Bot 2011-12-05 08:46:00 PST
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
Comment 10 Rob Buis 2011-12-05 09:11:36 PST
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?
Comment 11 Mary Wu 2011-12-05 19:27:28 PST
Created attachment 117979 [details]
Patch
Comment 12 Rob Buis 2011-12-07 08:49:34 PST
Comment on attachment 117979 [details]
Patch

LGTM
Comment 13 WebKit Review Bot 2011-12-07 10:24:53 PST
Comment on attachment 117979 [details]
Patch

Clearing flags on attachment: 117979

Committed r102249: <http://trac.webkit.org/changeset/102249>
Comment 14 WebKit Review Bot 2011-12-07 10:24:58 PST
All reviewed patches have been landed.  Closing bug.