Implement registration job ordering

Implement registration job ordering

To handle the case of multiple registration requests coming in at the same time,
the ServiceWorkerJobCoordinator now keeps a queue of registration events for each pattern.

When a new registration request comes in for a pattern, it looks to see if the most recent request (at the back of the queue) is identical, and if so, piggy-backs on that request rather than creating a new one.

BUG=285976
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243999

[alecflett, 2014-01-07 21:22:42]

ok, here is the follow-on to http://crrev.com/113133013 - no tests yet, but
aside from bugs, the code is ready to go.

(I'll add tests while you review the code)

[alecflett, 2014-01-07 23:42:25]

ok, a decent set of tests has been added.

[kinuko, 2014-01-08 08:38:35]

Ok, the code looks a bit rough but the logic has become clearer now, thanks for
that. Some comments for possible polishment..

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:27:
ServiceWorkerRegisterJob* job;
Please initialize with NULL etc

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:49: callback));
This code path is mostly same as that of Unregister, and handling jobs_[pattern]
map and pending jobs queue together is making the code a bit messty. Could we
introduce a smaller inner class for each queue and separate the code out from
jobs_[pattern] map part?  Also I think it's ok to always create RegisterJob
first and delete it if it's coalesced into another one for simpler code. I'd
probably write this like:

class JobQueue {
 public:
  ~JobQueue() {
    STLDeleteElements(&jobs_);
  }

  void Push(scoped_ptr<Job> job, callback) {
    if (jobs_.empty()) { 
      jobs_.push_back(job);
      job->Start();
    } else if (!job->Equals(jobs_.back())) {
      jobs_.push_back(job);
    }
    DCHECK(!jobs_.empty());
    jobs_.back()->AddCallback(callback);
  }

  void Pop(Job* job) {
    DCHECK(job == jobs_.front());
    jobs_.pop_front();
    if (!jobs_.empty())
      jobs_.front()->Start();
  }

 private:
  // The last one must be always active / running.
  std::deque<Job*> jobs_;
};

Then the code for Register(), Unregister() and FinishJob() would become much
smaller.  Wdyt?

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:117:
(*it->second.rbegin())->script_url() == script_url);
This line 116-117 could be probably separated out as a small generic routine
(like Equals) of ServiceWorkerRegistrationJob

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.h:45:
RegistrationJobMap;
A question, what flow is expected when we have concurrent registrations for the
same ServiceWorkerRegistration but with different patterns?

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:25: type_(type) {}
running_ is not initialized (or I hope we can just remove the flag for now)

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:54: }
Can you move this to .cc ?

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:58: DCHECK(type_ ==
REGISTER);
We seem to call this for UNREGISTER case too?

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:62: bool
is_running() const { return running_; }
This doesn't seem to be used, or relying on this flag feels a bit sketch-- can
we remove this (until we find it necessary)?

[alecflett, 2014-01-08 23:59:16]

(implemented everything except JobQueue. Uploaded a patch now in case I don't
finish today)

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:27:
ServiceWorkerRegisterJob* job;
On 2014/01/08 08:38:36, kinuko wrote:
> Please initialize with NULL etc

Done.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:49: callback));
On 2014/01/08 08:38:36, kinuko wrote:
> Then the code for Register(), Unregister() and FinishJob() would become much
> smaller.  Wdyt?

I like it. I'll give it a try.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:117:
(*it->second.rbegin())->script_url() == script_url);
On 2014/01/08 08:38:36, kinuko wrote:
> This line 116-117 could be probably separated out as a small generic routine
> (like Equals) of ServiceWorkerRegistrationJob

Done.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.h:45:
RegistrationJobMap;
On 2014/01/08 08:38:36, kinuko wrote:
> A question, what flow is expected when we have concurrent registrations for
the
> same ServiceWorkerRegistration but with different patterns?

That doesn't happen - registrations are keyed by pattern - you can't have the
same registration for two different patterns.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:25: type_(type) {}
On 2014/01/08 08:38:36, kinuko wrote:
> running_ is not initialized (or I hope we can just remove the flag for now)

Done.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:54: }
On 2014/01/08 08:38:36, kinuko wrote:
> Can you move this to .cc ?

oops! meant to do that before uploading. Done.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:58: DCHECK(type_ ==
REGISTER);
On 2014/01/08 08:38:36, kinuko wrote:
> We seem to call this for UNREGISTER case too?

the && short-circuits this call)

But we never should need to call this - unregister doesn't have the notion of a
script_url so to ever call it means the caller is trying to do something wrong.

https://codereview.chromium.org/126603002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:62: bool
is_running() const { return running_; }
On 2014/01/08 08:38:36, kinuko wrote:
> This doesn't seem to be used, or relying on this flag feels a bit sketch-- can
> we remove this (until we find it necessary)?

this is mainly for tests - I thought I had an EXPECT_TRUE/FALSE, I must have
left it out

I'd also like to expose this in the internals page (still under construction)

I'll make sure the test is there and will comment that this is (currently) only
for tests.

[alecflett, 2014-01-09 02:05:21]

Ok, this is finally ready - PTAL?

Implemented the JobQueue, cleaned up some callbacks (we have fewer callback
typedefs now, yay!) and removed running_ altogether. I'll deal with that one
later! PTAL?

[kinuko, 2014-01-09 10:54:07]

lgtm

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:42:
.Push(job.Pass(),
Was this formatting done by clang-format?  I (as an incomplete human) slightly
prefer

jobs_[pattern].Push(
    job.Pass(),
    base::Bind(...));

but if that's the case I'm ok with it...

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.h:43: class
JobQueue {
Unless we have tests specifically for this class this can be just moved into .cc
and it'd be probably cleaner. (Just have forward decl here)

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:44: return
job->type() == type_ && job->script_url() == script_url_;
When both types are UNREGISTER we still seem to call script_url() (which hits
DCHECK)?

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:51:
RegistrationType type() const { return type_; }
Looks like these public accessors are no longer necessary?

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:111: const
RegistrationType type_;
(While it's ok in this particular case) can you move type_ before WeakPtrFactory
so that WeakPtrFactory is the last one (and the first one to be invalidated)

[kinuko, 2014-01-09 14:08:08]

some more nits

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:51:
pending_jobs->second.Pop(job);
You may also want to remove the pattern from the jobs_ map if the queue is
empty?  (JobQueue class could have empty() method)

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:54:
ServiceWorkerJobCoordinator::JobQueue::JobQueue() {}
style-nit: I think we usually put inner class implementation before (or after,
though less common) the entire outer class definition (so each class
implementation doesn't interleave with each other).

[alecflett, 2014-01-09 20:17:19]

Address review comments

[commit-bot: I haz the power, 2014-01-09 20:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/126603002/320001

[alecflett, 2014-01-09 21:23:14]

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:42:
.Push(job.Pass(),
Yes, that was clang-format. the problem was this was the only way to format it
and stay within 80 cols.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:51:
pending_jobs->second.Pop(job);
On 2014/01/09 14:08:08, kinuko wrote:
> You may also want to remove the pattern from the jobs_ map if the queue is
> empty?  (JobQueue class could have empty() method)

oops, oversight! Done.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.cc:54:
ServiceWorkerJobCoordinator::JobQueue::JobQueue() {}
On 2014/01/09 14:08:08, kinuko wrote:
> style-nit: I think we usually put inner class implementation before (or after,
> though less common) the entire outer class definition (so each class
> implementation doesn't interleave with each other).

I'll move before.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.h:43: class
JobQueue {
On 2014/01/09 10:54:08, kinuko wrote:
> Unless we have tests specifically for this class this can be just moved into
.cc
> and it'd be probably cleaner. (Just have forward decl here)

The problem is that the enclosing class has a map<string, JobQueue> so it needs
to see the size and constructor, so it needs to stay here.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:44: return
job->type() == type_ && job->script_url() == script_url_;
On 2014/01/09 10:54:08, kinuko wrote:
> When both types are UNREGISTER we still seem to call script_url() (which hits
> DCHECK)?

good point. We don't actually hit this in practice, which means my tests aren't
sufficient! Added the check here, and added a test.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:51:
RegistrationType type() const { return type_; }
On 2014/01/09 10:54:08, kinuko wrote:
> Looks like these public accessors are no longer necessary?

Done.

https://codereview.chromium.org/126603002/diff/230001/content/browser/service...
content/browser/service_worker/service_worker_register_job.h:111: const
RegistrationType type_;
On 2014/01/09 10:54:08, kinuko wrote:
> (While it's ok in this particular case) can you move type_ before
WeakPtrFactory
> so that WeakPtrFactory is the last one (and the first one to be invalidated)

Done.

[commit-bot: I haz the power, 2014-01-09 22:27:12]

Change committed as 243999