Bug 181079 - navigator.onLine does not work inside service workers
Summary: navigator.onLine does not work inside service workers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 181325
  Show dependency treegraph
 
Reported: 2017-12-21 00:53 PST by Thomas Steiner
Modified: 2018-01-08 06:43 PST (History)
11 users (show)

See Also:


Attachments
Offline snackbar in Chrome (429.45 KB, image/png)
2017-12-21 00:53 PST, Thomas Steiner
no flags Details
Patch (40.66 KB, patch)
2018-01-05 04:13 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (40.77 KB, patch)
2018-01-08 05:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (40.76 KB, patch)
2018-01-08 05:38 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (40.81 KB, patch)
2018-01-08 06:04 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Steiner 2017-12-21 00:53:41 PST
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.
Comment 1 Chris Dumez 2017-12-21 08:38:18 PST
Thank you for the radar. We will investigate.
Comment 2 Radar WebKit Bug Importer 2017-12-21 08:38:55 PST
<rdar://problem/36178606>
Comment 3 Brady Eidson 2017-12-21 08:45:43 PST
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
Comment 4 Chris Dumez 2017-12-21 08:48:57 PST
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.
Comment 5 youenn fablet 2017-12-21 09:17:06 PST
Online was disabled from workers a month or so ago. It will always say offline iirc
Comment 6 Chris Dumez 2017-12-21 09:19:57 PST
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.
Comment 7 Chris Dumez 2017-12-21 12:22:53 PST
<rdar://problem/36164038>
Comment 8 youenn fablet 2018-01-05 04:13:20 PST
Created attachment 330545 [details]
Patch
Comment 9 Build Bot 2018-01-05 04:16:31 PST
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.
Comment 10 Darin Adler 2018-01-07 22:54:04 PST
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?
Comment 11 youenn fablet 2018-01-08 04:29:55 PST
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.
Comment 12 youenn fablet 2018-01-08 05:16:07 PST
Created attachment 330690 [details]
Patch for landing
Comment 13 Build Bot 2018-01-08 05:18:36 PST
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.
Comment 14 youenn fablet 2018-01-08 05:38:24 PST
Created attachment 330695 [details]
Patch for landing
Comment 15 Build Bot 2018-01-08 05:40:46 PST
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.
Comment 16 youenn fablet 2018-01-08 06:04:29 PST
Created attachment 330698 [details]
Patch for landing
Comment 17 Build Bot 2018-01-08 06:08:15 PST
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.
Comment 18 WebKit Commit Bot 2018-01-08 06:43:54 PST
Comment on attachment 330698 [details]
Patch for landing

Clearing flags on attachment: 330698

Committed r226510: <https://trac.webkit.org/changeset/226510>
Comment 19 WebKit Commit Bot 2018-01-08 06:43:56 PST
All reviewed patches have been landed.  Closing bug.