Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
307223003:
Fills the info about the live versions which aren't in the live registrations. (Closed)
|
Created:
6 years, 6 months ago by horo Modified:
6 years, 6 months ago Reviewers:
nhiroki CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, darin-cc_chromium.org, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFills the info about the live versions which aren't in the live registrations.
BUG=358657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274501
Patch Set 1 #
Total comments: 2
Patch Set 2 : incorporated nhiroki's comment #
Total comments: 5
Patch Set 3 : omit bracket #Messages
Total messages: 11 (0 generated)
nhiroki@ Could you please review this?
https://codereview.chromium.org/307223003/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/307223003/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:585: info.active_version = version->GetInfo(); There might be a case that |version| hasn't been activated yet (see ServiceWorkerRegisterJob::OnInstallFinished()). Can you check |it->is_active| and if false, set |.pending_version| to |version|?
https://codereview.chromium.org/307223003/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/307223003/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:585: info.active_version = version->GetInfo(); On 2014/06/03 02:37:10, nhiroki wrote: > There might be a case that |version| hasn't been activated yet (see > ServiceWorkerRegisterJob::OnInstallFinished()). Can you check |it->is_active| > and if false, set |.pending_version| to |version|? Done.
Looks good with one nit and one unrelated comment. https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:588: } nit: can you omit {} like line 590-594? https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:595: info.active_version.version_id = it->version_id; Hmm... this is not related to this CL, but these setup seems wrong. If |it->is_active| is false, we might need to setup |pending_version| instead of |active_version|. Do you think you could take a look at this, too? (fyi: ServiceWorkerStorage::GetOrCreateRegistration() makes a registration from a stored value. It might be helpful to understand.)
lgtm https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:595: info.active_version.version_id = it->version_id; On 2014/06/03 04:56:52, nhiroki wrote: > Hmm... this is not related to this CL, but these setup seems wrong. If > |it->is_active| is false, we might need to setup |pending_version| instead of > |active_version|. Do you think you could take a look at this, too? > > (fyi: ServiceWorkerStorage::GetOrCreateRegistration() makes a registration from > a stored value. It might be helpful to understand.) I think it's ok to make it in a separate CL because it has already been broken.
https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:588: } On 2014/06/03 04:56:52, nhiroki wrote: > nit: can you omit {} like line 590-594? Done. https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:595: info.active_version.version_id = it->version_id; On 2014/06/03 04:56:52, nhiroki wrote: > Hmm... this is not related to this CL, but these setup seems wrong. If > |it->is_active| is false, we might need to setup |pending_version| instead of > |active_version|. Do you think you could take a look at this, too? > > (fyi: ServiceWorkerStorage::GetOrCreateRegistration() makes a registration from > a stored value. It might be helpful to understand.) Um... I'm not so familiar with creating registration. Could you please file a bug?
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/307223003/40001
On 2014/06/03 05:10:47, horo wrote: > https://codereview.chromium.org/307223003/diff/20001/content/browser/service_... > content/browser/service_worker/service_worker_storage.cc:595: > info.active_version.version_id = it->version_id; > On 2014/06/03 04:56:52, nhiroki wrote: > > Hmm... this is not related to this CL, but these setup seems wrong. If > > |it->is_active| is false, we might need to setup |pending_version| instead of > > |active_version|. Do you think you could take a look at this, too? > > > > (fyi: ServiceWorkerStorage::GetOrCreateRegistration() makes a registration > from > > a stored value. It might be helpful to understand.) > > Um... > I'm not so familiar with creating registration. > Could you please file a bug? Filed in http://crbug.com/380030. Please feel free to assign me if you'd like to work on different issues. Thanks!
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 274501 |
