service worker: Update job gets the script url when the job starts

content/browser/service_worker/service_worker_register_job.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_register_job.h"
 
 #include <stdint.h>
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 32 matching lines @@
       script_url_(script_url),
       phase_(INITIAL),
       doom_installing_worker_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(false),
       skip_script_comparison_(false),
       promise_resolved_status_(SERVICE_WORKER_OK),
       weak_factory_(this) {}
 
 ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(

[nhiroki, 2016/04/28 05:51:46]

Can you remove DCHECK(registration->GetNewestVersion()) from callers of this
constructor (ServiceWorkerJobCoordinator::Update)?

[falken, 2016/04/28 06:25:17]

Great catch. Done.

     base::WeakPtr<ServiceWorkerContextCore> context,
     ServiceWorkerRegistration* registration,
     bool force_bypass_cache,
     bool skip_script_comparison)
     : context_(context),
       job_type_(UPDATE_JOB),
       pattern_(registration->pattern()),
-      script_url_(registration->GetNewestVersion()->script_url()),
       phase_(INITIAL),
       doom_installing_worker_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(force_bypass_cache),
       skip_script_comparison_(skip_script_comparison),
       promise_resolved_status_(SERVICE_WORKER_OK),
       weak_factory_(this) {
   internal_.registration = registration;
 }
@@ skipping 47 matching lines @@
 }
 
 void ServiceWorkerRegisterJob::Abort() {
   SetPhase(ABORT);
   CompleteInternal(SERVICE_WORKER_ERROR_ABORT, std::string());
   // Don't have to call FinishJob() because the caller takes care of removing
   // the jobs from the queue.
 }
 
 bool ServiceWorkerRegisterJob::Equals(ServiceWorkerRegisterJobBase* job) const {
-  if (job->GetType() != GetType())
+  if (job->GetType() != job_type_)
     return false;
   ServiceWorkerRegisterJob* register_job =
       static_cast<ServiceWorkerRegisterJob*>(job);
+  if (job_type_ == UPDATE_JOB)
+    return register_job->pattern_ == pattern_;
+  DCHECK(job_type_ == REGISTRATION_JOB);

[nhiroki, 2016/04/28 05:51:46]

DCHECK_EQ(REGISTRATION_JOB, job_type_);

[falken, 2016/04/28 06:25:17]

Done.

   return register_job->pattern_ == pattern_ &&
          register_job->script_url_ == script_url_;
 }
 
 RegistrationJobType ServiceWorkerRegisterJob::GetType() const {
   return job_type_;
 }
 
 void ServiceWorkerRegisterJob::DoomInstallingWorker() {
   doom_installing_worker_ = true;
@@ skipping 107 matching lines @@
   if (status != SERVICE_WORKER_OK) {
     Complete(status);
     return;
   }
 
   if (existing_registration.get() != registration()) {
     Complete(SERVICE_WORKER_ERROR_NOT_FOUND);
     return;
   }
 
-  // A previous job may have unregistered or installed a new version to this
-  // registration.
+  // A previous job may have unregistered this registration.
   if (registration()->is_uninstalling() ||
-      registration()->GetNewestVersion()->script_url() != script_url_) {
+      !registration()->GetNewestVersion()) {
     Complete(SERVICE_WORKER_ERROR_NOT_FOUND);
     return;
   }
 
+  script_url_ = registration()->GetNewestVersion()->script_url();

[nhiroki, 2016/04/28 05:51:46]

Can we have DCHECK(script_url_.is_empty()) immediately before this assignment?

[falken, 2016/04/28 06:25:17]

Good idea. Done.

+
   // TODO(michaeln): If the last update check was less than 24 hours
   // ago, depending on the freshness of the cached worker script we
   // may be able to complete the update job right here.
-
   UpdateAndContinue();
 }
 
 // Creates a new ServiceWorkerRegistration.
 void ServiceWorkerRegisterJob::RegisterAndContinue() {
   SetPhase(REGISTER);
 
   int64_t registration_id = context_->storage()->NewRegistrationId();
   if (registration_id == kInvalidServiceWorkerRegistrationId) {
     Complete(SERVICE_WORKER_ERROR_ABORT);
@@ skipping 316 matching lines @@
       new_version()->force_bypass_cache_for_scripts() ||
       registration()->last_update_check().is_null()) {
     registration()->set_last_update_check(base::Time::Now());
 
     if (registration()->has_installed_version())
       context_->storage()->UpdateLastUpdateCheckTime(registration());
   }
 }
 
 }  // namespace content