Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211243010:
[Service Worker Registration] Registered extension scheme to allow service workers (Closed)
Description[Service Worker Registration] Registered extension scheme to allow service workers
Service Workers require a secure origin, such as HTTPS. chrome-extension:// pages are not HTTP/HTTPS, but are secure so this change becomes a necessary step to allow extensions to register a Service Worker.
"chrome-extension" is added as a scheme allowing service workers.
BUG=501569
Committed: https://crrev.com/3868550081d275ef8f73933ffcec88da453215a2
Cr-Commit-Position: refs/heads/master@{#338708}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Test #
Total comments: 19
Patch Set 3 : Added dev test #
Total comments: 14
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : Added license #Messages
Total messages: 36 (13 generated)
annekao@google.com changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/1211243010/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1211243010/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:505: WebSecurityPolicy::registerURLSchemeAsAllowingServiceWorkers( We can actually move this into the constructor of ExtensionsRendererClient, which would be a bit cleaner (since it's an extension-specific thing). We should probably do the same with the ones above, but let's hold off on that for now. We should also start adding at least a basic test, and ensure that we get ever-so-slightly farther than we did before. :) Then, we can continue to iterate on that test, and it can serve as a nice progress tracker, too.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Added service worker registration for extension as a behavior feature for the trunk channel. service_worker_apitest tests whether a service worker is successfully registered. For now an ASSERT_FALSE is in place because there are still a few changes to be made in order to register a service worker completely. Within test.js, there is an assertEq that will make sure the right error message is being displayed when the registration fails. Two functions were added into the dispatcher. First to check if the current channel allows service worker registration and second to register the extension scheme as allowing service workers. These are called after the channel is set. Note: I'm not sure why extensions_renderer_client insists that there's a change because there isn't... Also, please let me know if the file location of the tests need to be changed.
Woo! https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:1: var registered = false; nit: you'll need the license in .js files, too. (.json and .html files are exempt) https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:2: var error_msg = ""; nitty nit: prefer single quotes, unless you have a quote with quotes. https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:6: if ('serviceWorker' in navigator) { We can probably ignore this - chrome always has serviceWorker. :) https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:9: console.log('ServiceWorker registration successful with scope: ', We're not supposed to spam the log with status from tests, because it can make a lot of noise. Of course, most people ignore this. That said, let's be good citizens :) These logs can be handy in debugging, though, so I sometimes see the pattern of var logForDebugging = false; function log(message) { if (logForDebugging) console.log(message); } Then, when you're debugging, you just flip the logForDebugging bit. https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:18: chrome.test.assertEq('SecurityError: Failed to register a' For this test extension, how about we use chrome.test.sendMessage() (see ExtensionTestMessageListener for examples) to forward either a "success" message, or the error? That way, we can re-use it for both the "fails when channel is wrong" and "gets as far as we can" tests. https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:19: + 'ServiceWorker: No URL is associated with the caller\'s' nitty nit: prefer double quotes when you have a quote with single quotes in it. https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:103: using base::ASCIIToUTF16; avoid using "usings" when it's less than about 5 - 10 instances. Even though this file is pretty bad for it already. :) https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1456: WebString extension_scheme(ASCIIToUTF16(extensions::kExtensionScheme)); This would be simpler using WebString::fromUtf8(kExtensionScheme). It's also basic enough that you can inline it in the registerURLScheme...() call. https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1464: bool Dispatcher::IsExtensionAllowedToUseServiceWorker() { It's fine to inline this function in AddChannelSpecificFeatures(). https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ext... File extensions/renderer/extensions_renderer_client.cc (left): https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ext... extensions/renderer/extensions_renderer_client.cc:27: } // namespace extensions Odd. Did your editor pad this with spaces? Or add another newline? If in doubt, you could always do git checkout master extensions/renderer/extensions_renderer_client.cc https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... File extensions/renderer/service_worker_apitest.cc (right): https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:5: #include "chrome/browser/extensions/extension_apitest.h" This would be a layering violation (no chrome/ files in top-level extensions/), but we can just put this whole thing in chrome/browser/extensions/. https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:8: namespace { No need to put this in an anonymous namespace (I don't know why some tests do) https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:11: public: nit: indentation is off. You can also use "git cl format" to handle little style things like this. https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:18: extensions::ScopedCurrentChannel current_channel_; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1211243010/diff/80001/extensions/renderer/ser... extensions/renderer/service_worker_apitest.cc:23: IN_PROC_BROWSER_TEST_F(ServiceWorkerTest, RegisterServiceWorker) { Let's have two tests, one that checks that service worker registration always fails with the channel as, say, dev (bumped to beta or stable as we begin releasing), and one that tests that service worker registration gets as far as we have with the other scope. This will probably mean you need two test classes (or a templated one, if you feel brave), since the channel needs to be set up pretty early on.
kalman@chromium.org changed reviewers: + kalman@chromium.org
Tiny drive-by. Awesome to see this happening. https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/service_worker/register/test.js:3: also, JS style is camelcase unlike C++. https://codereview.chromium.org/1211243010/diff/80001/extensions/common/featu... File extensions/common/features/behavior_feature.cc (right): https://codereview.chromium.org/1211243010/diff/80001/extensions/common/featu... extensions/common/features/behavior_feature.cc:14: const char* BehaviorFeature::kServiceWorker = "service_worker"; also alphabetical order, and leave the blank line between the constant declarations and the end of the namespace. https://codereview.chromium.org/1211243010/diff/80001/extensions/common/featu... File extensions/common/features/behavior_feature.h (right): https://codereview.chromium.org/1211243010/diff/80001/extensions/common/featu... extensions/common/features/behavior_feature.h:24: static const char* kServiceWorker; nice to keep in alphabetical order
kalman@chromium.org changed reviewers: - kalman@chromium.org
Fixed all small problems mentioned in the comments. Added the second test for the dev channel showing a different error. Also moved the test from extensions/renderer to browser/extensions.
https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:29: if (listener.message() != "success") { Instead of this, we should just assert that the error message *is* the expected one. After all, we don't expect it to succeed, right? (Same with below) https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:37: IN_PROC_BROWSER_TEST_F(ServiceWorkerTest, RegisterServiceWorkerDev) { Maybe rename this to "CannotRegisterServiceWorkersOnDev" with a comment about why dev is significant? (The feature is restricted to trunk, so on dev it should have existing behavior - which is for it to fail.) https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:38: extensions::ScopedCurrentChannel current_channel_override( nit: this is already in the extensions namespace, so you don't need the prefix (everywhere in this file). https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:12: var errorMsg = ''; no need to declare these way up here. https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:22: }).catch(function(err) { indentation looks off. https://codereview.chromium.org/1211243010/diff/100001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/1211243010/diff/100001/extensions/common/api/... extensions/common/api/_behavior_features.json:43: "service_worker": { If we're gonna alphabetize the other constants, we should alphabetize this list, too. https://codereview.chromium.org/1211243010/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1211243010/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1460: WebString::fromUTF8(extensions::kExtensionScheme)); nit: no extensions::
Oh, also, code review tip: As you address a comment, go to it's link in Rietveld and click "Done". Then, when you've finished them all, publish and mail comments and it'll say "done" next to each. (Of course, if you have a question on one, hit "reply" instead of "done"). It makes it easy for both you and the reviewer to make sure that all comments are addressed, without combing through the different patch sets. :)
Got it! https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:29: if (listener.message() != "success") { On 2015/07/13 19:32:24, Devlin wrote: > Instead of this, we should just assert that the error message *is* the expected > one. After all, we don't expect it to succeed, right? > > (Same with below) Done. https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:37: IN_PROC_BROWSER_TEST_F(ServiceWorkerTest, RegisterServiceWorkerDev) { On 2015/07/13 19:32:24, Devlin wrote: > Maybe rename this to "CannotRegisterServiceWorkersOnDev" with a comment about > why dev is significant? (The feature is restricted to trunk, so on dev it > should have existing behavior - which is for it to fail.) Done. https://codereview.chromium.org/1211243010/diff/100001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:38: extensions::ScopedCurrentChannel current_channel_override( On 2015/07/13 19:32:24, Devlin wrote: > nit: this is already in the extensions namespace, so you don't need the prefix > (everywhere in this file). Done. https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:12: var errorMsg = ''; On 2015/07/13 19:32:25, Devlin wrote: > no need to declare these way up here. Done. https://codereview.chromium.org/1211243010/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:22: }).catch(function(err) { On 2015/07/13 19:32:25, Devlin wrote: > indentation looks off. Done. https://codereview.chromium.org/1211243010/diff/100001/extensions/common/api/... File extensions/common/api/_behavior_features.json (right): https://codereview.chromium.org/1211243010/diff/100001/extensions/common/api/... extensions/common/api/_behavior_features.json:43: "service_worker": { On 2015/07/13 19:32:25, Devlin wrote: > If we're gonna alphabetize the other constants, we should alphabetize this list, > too. Done. https://codereview.chromium.org/1211243010/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/1211243010/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1460: WebString::fromUTF8(extensions::kExtensionScheme)); On 2015/07/13 19:32:25, Devlin wrote: > nit: no extensions:: Done.
Nice, super close! :) https://codereview.chromium.org/1211243010/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:23: // This should fail because there are changes to be made to nit: The "this should fail" almost looks like it's applying to the full test - let's move this comment above the RunExtensionTest() call. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:17: registration.scope); nit: indentation. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:26: chrome.test.assertTrue(registered); hmm... since registration is asynchronous, would this ever succeed? I think this would be better as having a chrome.test.succeed() in the resolve, and chrome.test.fail() in the reject. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:29: remove empty blank line.
https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:17: registration.scope); On 2015/07/13 21:22:11, Devlin wrote: > nit: indentation. Should the next line line up with the quote?
https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:17: registration.scope); On 2015/07/13 21:30:47, annekao wrote: > On 2015/07/13 21:22:11, Devlin wrote: > > nit: indentation. > > Should the next line line up with the quote? Yes. You actually have two choices - you can line up each parameter: MyMethod(MethodArgument1, MethodArgument2); - or you can use 4-space indentation for a line continuation: MyMethod( MethodArgument1, MethodArgument2); (preferring the former if it fits) Interestingly, in this case, 4-space indentation puts it at the same spot :)
https://codereview.chromium.org/1211243010/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:23: // This should fail because there are changes to be made to On 2015/07/13 21:22:11, Devlin wrote: > nit: The "this should fail" almost looks like it's applying to the full test - > let's move this comment above the RunExtensionTest() call. Done. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/register/test.js (right): https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:17: registration.scope); On 2015/07/13 21:22:11, Devlin wrote: > nit: indentation. Done. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:26: chrome.test.assertTrue(registered); On 2015/07/13 21:22:11, Devlin wrote: > hmm... since registration is asynchronous, would this ever succeed? I think > this would be better as having a chrome.test.succeed() in the resolve, and > chrome.test.fail() in the reject. Done. https://codereview.chromium.org/1211243010/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/register/test.js:29: On 2015/07/13 21:22:11, Devlin wrote: > remove empty blank line. Done.
lgtm! Woohoo!
The CQ bit was checked by annekao@google.com
The CQ bit was unchecked by annekao@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211243010/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/07/14 00:15:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) On 2015/07/10 21:51:23, Devlin wrote: > https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/service_worker/register/test.js:1: var > registered = false; > nit: you'll need the license in .js files, too. (.json and .html files are > exempt) Looks like we missed this one in sw.js.
On 2015/07/14 15:17:44, Devlin wrote: > On 2015/07/14 00:15:59, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > On 2015/07/10 21:51:23, Devlin wrote: > > > https://codereview.chromium.org/1211243010/diff/80001/chrome/test/data/extens... > > chrome/test/data/extensions/api_test/service_worker/register/test.js:1: var > > registered = false; > > nit: you'll need the license in .js files, too. (.json and .html files are > > exempt) > Looks like we missed this one in sw.js. Ah, my bad. Fixed it up, I'll recommit it.
The CQ bit was checked by annekao@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1211243010/#ps160001 (title: "Added license")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211243010/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by kalman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211243010/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3868550081d275ef8f73933ffcec88da453215a2 Cr-Commit-Position: refs/heads/master@{#338708} |
