Bug 175237

Created attachment 317331 [details] WIP for EWS

Created attachment 317332 [details] WIP for EWS

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.

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

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

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

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.

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

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.

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

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.

Created attachment 317345 [details] WIP for EWS

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.

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

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.

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

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.

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

Created attachment 317350 [details] WIP for EWS

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.

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

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.

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

Created attachment 317356 [details] WIP for EWS

Created attachment 317357 [details] Patch

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.

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.

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

Created attachment 317401 [details] Patch

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.

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

Created attachment 317420 [details] PFL

Created attachment 317421 [details] PFL

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

Created attachment 317437 [details] PFL again...

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

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

<rdar://problem/33808422>