ServiceWorker: Issue a warning after initial execution of worker script

ServiceWorker: Issue a warning after initial execution of worker script

This patch enables the inspector to issue a warning when developers add a event handler after an initial evaluation of the ServiceWorker script.

BUG=401870
TEST=manually

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184227

[shimazu, 2014-10-22 07:00:12]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org, kouhei@chromium.org

[shimazu, 2014-10-22 07:00:13]

horo@chromium.org: Please review changes under modules/serviceworer.

kouhei@chromium.org: Please review changes under core.

[kouhei (in TOK), 2014-10-22 07:28:28]

https://codereview.chromium.org/657213004/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:97: virtual void postInitialize() { };
Implement this in cpp file. It's better to avoid inline virtual unless its under
public/.

Nit: move this to line 85 after dispose()

https://codereview.chromium.org/657213004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:77: virtual bool
addEventListener(const AtomicString& eventType, PassRefPtr<EventListener>, bool
useCapture = false) override;
Don't we need to the same for removeEventListener too?

[shimazu, 2014-10-22 08:33:56]

Thanks for your reviews!

I modified this patch as we chatted offline. Please review again:)

https://codereview.chromium.org/657213004/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:97: virtual void postInitialize() { };
On 2014/10/22 07:28:28, kouhei wrote:
> Implement this in cpp file. It's better to avoid inline virtual unless its
under
> public/.
> 
> Nit: move this to line 85 after dispose()

Done.

https://codereview.chromium.org/657213004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:77: virtual bool
addEventListener(const AtomicString& eventType, PassRefPtr<EventListener>, bool
useCapture = false) override;
On 2014/10/22 07:28:28, kouhei wrote:
> Don't we need to the same for removeEventListener too?

No, I think.
This is because the removed function won't be executed even if the
removeEventListener is not executed.

[horo, 2014-10-22 08:51:24]

lgtm with nit

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:60: //
WorkerGlobalScope
stopFetch() is also WorkerGlobalScope's overridden method.

// WorkerGlobalScope
virtual void stopFetch() override;
virtual void didEvaluateWorkerScript() override;

[kouhei (in TOK), 2014-10-22 09:11:47]

https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:87: virtual void
didEvaluateWorkerScript();
Nit: above dispose().

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:68: ,
m_scriptEvaluated(false)
Nit: m_didEvaluateScript?
https://code.google.com/p/chromium/codesearch#search/&q=m_did%20file:WebKit&s...

[kouhei (in TOK), 2014-10-22 09:12:10]

Comments optional. lgtm

On 2014/10/22 09:11:47, kouhei wrote:
>
https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
> File Source/core/workers/WorkerGlobalScope.h (right):
> 
>
https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
> Source/core/workers/WorkerGlobalScope.h:87: virtual void
> didEvaluateWorkerScript();
> Nit: above dispose().
> 
>
https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
> File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):
> 
>
https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
> Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:68: ,
> m_scriptEvaluated(false)
> Nit: m_didEvaluateScript?
>
https://code.google.com/p/chromium/codesearch#search/&q=m_did%20file:WebKit&s...

[shimazu, 2014-10-23 00:52:23]

I'll commit this patch. Thanks:)

https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.h:87: virtual void
didEvaluateWorkerScript();
On 2014/10/22 09:11:47, kouhei wrote:
> Nit: above dispose().

Done.

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:68: ,
m_scriptEvaluated(false)
On 2014/10/22 09:11:47, kouhei wrote:
> Nit: m_didEvaluateScript?
>
https://code.google.com/p/chromium/codesearch#search/&q=m_did%20file:WebKit&s...

Done.

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/657213004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:60: //
WorkerGlobalScope
On 2014/10/22 08:51:24, horo wrote:
> stopFetch() is also WorkerGlobalScope's overridden method.
> 
> // WorkerGlobalScope
> virtual void stopFetch() override;
> virtual void didEvaluateWorkerScript() override;

Done.

[shimazu, 2014-10-23 00:52:29]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-10-23 00:53:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657213004/60001

[commit-bot: I haz the power, 2014-10-23 02:04:51]

Committed patchset #4 (id:60001) as 184227