Upstream PageClientBlackBerry.h into WebCore/platform/blackberry

• Component: Platform
• Status: RESOLVED FIXED

[Mary Wu]

Upstream GeolocationService/PageClient into WebCore/platform/blackberry and small style fix of DragDataBlackBerry.cpp

[Mary Wu]

Created attachment 118540 [details]
Patch

[Leo Yang]

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.

[Mary Wu]

ok, I will split another bug then...

[Mary Wu]

Created attachment 118544 [details]
Patch

[Mary Wu]

(In reply to comment #3)
> ok, I will split another bug then...

https://bugs.webkit.org/show_bug.cgi?id=74171

[Leo Yang]

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'.

[Mary Wu]

Created attachment 118553 [details]
Patch

[Rob Buis]

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

[Mary Wu]

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

[Mary Wu]

Created attachment 118722 [details]
Patch

[Rob Buis]

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.

[Mary Wu]

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

[Mary Wu]

Created attachment 119132 [details]
Patch

[Rob Buis]

Comment on attachment 119132 [details]
Patch

LGTM!

[Daniel Bates]

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>.

[Daniel Bates]

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

[Mary Wu]

> 
> > 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?

[Mary Wu]

Created attachment 119145 [details]
Patch

[Leo Yang]

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.

[Mary Wu]

change title since the content changed now.

[Daniel Bates]

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;
}

[Mary Wu]

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

[Mary Wu]

(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.

[Mary Wu]

Created attachment 120143 [details]
Patch

[WebKit Review Bot]

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.

[Daniel Bates]

Comment on attachment 120143 [details]
Patch

Thank you Mary.

[WebKit Review Bot]

Comment on attachment 120143 [details]
Patch

Clearing flags on attachment: 120143

Committed r103393: <http://trac.webkit.org/changeset/103393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.