navigator.onLine does not work inside service workers

[Thomas Steiner]

Created attachment 330018 [details]
Offline snackbar in Chrome

Steps to reproduce:
1) Go to https://www.trivago.com/las-vegas-34741/hotel.
2) Refresh to be sure the Service Worker is installed and active.
3) Turn Wi-Fi off. 
4) Reload. 
5) Offline fallback page shows (maze game).

Expected behavior:
After step 3) a "snackbar" UI component "you're offline should show (see Chrome screenshot attached).

This is most probably related to https://bugs.webkit.org/show_bug.cgi?id=32327#c6.

[Chris Dumez]

Thank you for the radar. We will investigate.

[Radar WebKit Bug Importer]

<rdar://problem/36178606>

[Brady Eidson]

We simply haven’t gotten to navigator.online inside the SW context yet.(In reply to 

Radar WebKit Bug Importer from comment #2)
> <rdar://problem/36178606>

We already have a rdar for this, this will need to be duped

[Chris Dumez]

On my machine, with STP46, I do see the "You are offline" banner showing up on the lower left side of the screen at step 3.

[youenn fablet]

Online was disabled from workers a month or so ago. It will always say offline iirc

[Chris Dumez]

As Thomas Steiner commented on https://bugs.webkit.org/show_bug.cgi?id=32327#c6, he has Parallels installed, in which case we always report online (for non service workers).

So there are several issues here. The fact is that even once we support navigator.online inside service workers, it is still not going to work for Thomas due to Bug 32327.

[Chris Dumez]

<rdar://problem/36164038>

[youenn fablet]

Created attachment 330545 [details]
Patch

[EWS Watchlist]

Attachment 330545 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 330545 [details]
Patch

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

> Source/WebCore/testing/ServiceWorkerInternals.h:37
> +    static Ref<ServiceWorkerInternals> create(ServiceWorkerIdentifier identifier) { return adoptRef(*new ServiceWorkerInternals { identifier }); }

For new classes I suggest having the create functions not be inlined; the constructor can be inlined in the create function, which seems better to me.

> Source/WebCore/testing/js/WebCoreTestSupport.cpp:196
> +        auto* execState = globalScope.execState();

In new WebCore code I would suggest:

    auto& state = *globalScope.execState();

Over time we we always be using references for this, not pointers.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:101
> +    auto addResult = allServiceWorkerThreadProxies().add(this);
> +    ASSERT_UNUSED(addResult, addResult.isNewEntry);

Since this is for debugging only, maybe assert !contains and do the assertion before the add rather than using the add result?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:112
> +    allServiceWorkerThreadProxies().remove(this);

Maybe assert contains here?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:69
> +    // Public only for testing purposes.
> +    WEBCORE_EXPORT void notifyNetworkStateChange(bool isOnline);

Is this what WEBCORE_TESTSUPPORT_EXPORT is for? Or is that something else?

[youenn fablet]

Thanks for the review.

(In reply to Darin Adler from comment #10)
> Comment on attachment 330545 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330545&action=review
> 
> > Source/WebCore/testing/ServiceWorkerInternals.h:37
> > +    static Ref<ServiceWorkerInternals> create(ServiceWorkerIdentifier identifier) { return adoptRef(*new ServiceWorkerInternals { identifier }); }
> 
> For new classes I suggest having the create functions not be inlined; the
> constructor can be inlined in the create function, which seems better to me.

What is the reason behind this suggestion?
To limit the number of #include in headers, it would be better to keep create inlined and constructor not be inlined.

> > Source/WebCore/testing/js/WebCoreTestSupport.cpp:196
> > +        auto* execState = globalScope.execState();
> 
> In new WebCore code I would suggest:
> 
>     auto& state = *globalScope.execState();
> 
> Over time we we always be using references for this, not pointers.

OK

> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:101
> > +    auto addResult = allServiceWorkerThreadProxies().add(this);
> > +    ASSERT_UNUSED(addResult, addResult.isNewEntry);
> 
> Since this is for debugging only, maybe assert !contains and do the
> assertion before the add rather than using the add result?

OK, I guess this is better for clarity.

> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:112
> > +    allServiceWorkerThreadProxies().remove(this);
> 
> Maybe assert contains here?

OK

> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:69
> > +    // Public only for testing purposes.
> > +    WEBCORE_EXPORT void notifyNetworkStateChange(bool isOnline);
> 
> Is this what WEBCORE_TESTSUPPORT_EXPORT is for? Or is that something else?

Will look at it.

[youenn fablet]

Created attachment 330690 [details]
Patch for landing

[EWS Watchlist]

Attachment 330690 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 330695 [details]
Patch for landing

[EWS Watchlist]

Attachment 330695 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 330698 [details]
Patch for landing

[EWS Watchlist]

Attachment 330698 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:74:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 330698 [details]
Patch for landing

Clearing flags on attachment: 330698

Committed r226510: <https://trac.webkit.org/changeset/226510>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.