Implement most of ServiceWorkerContainer::addRegistration

[Brady Eidson]

Implement most of ServiceWorkerContainer::addRegistration

Up to - but not including - actually posting the job to the job queue.

Seems silly, except it lays a lot of infrastructure to make future work go quicker.

[Brady Eidson]

Created attachment 317331 [details]
WIP for EWS

[Build Bot]

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


ERROR: Source/WebCore/workers/ServiceWorkerContainer.cpp:92:  %, [, (, and { are undefined character escapes.  Unescape them.  [build/printf_format] [3]
Total errors found: 1 in 22 files


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

[Brady Eidson]

Created attachment 317332 [details]
WIP for EWS

[Build Bot]

Comment on attachment 317332 [details]
WIP for EWS

Attachment 317332 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4259696

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317337 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317332 [details]
WIP for EWS

Attachment 317332 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4259719

New failing tests:
fast/history/page-cache-geolocation.html
fast/history/page-cache-geolocation-active-watcher.html
fast/loader/window-properties-restored-from-page-cache.html
fast/history/page-cache-geolocation-active-oneshot.html

[Build Bot]

Created attachment 317338 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317332 [details]
WIP for EWS

Attachment 317332 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4259686

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317339 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317332 [details]
WIP for EWS

Attachment 317332 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4259729

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317340 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Brady Eidson]

It definitely has to be an ActiveDOMObject, gotta be much smarted about the canSuspendForDocumentSuspension return value in the future. For now it can be "true" universally.

[Brady Eidson]

Created attachment 317345 [details]
WIP for EWS

[Build Bot]

Comment on attachment 317345 [details]
WIP for EWS

Attachment 317345 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4260643

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317347 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317345 [details]
WIP for EWS

Attachment 317345 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4260636

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317348 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317345 [details]
WIP for EWS

Attachment 317345 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4260677

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317349 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Brady Eidson]

Created attachment 317350 [details]
WIP for EWS

[Build Bot]

Comment on attachment 317350 [details]
WIP for EWS

Attachment 317350 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4260974

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317351 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317350 [details]
WIP for EWS

Attachment 317350 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4261004

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317352 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Brady Eidson]

Created attachment 317356 [details]
WIP for EWS

[Brady Eidson]

Created attachment 317357 [details]
Patch

[youenn fablet]

Comment on attachment 317357 [details]
Patch

LGTM overall.
Main question is why ServiceWorkerContainer needs to be ActiveDOMObject.
It is not clear from this patch but I guess there is a reason.

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

> Source/WebCore/page/NavigatorBase.cpp:81
> +    m_serviceWorkerContainer = ServiceWorkerContainer::create(context, *this);

It seems to be better to create m_serviceWorkerContainer when needed.
Cannot we use CallWith in the getter?

If we need to create it here, we should move to a unique ref instead of a unique ptr.

> Source/WebCore/page/NavigatorBase.h:58
> +    NavigatorBase(ScriptExecutionContext&);

explicit.

> Source/WebCore/page/WorkerNavigator.cpp:34
> +    , m_userAgent(userAgent)

userAgent should be a String&&

> Source/WebCore/page/WorkerNavigator.h:41
> +    explicit WorkerNavigator(ScriptExecutionContext&, const String&);

No longer need for explicit if there is really a need for ScriptExecutionContext& as a parameter.

> Source/WebCore/workers/ServiceWorkerContainer.cpp:68
> +    auto* context = scriptExecutionContext();

Can we use CallWith=ScriptExecutionContext that will provide a ScriptExecutionContext& directly?

> Source/WebCore/workers/ServiceWorkerContainer.cpp:75
> +        promise->reject(Exception(TypeError, ASCIILiteral("serviceWorker.register() cannot be called with a null script URL")));

I like error messages with messages like done here!
Other code usually use "Exception { }" instead of Exception().

> Source/WebCore/workers/ServiceWorkerContainer.cpp:97
> +    String scope = options.scope.isEmpty() ? "./" : options.scope;

ASCIILiteral?

> Source/WebCore/workers/ServiceWorkerContainer.h:42
> +class ServiceWorkerContainer final : public EventTargetWithInlineData, public ActiveDOMObject {

It is not very clear to me why ServiceWorkerContainer should be an ActiveDOMObject but I guess it is fine.

> Source/WebCore/workers/ServiceWorkerContainer.h:48
> +    explicit ServiceWorkerContainer(ScriptExecutionContext&, NavigatorBase&);

No longer need for explicit.

> Source/WebCore/workers/ServiceWorkerRegistration.cpp:55
> +    return UpdateViaCache::None;

Probably not a big deal but initial value is imports according spec.

[Brady Eidson]

Points I don't reply to I address as a given with no further explanation.

(In reply to youenn fablet from comment #27)
> Comment on attachment 317357 [details]
> Patch
> 
> LGTM overall.
> Main question is why ServiceWorkerContainer needs to be ActiveDOMObject.
> It is not clear from this patch but I guess there is a reason.

It is the object that will be the decider of "is there SW activity going on in this Document's context that would prevent Document suspension"

(That will be coming up next in https://bugs.webkit.org/show_bug.cgi?id=175241

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317357&action=review
> 
> > Source/WebCore/page/NavigatorBase.cpp:81
> > +    m_serviceWorkerContainer = ServiceWorkerContainer::create(context, *this);
> 
> It seems to be better to create m_serviceWorkerContainer when needed.
> Cannot we use CallWith in the getter?

I believe "CallWith=Context" gives you the active context of the script making the call, not the context of the DOMWindow whose Navigator owns this ServiceWorkerContainer.

Assuming I'm right about that (which I think I am?) we need to capture the context of the actual DOMWindow/WorkerGlobalScope the container belongs to at creation.

> If we need to create it here, we should move to a unique ref instead of a
> unique ptr.

Good call.

> > Source/WebCore/page/WorkerNavigator.h:41
> > +    explicit WorkerNavigator(ScriptExecutionContext&, const String&);
> 
> No longer need for explicit if there is really a need for
> ScriptExecutionContext& as a parameter.
> 
> > Source/WebCore/workers/ServiceWorkerContainer.cpp:68
> > +    auto* context = scriptExecutionContext();
> 
> Can we use CallWith=ScriptExecutionContext that will provide a
> ScriptExecutionContext& directly?

See my reply above.

I believe this is the normal trap we fall into with other DOMWindow et.al. calls in confusing "the context this object belongs to" with "the context from which the JS call is being made", which are not necessarily the same.
 
> > Source/WebCore/workers/ServiceWorkerContainer.cpp:75
> > +        promise->reject(Exception(TypeError, ASCIILiteral("serviceWorker.register() cannot be called with a null script URL")));
> 
> I like error messages with messages like done here!
> Other code usually use "Exception { }" instead of Exception().

New other code sure does. I missed the memo. Is that actually preferred style now? I though we preferred brace construction only when the type can definitely be inferred.

> > Source/WebCore/workers/ServiceWorkerContainer.h:42
> > +class ServiceWorkerContainer final : public EventTargetWithInlineData, public ActiveDOMObject {
> 
> It is not very clear to me why ServiceWorkerContainer should be an
> ActiveDOMObject but I guess it is fine.

See above.

In this patch it definitely could simply be a ContextDestructionObserver, but coming very soon (e.g. next patch) it will be managing a live queue of tasks that directly relate to Document suspension.

> > Source/WebCore/workers/ServiceWorkerRegistration.cpp:55
> > +    return UpdateViaCache::None;
> 
> Probably not a big deal but initial value is imports according spec.

This particular line is truly about stubbing, but you're right.

[Brady Eidson]

(In reply to youenn fablet from comment #27)
> Comment on attachment 317357 [details]
> Patch
> ...
> 
> > Source/WebCore/page/WorkerNavigator.cpp:34
> > +    , m_userAgent(userAgent)
> 
> userAgent should be a String&&

This one turned out to be *not* true.

[Brady Eidson]

Created attachment 317401 [details]
Patch

[Andy Estes]

Comment on attachment 317401 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27513
> +				51F174FE1F35899200C74950 /* WorkerType.h in Headers */,

This list looked sorted before you added WorkerType.h.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27602
> +				51F174FF1F35899700C74950 /* ServiceWorkerUpdateViaCache.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28551
> +				51F175061F358BF700C74950 /* JSWorkerType.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28834
> +				51F175031F358B3B00C74950 /* JSServiceWorkerUpdateViaCache.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:32210
> +				51F175071F358BF900C74950 /* JSWorkerType.cpp in Sources */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:33848
> +				51F175021F358B3B00C74950 /* JSServiceWorkerUpdateViaCache.cpp in Sources */,

Ditto.

> Source/WebCore/page/NavigatorBase.h:66
> -    std::unique_ptr<ServiceWorkerContainer> m_serviceWorkerContainer;
> +    UniqueRef<ServiceWorkerContainer> m_serviceWorkerContainer;

Should this be guarded with an #if ENABLE(SERVICE_WORKER)?

> Source/WebCore/workers/ServiceWorkerContainer.cpp:63
> +    promise->reject(Exception(UnknownError, ASCIILiteral("serviceWorker.ready() is not yet implemented")));

I like how you use brace initialization for the Exception in addRegistration().

> Source/WebCore/workers/ServiceWorkerContainer.cpp:75
> +        promise->reject(Exception { TypeError, ASCIILiteral("serviceWorker.register() cannot be called with a null script URL") });

You say "null script URL" in the error message, but you check for an empty or null URL.

[Brady Eidson]

(In reply to Andy Estes from comment #31)
> Comment on attachment 317401 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317401&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27513
> > +				51F174FE1F35899200C74950 /* WorkerType.h in Headers */,
> 
> This list looked sorted before you added WorkerType.h.

Regarding all of these: In the parts of the project that show up in the UI they are alphabetized.
In the rest of the project file, Xcode doesn't try to alphabetize.

> 
> > Source/WebCore/page/NavigatorBase.h:66
> > -    std::unique_ptr<ServiceWorkerContainer> m_serviceWorkerContainer;
> > +    UniqueRef<ServiceWorkerContainer> m_serviceWorkerContainer;
> 
> Should this be guarded with an #if ENABLE(SERVICE_WORKER)?

It is!

> > Source/WebCore/workers/ServiceWorkerContainer.cpp:63
> > +    promise->reject(Exception(UnknownError, ASCIILiteral("serviceWorker.ready() is not yet implemented")));
> 
> I like how you use brace initialization for the Exception in
> addRegistration().
> 
> > Source/WebCore/workers/ServiceWorkerContainer.cpp:75
> > +        promise->reject(Exception { TypeError, ASCIILiteral("serviceWorker.register() cannot be called with a null script URL") });
> 
> You say "null script URL" in the error message, but you check for an empty
> or null URL.

👍

[Brady Eidson]

Created attachment 317420 [details]
PFL

[Brady Eidson]

Created attachment 317421 [details]
PFL

[WebKit Commit Bot]

Comment on attachment 317421 [details]
PFL

Rejecting attachment 317421 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 317421, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0-ab3c-d52691b4dbfc ...
Currently at 220334 = f709d2127e0549b7123f04882043bb4b5825e026
r220335 = 69d38d63e6c2256d07edefc13a2a8d7e86fe6bba
r220336 = 603851053c1797ceae110d4ecd8ec23585e12f23
r220337 = c158e17d784e3056240cef3cc20a7039c826ac5e
r220342 = 4530f04e017f6f530e1dae4d9b6c70213acb9e65
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/4270641

[Brady Eidson]

Created attachment 317437 [details]
PFL again...

[WebKit Commit Bot]

Comment on attachment 317437 [details]
PFL again...

Rejecting attachment 317437 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 317437, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
rdparty/autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://webkit-queues.webkit.org/results/4271045

[Ryan Haddad]

Despite the commit bot error, it appears that this landed in https://trac.webkit.org/changeset/220344/webkit

[Radar WebKit Bug Importer]

<rdar://problem/33808422>