Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377603002:
Disable ServiceWorker when JS is disabled (Closed)
|
Created:
4 years, 2 months ago by shimazu Modified:
4 years, 2 months ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable ServiceWorker when JS is disabled
This patch restricts the service worker when JavaScript is disabled by checking
the settings in ChromeContentBrowserClient::AllowServiceWorker().
BUG=632823
Committed: https://crrev.com/ce8af8812b8de0246975c932063b1e03ed570068
Cr-Commit-Position: refs/heads/master@{#423745}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added browser tests #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : Check the icon to notify JS is diabled #Patch Set 5 : wrap by FILE_PATH_LITERAL #
Total comments: 14
Patch Set 6 : Updated comments, removed the mock headers and renamed the function #
Messages
Total messages: 41 (24 generated)
The CQ bit was checked by shimazu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shimazu@chromium.org changed reviewers: + falken@chromium.org, jochen@chromium.org
PTAL, thanks:)
Description was changed from ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by the content settings by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 ========== to ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
please add a test
https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1786: // Check if Cookie is allowed nit: add a period to the end of comments: https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... "Check if cookies are allowed." is more natural. https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1791: // Check if JS is allowed nit: same
Thanks for your comments! Added tests and fixed the comments. PTAL again:) https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1786: // Check if Cookie is allowed On 2016/09/28 13:51:27, falken (ooo Sep 29) wrote: > nit: add a period to the end of comments: > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... > > "Check if cookies are allowed." is more natural. Done. https://codereview.chromium.org/2377603002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1791: // Check if JS is allowed On 2016/09/28 13:51:27, falken (ooo Sep 29) wrote: > nit: same Done.
https://codereview.chromium.org/2377603002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2377603002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:209: EXPECT_EQ(expected_title2, title_watcher2.WaitAndGetTitle()); you could check that the tab's tab_specific_content_settings return true for IsContentBlocked(CONTENT_SETTINGS_TYPE_JAVASCRIPT) and not before navigating
The CQ bit was checked by shimazu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added the check of ContentSettings and moved the initialization codes of content settings related to JavaScripts to the beginning of a navigation. PTAL. https://codereview.chromium.org/2377603002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2377603002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:209: EXPECT_EQ(expected_title2, title_watcher2.WaitAndGetTitle()); On 2016/09/29 12:15:49, jochen (slow) wrote: > you could check that the tab's tab_specific_content_settings return true for > IsContentBlocked(CONTENT_SETTINGS_TYPE_JAVASCRIPT) and not before navigating Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by shimazu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I don't understand why it's safe to add JAVASCRIPT to the Clear* functions. Why was this needed and does it change behavior outside of service workers? https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1812: // Check if javascripts are allowed nit: Add a period to the end of the sentence. Also, "javascripts are" => "JavaScript is" is more common. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1821: // Check if cookies are allowed nit: period at end of the sentence. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:138: "HTTP/1.1 200 OK\nContent-Type: text/javascript"); Apparently we don't need these mock headers: https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha... (note that sw.js in the other test doesn't have them) https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.cc:671: status.first == CONTENT_SETTINGS_TYPE_JAVASCRIPT) Why was this change needed? Before this patch, if you block JS on a site, the icon would be cleared immediately? https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:166: // |blocked_by_policy| should be true, and this function should invoke nit: update this comment: there is no longer a |blocked_by_policy| parameter. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:175: // CONTENT_SETTINGS_TYPE_COOKIES related information. This comment is no longer true. Now it's COOKIES and JAVASCRIPT. Can you also add a comment explaining why COOKIES and JAVASCRIPT are ones excepted? What is special about those two? Actually the comment is already wrong, there is no longer |content_blocked_| and |content_allowed_| arrays. The naming is also inconsistent with ClearNavigationRelatedContentSettings. It should just be ClearContentSettingsExceptForNavigationRelatedSettings, doesn't need the "Blocked". https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:179: // Resets all cookies related information. ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> I don't understand why it's safe to add JAVASCRIPT to the Clear* functions. Why was this needed and does it change behavior outside of service workers? I think the JAVASCRIPT setting is necessary for navigation with serviceworkers, but I'm still not sure how's dangerous clearing before navigation. jochen@: what do you think about that? https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1812: // Check if javascripts are allowed On 2016/10/05 01:16:50, falken wrote: > nit: Add a period to the end of the sentence. Also, "javascripts are" => > "JavaScript is" is more common. Done. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1821: // Check if cookies are allowed On 2016/10/05 01:16:49, falken wrote: > nit: period at end of the sentence. Done. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:138: "HTTP/1.1 200 OK\nContent-Type: text/javascript"); On 2016/10/05 01:16:50, falken wrote: > Apparently we don't need these mock headers: > https://cs.chromium.org/chromium/src/net/test/embedded_test_server/request_ha... > > (note that sw.js in the other test doesn't have them) Done. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.cc:671: status.first == CONTENT_SETTINGS_TYPE_JAVASCRIPT) On 2016/10/05 01:16:50, falken wrote: > Why was this change needed? Before this patch, if you block JS on a site, the > icon would be cleared immediately? Yes, the icon will be shown during navigation and be cleared after navigation without this change. I heard from jochen that previously the setting of cookies is only needed for navigation, so I added that because now the service workers additionally requires the JS settings. I'm still not sure why the other settings should be cleared after the navigation though.. I'm guessing it's a kind of white listing, but no confidence. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:166: // |blocked_by_policy| should be true, and this function should invoke On 2016/10/05 01:16:50, falken wrote: > nit: update this comment: there is no longer a |blocked_by_policy| parameter. Done. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:175: // CONTENT_SETTINGS_TYPE_COOKIES related information. On 2016/10/05 01:16:50, falken wrote: > This comment is no longer true. Now it's COOKIES and JAVASCRIPT. Can you also > add a comment explaining why COOKIES and JAVASCRIPT are ones excepted? What is > special about those two? > > Actually the comment is already wrong, there is no longer |content_blocked_| and > |content_allowed_| arrays. > > The naming is also inconsistent with ClearNavigationRelatedContentSettings. It > should just be ClearContentSettingsExceptForNavigationRelatedSettings, doesn't > need the "Blocked". Done. https://codereview.chromium.org/2377603002/diff/80001/chrome/browser/content_... chrome/browser/content_settings/tab_specific_content_settings.h:179: // Resets all cookies related information. On 2016/10/05 01:16:50, falken wrote: > ditto Done.
lgtm I don't totally understand the clear settings on navigation thing, but defer to jochen there. I just didn't want to regress something like the JavaScript badge won't get cleared (in the non-service worker case) because it's now whitelisted in ClearBlockedContentSettingsExceptForCookies.
The CQ bit was checked by shimazu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/05 at 03:38:00, falken wrote: > lgtm > > I don't totally understand the clear settings on navigation thing, but defer to jochen there. I just didn't want to regress something like the JavaScript badge won't get cleared (in the non-service worker case) because it's now whitelisted in ClearBlockedContentSettingsExceptForCookies. oh well, it's a bit of a mess meanwhile, with some settings clearing themselves in DidStartNavigation... your specific concern (badge not getting cleared) shouldn't be an issue, as we clear it together with the cookie settings at the beginning of the navigation (instead of at the end)
On 2016/10/06 12:11:38, jochen wrote: > On 2016/10/05 at 03:38:00, falken wrote: > > lgtm > > > > I don't totally understand the clear settings on navigation thing, but defer > to jochen there. I just didn't want to regress something like the JavaScript > badge won't get cleared (in the non-service worker case) because it's now > whitelisted in ClearBlockedContentSettingsExceptForCookies. > > oh well, it's a bit of a mess meanwhile, with some settings clearing themselves > in DidStartNavigation... > > your specific concern (badge not getting cleared) shouldn't be an issue, as we > clear it together with the cookie settings at the beginning of the navigation > (instead of at the end) I'd like to commit this patch as is for supporting disabling JS. Should we move all of settings before navigation?
The CQ bit was checked by shimazu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2377603002/#ps100001 (title: "Updated comments, removed the mock headers and renamed the function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shimazu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 ========== to ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 ========== to ========== Disable ServiceWorker when JS is disabled This patch restricts the service worker when JavaScript is disabled by checking the settings in ChromeContentBrowserClient::AllowServiceWorker(). BUG=632823 Committed: https://crrev.com/ce8af8812b8de0246975c932063b1e03ed570068 Cr-Commit-Position: refs/heads/master@{#423745} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ce8af8812b8de0246975c932063b1e03ed570068 Cr-Commit-Position: refs/heads/master@{#423745} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
