[Webkit-unassigned] [Bug 73397] [BlackBerry] Upstream BlackBerry porting of pluginView
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 30 20:06:32 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=73397
Daniel Bates <dbates at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #117311|review? |review-
Flag| |
--- Comment #6 from Daniel Bates <dbates at webkit.org> 2011-11-30 20:06:32 PST ---
(From update of attachment 117311)
View in context: https://bugs.webkit.org/attachment.cgi?id=117311&action=review
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:33
> +#include "Element.h"
This include is unnecessary as HTMLPlugInElement.h (included below) includes HTMLFrameOwnerElement.h => HTMLElement.h => StyledElement.h, which includes this file.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:38
> +#include "GraphicsContext.h"
This include is unnecessary as PlatformContextSkia.h (included below) includes this file.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:42
> +#include "Image.h"
This file is unnecessary as PlatformContextSkia.h (included below) includes GraphicsContext.h, which includes this file.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:57
> +#include "Touch.h"
This file is unnecessary as TouchList.h (below) includes this file.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:63
> +#include <BlackBerryPlatformWindow.h>
We should move this include to after line 73 so that it's grouped with the other BlackBerry platform headers.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:81
> +#define UNINIT_COORD 0xffffffff
This should be a C++ constant and shouldn't have an underscore in its name:
const unsigned UninitializedCoordinate = 0xffffffff;
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:103
> +static NPRect toNPRect(const IntRect& rect)
> +{
> + NPRect npRect;
> + npRect.top = rect.y();
> + npRect.left = rect.x();
> + npRect.bottom = rect.y() + rect.height();
> + npRect.right = rect.x() + rect.width();
> + return npRect;
> +}
> +
> +static NPRect toNPRect(const BlackBerry::Platform::IntRect& rect)
> +{
> + NPRect npRect;
> + npRect.top = rect.y();
> + npRect.left = rect.x();
> + npRect.bottom = rect.y() + rect.height();
> + npRect.right = rect.x() + rect.width();
> + return npRect;
> +}
This is OK AS-IS. That being said, these functions are identical up to the datatype of their argument. Can we make this a template function?
Also, the name of this function doesn't conform to the WebKit Code Style Guidelines per (8) of section Names. We can rename this function in separate bug.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:106
> +// Standard plugin widget stuff.
Remove this comment. It's inane.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:146
> + // do not call setNPWindowIfNeeded immediately, will be called on paint()
Nit: do => Do
Also, this comment is missing a period.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:149
> + // (i) in order to move/resize the plugin window at the same time as the
Nit: in => In
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:154
> + // (ii) if we are running layout tests from DRT, paint() won't ever get called
if => If
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:155
> + // so we need to call setNPWindowIfNeeded() if window geometry has changed
Missing a period at the end of this sentence.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:312
> + double zoomFactorH = (double)m_private->m_pluginBufferSize.width() / (double)frameRect().width();
> + double zoomFactorW = (double)m_private->m_pluginBufferSize.height() / (double)frameRect().height();
Nit: We should use C++ style casts.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:428
> + // sanity check.
Remove this comment. It's inane. I take it this "sanity check" is unusual to do since we have this comment. It would have been better if the comment explained why? Regardless, we should just remove this comment.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:470
> + if (platformKeyEvent->type() == PlatformKeyboardEvent::RawKeyDown
> + || platformKeyEvent->type() == PlatformKeyboardEvent::KeyDown) {
Nit: The indentation here is off. The "||" should be left-aligned with the 'p' in platformKeyEvent on line 469.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:532
> + return;
This looks weird. We don't need to fix this now, but we should eventually come back to this code. What other mouse events are there? Does control ever reach here? Maybe this should be an ASSERT_NOT_REACHED().
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:545
> + npTouchEvent.points = new NPTouchPoint[touchList->length()];
Can we use OwnArrayPtr here? Then we can remove the delete[] on line 576.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:951
> +void PluginView::setNPWindowRect(const IntRect& rc)
This argument is unused and depending on our compiler warning level may raise a warning. We should just remove the argument name.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:987
> + // FLASH WORKAROUND: Only set initially. Multiple calls to
> + // setNPWindow() cause the plugin to crash in windowed mode.
This should read:
// Only set the width and height of the plugin content the first time setNPWindow() is called so as to workaround
// an issue in Flash where multiple calls to setNPWindow() cause the plugin to crash in windowed mode.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1000
> + BlackBerry::Platform::Graphics::Window *window =
> + frameView->hostWindow()->platformPageClient()->platformWindow();
I would write this on one line to improve readability.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1007
> + // FIXME: Passing zoomFactor to setwindow make windowed plugin scale incorrectly.
Nit: I suggest adding an empty line above this FIXME comment so as to make it stand out.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1013
> + setCallingPlugin(false);
Nit: I suggest adding a empty line above this line so as to demarcate the end of the zoom factor code above.
We should define a RAII object that calls setCallingPlugin(true) in its constructor and calls setCallingPlugin(false) in its destructor. We can do this in a follow up patch.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1025
> + // since the window manager should take care of that for us.
Nit: Remove the leading whitespace (before the word "since") in this comment such that there is exactly one space after the "//" in this line.
> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1039
> + FILE* fileHandle = fopen((filename.utf8()).data(), "r");
Nit: (filename.utf8()) => filename.utf8()
That is, the parentheses around "filename.utf8()" aren't necessary.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list