Created attachment 118540 [details] Patch

Comment on attachment 118540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118540&action=review Informal review. I only looked at I commented part and feel that you shouldn't mix style fix with you newly upstreamed files. > Source/WebCore/ChangeLog:35 > + * platform/blackberry/DragDataBlackBerry.cpp: Small style fix. > + (WebCore::DragData::containsURL): > + (WebCore::DragData::asFilenames): > + (WebCore::DragData::asURL): > + (WebCore::DragData::asFragment): > + * platform/blackberry/GeolocationServiceBlackBerry.cpp: Added. > + (WebCore::GeolocationServiceBlackBerry::create): > + (WebCore::GeolocationServiceBlackBerry::GeolocationServiceBlackBerry): > + (WebCore::GeolocationServiceBlackBerry::~GeolocationServiceBlackBerry): > + (WebCore::GeolocationServiceBlackBerry::startUpdating): > + (WebCore::GeolocationServiceBlackBerry::stopUpdating): > + (WebCore::GeolocationServiceBlackBerry::suspend): > + (WebCore::GeolocationServiceBlackBerry::resume): > + (WebCore::GeolocationServiceBlackBerry::lastPosition): > + (WebCore::GeolocationServiceBlackBerry::lastError): > + (WebCore::GeolocationServiceBlackBerry::onLocationUpdate): > + (WebCore::GeolocationServiceBlackBerry::onLocationError): > + (WebCore::GeolocationServiceBlackBerry::onPermission): > + (WebCore::GeolocationServiceBlackBerry::deferNotifications): > + (WebCore::GeolocationServiceBlackBerry::resumeNotifications): > + * platform/blackberry/GeolocationServiceBlackBerry.h: Added. > + (WebCore::GeolocationServiceBlackBerry::tracker): > + (WebCore::GeolocationServiceBlackBerry::hasDeferredNotifications): Could you remove the information for each function since they are newly added? > Source/WebCore/platform/blackberry/DragDataBlackBerry.cpp:71 > return false; > } > > -bool DragData::containsURL(Frame*, FilenameConversionPolicy filenamePolicy) const > +bool DragData::containsURL(Frame*, FilenameConversionPolicy) const > { > notImplemented(); > return false; > } > > -void DragData::asFilenames(WTF::Vector<String, 0u>& result) const > +void DragData::asFilenames(Vector<String>&) const > { > - // FIXME: remove explicit 0 size in result template once this is implemented > notImplemented(); > } > I would use a separate patch to do this. > Source/WebCore/platform/blackberry/DragDataBlackBerry.cpp:91 > -String DragData::asURL(Frame*, FilenameConversionPolicy filenamePolicy, String* title) const > +String DragData::asURL(Frame*, FilenameConversionPolicy, String*) const > { > notImplemented(); > return String(); > } > > -WTF::PassRefPtr<DocumentFragment> DragData::asFragment(Frame*, PassRefPtr<Range> context, > - bool allowPlainText, bool& chosePlainText) const > +PassRefPtr<DocumentFragment> DragData::asFragment(Frame*, PassRefPtr<Range>, bool, bool&) const > { Ditto.

ok, I will split another bug then...

Created attachment 118544 [details] Patch

(In reply to comment #3) > ok, I will split another bug then... https://bugs.webkit.org/show_bug.cgi?id=74171

Comment on attachment 118544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118544&action=review Informal review. > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.h:74 > +#endif You could add ' // GeolocationServiceBlackBerry_h' after '#endif'. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:65 > +#endif You could add ' // PageClientBlackBerry_h' after '#endif'.

Created attachment 118553 [details] Patch

Comment on attachment 118553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118553&action=review Mostly just some nits left. > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:106 > + true, // providesAltititude providesAltititude? > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:112 > + 0.0, // heading 0.0 -> 0 > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:114 > + 0.0); // speed 0.0 -> 0 > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:45 > + virtual void setPreventsScreenDimming(bool preventDimming) = 0; preventDimming is obvious, can be removed. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:46 > + virtual void showVirtualKeyboard(bool showKeyboard) = 0; showKeyboard is obvious, can be removed. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:61 > + virtual int showAlertDialog(BlackBerry::WebKit::WebPageClient::AlertType atype) = 0; Remove atype

All accepts excepte the first one: > > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:106 > > + true, // providesAltititude > > providesAltititude? > that follows interface naming in the header file page/Coordinates.h

Created attachment 118722 [details] Patch

Comment on attachment 118722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118722&action=review Some nits left, nearly there :) > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:48 > + m_geolocation->frame()->loader()->client()->didCreateGeolocation(m_geolocation); It seems safer to test for m_geolocation->frame() being not null, like in GeolocationServiceBlackBerry::startUpdating. > Source/WebCore/platform/blackberry/GeolocationServiceBlackBerry.cpp:106 > + true, // providesAltititude This is not a valid word, it should be "providesAltitude". > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:51 > + virtual bool shouldPluginEnterFullScreen(WebCore::PluginView*, const char*) = 0; The second param name should be provided, I can't tell what it is doing without the name. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:52 > + virtual void didPluginEnterFullScreen(WebCore::PluginView*, const char*) = 0; Ditto. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:53 > + virtual void didPluginExitFullScreen(WebCore::PluginView*, const char*) = 0; Ditto. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:54 > + virtual void onPluginStartBackgroundPlay(WebCore::PluginView*, const char*) = 0; Ditto. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:55 > + virtual void onPluginStopBackgroundPlay(WebCore::PluginView*, const char*) = 0; Ditto.

remove GeolocationServiceBlackBerry.cpp/h from the patch since they're dead code and we used different geo location client as confirmed with David Tapuska.

Created attachment 119132 [details] Patch

Comment on attachment 119132 [details] Patch LGTM!

Comment on attachment 119132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119132&action=review > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:28 > +namespace Graphics { Maybe adding an empty line about this line will make this namespace directive stand out a bit more from the forward declaration of the class. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:29 > +class Window; This should be indented per (3) of Indentation of <http://www.webkit.org/coding/coding-style.html>. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:56 > + virtual bool shouldPluginEnterFullScreen(WebCore::PluginView*, const char* pluginUniquePrefix) = 0; > + virtual void didPluginEnterFullScreen(WebCore::PluginView*, const char* pluginUniquePrefix) = 0; > + virtual void didPluginExitFullScreen(WebCore::PluginView*, const char* pluginUniquePrefix) = 0; > + virtual void onPluginStartBackgroundPlay(WebCore::PluginView*, const char* pluginUniquePrefix) = 0; > + virtual void onPluginStopBackgroundPlay(WebCore::PluginView*, const char* pluginUniquePrefix) = 0; > + virtual bool lockOrientation(bool landscape) = 0; pluginUniquePrefix => uniquePluginPrefix We tend to place the adjective/verb before the noun in names. This follows from (8) of section Names of <http://www.webkit.org/coding/coding-style.html>.

Comment on attachment 119132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119132&action=review >> Source/WebCore/platform/blackberry/PageClientBlackBerry.h:28 >> +namespace Graphics { > > Maybe adding an empty line about this line will make this namespace directive stand out a bit more from the forward declaration of the class. *above

> > > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:29 > > +class Window; > > This should be indented per (3) of Indentation of <http://www.webkit.org/coding/coding-style.html>. > it can't pass style check script if indent. As per coding-style, it means the content inside class Window should be indent. "class Window" belongs to "The contents of an outermost namespace block", so needn't indent?

Created attachment 119145 [details] Patch

Comment on attachment 119145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119145&action=review > Source/WebCore/ChangeLog:3 > + Upstream PageClientBlackBerry.h into WebCore/platform/blackberry This doesn't match the bug title.

change title since the content changed now.

Comment on attachment 119145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119145&action=review > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:34 > +namespace BlackBerry { > +namespace Platform { > +class NetworkStreamFactory; > + > +namespace Graphics { > +class Window; > +} > + > +} > +} From talking with Dave Levin today on IRC, this should be written as: namespace BlackBerry { namespace Platform { class NetworkStreamFactory; namespace Graphics { class Window; } } } This formatting makes the forward declarations stand out since they are indented. Some additional remarks on this formatting are mentioned in the description and comments of bug #36760. The comments in bug #36760 and the discussion from Dave Levin seemed to imply that the remarks on namespace indentation in <http://www.webkit.org/coding/coding-style.html> pertain to a namespace that contains class/struct definitions. That is, the code style guidelines don't pertain to a namespace that only lists forward declarations (as is the case here). We should look to clarify this on webkit-dev. > Source/WebCore/platform/blackberry/PageClientBlackBerry.h:40 > +namespace WebCore { > +class IntRect; > +class IntSize; > +class PluginView; > +} Similarly, since this namespace only lists forward declarations it should be written as: namespace WebCore { class IntRect; class IntSize; class PluginView; }

Thanks for your clear explain, Daniel. yes, I strongly suggest webkit coding-style page be updated to add this rule. update package loaded again...

(In reply to comment #22) > Thanks for your clear explain, Daniel. yes, I strongly suggest webkit coding-style page be updated to add this rule. > update package loaded again... Just found bug #36760 was very old and still in "NEW" status, so can't pass style-check script if change the patch...I prefer to let it be and change the style after the bug #36760 be resolved.

Created attachment 120143 [details] Patch

Attachment 120143 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/blackberry/PageClientBlackBerry.h:26: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 120143 [details] Patch Thank you Mary.

Comment on attachment 120143 [details] Patch Clearing flags on attachment: 120143 Committed r103393: <http://trac.webkit.org/changeset/103393>

All reviewed patches have been landed. Closing bug.