workers: Move core worker functionality into InProcessWorkerBase.

workers: Move core worker functionality into InProcessWorkerBase.

WorkerBase implements the core worker functionality, with Worker providing
implementation for dedicated workers, and CompositedWorker providing
implementation for compositor-workers.

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

[sadrul, 2015-04-08 20:22:49]

sadrul@chromium.org changed reviewers:
+ kinuko@chromium.org, tkent@chromium.org

[sadrul, 2015-04-08 20:22:50]

Hi! As discussed in https://codereview.chromium.org/1018863002, this CL
refactors the work code a bit. This should also help avoid issues like
http://crbug.com/471785 in the future.

[kinuko, 2015-04-09 07:28:51]

On 2015/04/08 20:22:50, sadrul wrote:
> Hi! As discussed in https://codereview.chromium.org/1018863002, this CL
> refactors the work code a bit. This should also help avoid issues like
> http://crbug.com/471785 in the future.

Can I have access to the bug?

[kinuko, 2015-04-09 08:11:34]

I think I prefer just having Worker and CompositeWorker derive from
AbstractWorker but factor out a common code for composition (e.g. we could have
InProcessWorkerScriptLoaderClient that have common code for initialize,
didReceiveResponse and notifyFinished), but the new code looks much better than
having CompositeWorker derive from Worker.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
File Source/core/workers/WorkerBase.h (right):

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:28: class CORE_EXPORT WorkerBase : public
AbstractWorker, private WorkerScriptLoaderClient {
Could this be called InProcessWorkerBase or something that implies it's a base
class only for in-process ones?

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:33: bool initialize(ExecutionContext*, const
String&, ExceptionState&);
Looks like this could be protected.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:37: bool hasPendingActivity() const override;
Please comment that they're ActiveDOMObject overrides.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:45: virtual WorkerGlobalScopeProxy*
createWorkerGlobalScopeProxy(ExecutionContext*) = 0;
Please document.

[sadrul, 2015-04-09 18:53:41]

On 2015/04/09 08:11:34, kinuko wrote:
> I think I prefer just having Worker and CompositeWorker derive from
> AbstractWorker but factor out a common code for composition (e.g. we could
have
> InProcessWorkerScriptLoaderClient that have common code for initialize,
> didReceiveResponse and notifyFinished), but the new code looks much better
than
> having CompositeWorker derive from Worker.

I looked into InProcessWorkerScriptLoaderClient, and I think it would be simple
to do, but because we would still either need to duplicate the code related to
m_contextProxy, or keep the common ancestor for Worker and CompositorWorker, I
am not sure that gives us a lot value. What do you think? In the meantime, I
have addressed your comments in the CL. PTAL.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
File Source/core/workers/WorkerBase.h (right):

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:28: class CORE_EXPORT WorkerBase : public
AbstractWorker, private WorkerScriptLoaderClient {
On 2015/04/09 08:11:34, kinuko wrote:
> Could this be called InProcessWorkerBase or something that implies it's a base
> class only for in-process ones?

Done.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:33: bool initialize(ExecutionContext*, const
String&, ExceptionState&);
On 2015/04/09 08:11:34, kinuko wrote:
> Looks like this could be protected.

Done.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:37: bool hasPendingActivity() const override;
On 2015/04/09 08:11:34, kinuko wrote:
> Please comment that they're ActiveDOMObject overrides.

Done.

https://codereview.chromium.org/1075603003/diff/1/Source/core/workers/WorkerB...
Source/core/workers/WorkerBase.h:45: virtual WorkerGlobalScopeProxy*
createWorkerGlobalScopeProxy(ExecutionContext*) = 0;
On 2015/04/09 08:11:34, kinuko wrote:
> Please document.

Done.

[kinuko, 2015-04-13 05:55:52]

(Sorry, didn't realize I hadn't hit publish.)

Ok, lgtm. 

> I looked into InProcessWorkerScriptLoaderClient, and I think it would be
simple
> to do, but because we would still either need to duplicate the code related to
> m_contextProxy, or keep the common ancestor for Worker and CompositorWorker, I
> am not sure that gives us a lot value. What do you think? In the meantime, I
> have addressed your comments in the CL. PTAL.

I can live with the current code too. I don't think it has little value though
if we could have clearer class definitions with simpler inheritance, given that
the remaining dup is small and we already have the common ancestor for Worker
and CompositeWorker which seems enough for messaging proxy.

https://codereview.chromium.org/1075603003/diff/20001/Source/core/workers/InP...
File Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1075603003/diff/20001/Source/core/workers/InP...
Source/core/workers/InProcessWorkerBase.h:48: // Creates a proxy to allow
communicating with the worker's global scope.
Hmm, ok, sorry, may not be very informative as the method name implies it.  We
can remove this comment, or maybe add a bit more info like: the created proxy is
supposed to be self-owned and to be deleted upon terminateWorkerGlobalScope().

[sadrul, 2015-04-13 16:50:03]

https://codereview.chromium.org/1075603003/diff/20001/Source/core/workers/InP...
File Source/core/workers/InProcessWorkerBase.h (right):

https://codereview.chromium.org/1075603003/diff/20001/Source/core/workers/InP...
Source/core/workers/InProcessWorkerBase.h:48: // Creates a proxy to allow
communicating with the worker's global scope.
On 2015/04/13 05:55:52, kinuko wrote:
> Hmm, ok, sorry, may not be very informative as the method name implies it.  We
> can remove this comment, or maybe add a bit more info like: the created proxy
is
> supposed to be self-owned and to be deleted upon terminateWorkerGlobalScope().

Good point. Updated the comment to document the lifetime of the proxy.

[sadrul, 2015-04-14 00:54:06]

tkent@ Hi! Would you mind reviewing the CL too? (I need your approval for
Source/modules/ and Source/web/) Thanks!

[tkent, 2015-04-14 01:02:48]

lgtm.

I'm sorry for the delay.
You should be an OWNER of modules/compositorworker/ :)

https://codereview.chromium.org/1075603003/diff/40001/Source/core/workers/Wor...
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/1075603003/diff/40001/Source/core/workers/Wor...
Source/core/workers/Worker.h:15: class WorkerScriptLoader;
nit: It's unnecessary

[sadrul, 2015-04-14 02:25:08]

On 2015/04/14 01:02:48, tkent wrote:
> lgtm.
> 
> I'm sorry for the delay.
> You should be an OWNER of modules/compositorworker/ :)

Noted :) [I am waiting for the code in there to become a bit more mature and
blessed]

https://codereview.chromium.org/1075603003/diff/40001/Source/core/workers/Wor...
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/1075603003/diff/40001/Source/core/workers/Wor...
Source/core/workers/Worker.h:15: class WorkerScriptLoader;
On 2015/04/14 01:02:48, tkent wrote:
> nit: It's unnecessary

Done.

[sadrul, 2015-04-14 02:25:29]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-04-14 02:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1075603003/#ps60001
(title: ".")

[commit-bot: I haz the power, 2015-04-14 02:25:48]

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

[commit-bot: I haz the power, 2015-04-14 02:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-14 02:47:46]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[sadrul, 2015-04-14 05:11:56]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2015-04-14 05:12:16]

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

[commit-bot: I haz the power, 2015-04-14 05:32:52]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193670