Bug 95532 – [EFL] Add proper support for navigator.onLine and associated events

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

The EFL port currently relies on the client application to provide it with the current onLine state.
We should be able to detect and monitor the onLine state without relying on the client application, similarly to what other ports are doing.

[Chris Dumez]

Created attachment 161628 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 161628 [details]
Patch

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

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:49
> +    // Assume that we're offline until proven otherwise.
> +    m_isOnLine = false;

It that really wise?

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:60
> +        if (!syspath || !strcmp(syspath, UdevLoopBackInterfaceSysPath))

When you are already taking assumption that UdevLoo/... is non-null, why not use strncmp() with 2?

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:77
> +      eeze_net_free(static_cast<Eeze_Net*>(data));

why different indentation

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:93
> +    char buffer[4096];
> +    size_t len;

why not initialize len to 4096;?

size_t len = 4096
char buffer[len]

then replace it

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:102
> +            if (nlh->nlmsg_type == RTM_NEWADDR
> +                || nlh->nlmsg_type == RTM_DELADDR) {

why not just keep that on one line?

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:116
> +        int sock = ecore_main_fd_handler_fd_get(m_fdHandler);

socket?

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:120
> +    eeze_shutdown();

what if it is used elsewhere? (I believe it is)

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:147
> +    if (bind(sock, (struct sockaddr *)&addr, sizeof(addr)) == -1) {

wrong styled cast

[Gyuyoung Kim]

Comment on attachment 161628 [details]
Patch

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

> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:89
> +static Eina_Bool readSocketCallback(void* userData, Ecore_Fd_Handler* handler)

We decided to use standard boolean type except for public API.

[Chris Dumez]

Comment on attachment 161628 [details]
Patch

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

>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:89
>> +static Eina_Bool readSocketCallback(void* userData, Ecore_Fd_Handler* handler)
> 
> We decided to use standard boolean type except for public API.

It is a EFL callback, I'm not sure I can use bool but I'll try.

>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:120
>> +    eeze_shutdown();
> 
> what if it is used elsewhere? (I believe it is)

Not a problem. This merely decreases some ref count. I call _init() to increment refcount so I need to call _shutdown() to decrement it as well:
http://docs.enlightenment.org/auto/eeze/group__main.html#ga908dda677760bf318a67428938a07d42

[Chris Dumez]

Comment on attachment 161628 [details]
Patch

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

>>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:89
>>> +static Eina_Bool readSocketCallback(void* userData, Ecore_Fd_Handler* handler)
>> 
>> We decided to use standard boolean type except for public API.
> 
> It is a EFL callback, I'm not sure I can use bool but I'll try.

As I thought, this won't compile. It needs to be Eina_Bool.

[Chris Dumez]

Created attachment 161640 [details]
Patch for landing

Take Kenneth's feedback into consideration.

[Raphael Kubo da Costa (:rakuco)]

Does this work with --no-networkinfo as well?

[Gyuyoung Kim]

(In reply to comment #5)
> (From update of attachment 161628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161628&action=review
> 
> >>> Source/WebCore/platform/network/efl/NetworkStateNotifierEfl.cpp:89
> >>> +static Eina_Bool readSocketCallback(void* userData, Ecore_Fd_Handler* handler)
> >> 
> >> We decided to use standard boolean type except for public API.
> > 
> > It is a EFL callback, I'm not sure I can use bool but I'll try.
> 
> As I thought, this won't compile. It needs to be Eina_Bool.

Yes, I add this case to EFL coding style wiki page. Thanks.
https://trac.webkit.org/wiki/EFLWebKitCodingStyle

[Chris Dumez]

(In reply to comment #7)
> Does this work with --no-networkinfo as well?

Right, it Gamepads and NetworkInfo support is disabled then it won't compile. I'll fix the CMake.

[Chris Dumez]

Created attachment 161649 [details]
Patch for landing

Link unconditionally against Eeze library now.

[Chris Dumez]

Comment on attachment 161649 [details]
Patch for landing

I need to double check something before this lands. I think it may cause a regression.

[Chris Dumez]

Created attachment 161672 [details]
Fixed patch

Fix 2 issues with the first version of the patch
1. use ssize_t instead of size_t for len (used to store return value of recv()). We cannot use size_t since recv() returns -1 when there is nothing to read anymore.
2. Use MSG_DONTWAIT flag for recv() to make the operation non-blocking.

Setting r? again since those are not style changes.

[WebKit Review Bot]

Comment on attachment 161672 [details]
Fixed patch

Clearing flags on attachment: 161672

Committed r127258: <http://trac.webkit.org/changeset/127258>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.