Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895643002:
[subresource_filter] Display dev tools console message for subresource blocking (Closed)
|
Created:
3 years, 7 months ago by shivanisha Modified:
3 years, 7 months ago CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Display dev tools console message for subresource blocking
This CL displays a dev tools message when a sub resource is blocked, subsequent CLs
will include messages for subframes and main frame navigations.
Note that the string being displayed in this CL is not the final string. This CL is
to ensure that a place holder and tests are in place for the final string.
BUG=724549
Review-Url: https://codereview.chromium.org/2895643002
Cr-Commit-Position: refs/heads/master@{#474670}
Committed: https://chromium.googlesource.com/chromium/src/+/f6319b29cab58d45f95c0a8f2388cf5650732870
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedback addressed #
Total comments: 7
Patch Set 3 : Feedback addressed. #
Total comments: 4
Patch Set 4 : Added comment #
Total comments: 2
Patch Set 5 : rbyers@ feedback. #
Messages
Total messages: 59 (36 generated)
The CQ bit was checked by shivanisha@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...
Description was changed from ========== Initial patch BUG= ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG= ==========
shivanisha@chromium.org changed reviewers: + csharrison@chromium.org
PTAL, Thanks!
Looks like you need to upload your layout test. Would you also update the description to include a BUG= and wrap to 80 cols? https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:21: void emitErrorForDisallowedLoads(const String& url, Document& document) { Just inlining this in AllowLoad is better imo. https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:22: String message = Use StringBuilder for more efficient string appending. https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:23: "Subresource filtering disallowed loading this resource, " + url + "."; Needs a TODO to update the string when we've decided on it. https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:57: if (subresource_filter_->ShouldLogToConsole() && This logic needs to be in ReportLoad, because its name is Report :) We definitely don't want to log a message if the reporting policy isn't KReport (e.g. for preloads). Would you add a test for this case? E.g. make sure that preloads from the preload scanner do not trigger a console message.
Description was changed from ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG= ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG=724549 ==========
Description was changed from ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG=724549 ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG=724549 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@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...
PTAL, Thanks! Latest patch addresses the feedback. The layout test is the existing one which has a disallowed resource and an allowed resource. This CL adds an expected file for that to show that the console message is displayed only for the disallowed resource. https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:21: void emitErrorForDisallowedLoads(const String& url, Document& document) { On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > Just inlining this in AllowLoad is better imo. Compiler complains about declaring variables inside the case statement in ReportLoad, so letting it remain in a separate function vs declaring the variables outside of switch. https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:22: String message = On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > Use StringBuilder for more efficient string appending. done https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:23: "Subresource filtering disallowed loading this resource, " + url + "."; On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > Needs a TODO to update the string when we've decided on it. done https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:57: if (subresource_filter_->ShouldLogToConsole() && On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > This logic needs to be in ReportLoad, because its name is Report :) We definitely don't want to log a message if the reporting policy isn't KReport (e.g. for preloads). > Done. Thanks. > Would you add a test for this case? > E.g. make sure that preloads from the preload scanner do not trigger a console message. The layout tests mock out only the WebDocumentSubresourceFilter so there isn't a way currently to mock the value of SecurityViolationReportingPolicy passed in to SubresourceFilter::AllowLoad()
LGTM % nits! https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:21: void emitErrorForDisallowedLoads(const String& url, Document& document) { On 2017/05/22 14:26:27, shivanisha wrote: > On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > > Just inlining this in AllowLoad is better imo. > > Compiler complains about declaring variables inside the case statement in > ReportLoad, so letting it remain in a separate function vs declaring the > variables outside of switch. How about renaming this method "GetErrorStringForDisallowedLoad" and just returning the string built by the string builder? https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:22: void emitErrorForDisallowedLoads(const String& url, Document& document) { Since this is semantically the URL, let's just make it a KURL input and call GetString() inside the method. https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:22: void emitErrorForDisallowedLoads(const String& url, Document& document) { Oh also, I think Blink doesn't do camel case functions anymore since the big rename. https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:24: String message = "Subresource filtering disallowed loading this resource, "; Just inline this in Append so we can use the StringView optimizations and not do potentially double allocations. https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:92: if (subresource_filter_->ShouldLogToConsole() && document) { nit: check |document| first in the conditional since it's a simple null check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:21: void emitErrorForDisallowedLoads(const String& url, Document& document) { On 2017/05/22 at 14:35:03, Charlie Harrison wrote: > On 2017/05/22 14:26:27, shivanisha wrote: > > On 2017/05/19 at 17:56:44, Charlie Harrison wrote: > > > Just inlining this in AllowLoad is better imo. > > > > Compiler complains about declaring variables inside the case statement in > > ReportLoad, so letting it remain in a separate function vs declaring the > > variables outside of switch. > > How about renaming this method "GetErrorStringForDisallowedLoad" and just returning the string built by the string builder? done https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:22: void emitErrorForDisallowedLoads(const String& url, Document& document) { On 2017/05/22 at 14:35:04, Charlie Harrison wrote: > Since this is semantically the URL, let's just make it a KURL input and call GetString() inside the method. Done https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:24: String message = "Subresource filtering disallowed loading this resource, "; On 2017/05/22 at 14:35:04, Charlie Harrison wrote: > Just inline this in Append so we can use the StringView optimizations and not do potentially double allocations. Done https://codereview.chromium.org/2895643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:92: if (subresource_filter_->ShouldLogToConsole() && document) { On 2017/05/22 at 14:35:03, Charlie Harrison wrote: > nit: check |document| first in the conditional since it's a simple null check. done
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (left): https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:70: // TODO(csharrison): log console errors here based on Shouldn't we put it here so this will log if enable_logging is set with DRYRUN?
https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (left): https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:70: // TODO(csharrison): log console errors here based on On 2017/05/23 at 20:35:51, Charlie Harrison wrote: > Shouldn't we put it here so this will log if enable_logging is set with DRYRUN? I think for warning list we don't want to log for each subresource but just for the document. WDYT?
still LGTM % one addition https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (left): https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:70: // TODO(csharrison): log console errors here based on On 2017/05/23 20:40:01, shivanisha wrote: > On 2017/05/23 at 20:35:51, Charlie Harrison wrote: > > Shouldn't we put it here so this will log if enable_logging is set with > DRYRUN? > > I think for warning list we don't want to log for each subresource but just for > the document. WDYT? SGTM. Can you add a quick comment to that effect?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@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...
Thanks csharrison@.
https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (left): https://codereview.chromium.org/2895643002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:70: // TODO(csharrison): log console errors here based on On 2017/05/23 at 20:42:04, Charlie Harrison wrote: > On 2017/05/23 20:40:01, shivanisha wrote: > > On 2017/05/23 at 20:35:51, Charlie Harrison wrote: > > > Shouldn't we put it here so this will log if enable_logging is set with > > DRYRUN? > > > > I think for warning list we don't want to log for each subresource but just for > > the document. WDYT? > > SGTM. Can you add a quick comment to that effect? Added a comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2895643002/#ps60001 (title: "Added comment")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by shivanisha@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shivanisha@chromium.org changed reviewers: + rbyers@chromium.org
On 2017/05/25 at 13:01:06, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) rbyers@, PTAL for owner's approval, Thanks!
On 2017/05/25 13:13:21, shivanisha wrote: > On 2017/05/25 at 13:01:06, commit-bot wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > rbyers@, PTAL for owner's approval, Thanks! rbyers please note that the console message is not final (we just want to make sure the plumbing works across our whole component). That particular string will not be shown to developers and is only a placeholder. shivanisha would you mind explaining that in the CL description?
Description was changed from ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. BUG=724549 ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. Note that the string being displayed in this CL is not the final string. This CL is to ensure a place holder and tests are in place for the final string. BUG=724549 ==========
Description was changed from ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. Note that the string being displayed in this CL is not the final string. This CL is to ensure a place holder and tests are in place for the final string. BUG=724549 ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. Note that the string being displayed in this CL is not the final string. This CL is to ensure that a place holder and tests are in place for the final string. BUG=724549 ==========
On 2017/05/25 at 13:26:58, csharrison wrote: > On 2017/05/25 13:13:21, shivanisha wrote: > > On 2017/05/25 at 13:01:06, commit-bot wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > rbyers@, PTAL for owner's approval, Thanks! > > rbyers please note that the console message is not final (we just want to make sure the plumbing works across our whole component). That particular string will not be shown to developers and is only a placeholder. > > shivanisha would you mind explaining that in the CL description? done.
LGTM with possible nit (your call) https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:96: kJSMessageSource, kErrorMessageLevel, nit: I'm missing the context on how subresource filtering is used, but depending on the usage you might want to consider using kInterventionMessageSource. I.e. should the warning show up in the lighthouse as an indication to the developer they're violating some best practice?
The CQ bit was checked by shivanisha@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...
https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:96: kJSMessageSource, kErrorMessageLevel, On 2017/05/25 at 13:52:10, Rick Byers wrote: > nit: I'm missing the context on how subresource filtering is used, but depending on the usage you might want to consider using kInterventionMessageSource. I.e. should the warning show up in the lighthouse as an indication to the developer they're violating some best practice? Thanks, that makes me think it shouldn't be kJSMessageSource also, since any subresource may be blocked not just a JS. So either kOtherMessageSource or kInterventionMessageSource. I haven't used Lighthouse, but does adding a message as kInterventionMessageSource sufficient for Lighthouse or does it also require a reference document. If yes, then I think its fine to just be a kOtherMessageSource console level error for now since we would be adding a reference link there as well, in the final string. Also adding a TODO for considering if this should be added to Lighthouse.
On 2017/05/25 14:35:25, shivanisha wrote: > https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): > > https://codereview.chromium.org/2895643002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:96: > kJSMessageSource, kErrorMessageLevel, > On 2017/05/25 at 13:52:10, Rick Byers wrote: > > nit: I'm missing the context on how subresource filtering is used, but > depending on the usage you might want to consider using > kInterventionMessageSource. I.e. should the warning show up in the lighthouse > as an indication to the developer they're violating some best practice? > > Thanks, that makes me think it shouldn't be kJSMessageSource also, since any > subresource may be blocked not just a JS. So either kOtherMessageSource or > kInterventionMessageSource. I haven't used Lighthouse, but does adding a message > as kInterventionMessageSource sufficient for Lighthouse or does it also require > a reference document. If yes, then I think its fine to just be a > kOtherMessageSource console level error for now since we would be adding a > reference link there as well, in the final string. Also adding a TODO for > considering if this should be added to Lighthouse. Looking at the lighthouse code it seems to me like kInterventionMessageSource currently isn't wired up (see https://github.com/GoogleChrome/lighthouse/issues/1458). You probably just want kOtherMessageSource for now and we can figure out how exactly this fits into lighthouse later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2895643002/#ps80001 (title: "rbyers@ feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks rbyers@
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495728398320710,
"parent_rev": "199d1412c583fb89b734f72a1911d49ecd2dfba5", "commit_rev":
"f6319b29cab58d45f95c0a8f2388cf5650732870"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. Note that the string being displayed in this CL is not the final string. This CL is to ensure that a place holder and tests are in place for the final string. BUG=724549 ========== to ========== [subresource_filter] Display dev tools console message for subresource blocking This CL displays a dev tools message when a sub resource is blocked, subsequent CLs will include messages for subframes and main frame navigations. Note that the string being displayed in this CL is not the final string. This CL is to ensure that a place holder and tests are in place for the final string. BUG=724549 Review-Url: https://codereview.chromium.org/2895643002 Cr-Commit-Position: refs/heads/master@{#474670} Committed: https://chromium.googlesource.com/chromium/src/+/f6319b29cab58d45f95c0a8f2388... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f6319b29cab58d45f95c0a8f2388... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
