Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692653003:
[Devtools] Added Enable/Disable for request blocking in network (Closed)
|
Created:
3 years, 10 months ago by allada Modified:
3 years, 9 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Added Enable/Disable for request blocking in network
* Added Enable/Disable button in network blocking drawer.
* Persists the network blocking entries between devtools reloads.
* Changed code to use setBlockedURLs instead of add/remove counterparts.
* Moved icon notifier from main to network module.
see: http://imgur.com/a/G0VFI
R=pfeldman
BUG=691164
Review-Url: https://codereview.chromium.org/2692653003
Cr-Commit-Position: refs/heads/master@{#455986}
Committed: https://chromium.googlesource.com/chromium/src/+/01c043f5f744262981becd3d45a79981ad3f6b67
Patch Set 1 : Merge branch 'BLOCK_REQUEST_REMOVE_CONTEXT_MENU' into ADD_ENABLE_DISABLE_REQUEST_BLOCKINg #
Total comments: 8
Patch Set 2 : changes #
Total comments: 4
Patch Set 3 : changes #
Total comments: 4
Patch Set 4 : changes #
Total comments: 6
Patch Set 5 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into ADD_ENABLE_DISABLE_REQ… #
Total comments: 1
Messages
Total messages: 72 (48 generated)
Patchset #1 (id:1) has been deleted
PTL
The CQ bit was checked by allada@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...
You should not need changes to backend.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
PTaL
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js (right): https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:122: } else if (SDK.multitargetNetworkManager.isRequestBlockingEnabled() && blockedURLsSetting.get().length) { You don't need to check the blocked url length. setting is on -> blocking is engaged -> warning. https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:126: UI.inspectorView.setPanelIcon('network', icon); this.name https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:882: this._blockedURLs.forEach(url => agent.addBlockedURL(url)); Let's follow up immediately with converting add/removeBlockedURL to setBlockedURLs. https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:946: SDK.MultitargetNetworkManager.RequestBlockingEnabledChangedEvent = class { Seems like a lot of boilerplate for a simple listener. How about pass nothing in the event, use old events and expose requestBlockingEnabled() ?
Patchset #3 (id:180001) has been deleted
PTL https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js (right): https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:122: } else if (SDK.multitargetNetworkManager.isRequestBlockingEnabled() && blockedURLsSetting.get().length) { On 2017/02/13 18:24:26, pfeldman wrote: > You don't need to check the blocked url length. setting is on -> blocking is > engaged -> warning. I thought it'd be better to allow the user to keep the setting on but have nothing in the list to filter out. In which case we should only show the warning when it "might" actually filter stuff out. https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:126: UI.inspectorView.setPanelIcon('network', icon); On 2017/02/13 18:24:26, pfeldman wrote: > this.name Done. https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:882: this._blockedURLs.forEach(url => agent.addBlockedURL(url)); On 2017/02/13 18:24:26, pfeldman wrote: > Let's follow up immediately with converting add/removeBlockedURL to > setBlockedURLs. Done. see: https://codereview.chromium.org/2703603003 https://codereview.chromium.org/2692653003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:946: SDK.MultitargetNetworkManager.RequestBlockingEnabledChangedEvent = class { On 2017/02/13 18:24:26, pfeldman wrote: > Seems like a lot of boilerplate for a simple listener. How about pass nothing in > the event, use old events and expose requestBlockingEnabled() ? How about this as a compromise.
Patchset #3 (id:200001) has been deleted
The CQ bit was checked by allada@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...
Patchset #3 (id:220001) has been deleted
The CQ bit was checked by allada@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js (right): https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:120: function updateVisibility() { You need to call it immediately as well - what if the setting has been already modified. https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:127: icon.title = Common.UIString('Requests may be blocked'); In this case you can go even further and report the actual blockings. Otherwise you deliver something in the middle.
PTaL https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js (right): https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:120: function updateVisibility() { On 2017/02/22 00:02:46, pfeldman wrote: > You need to call it immediately as well - what if the setting has been already > modified. Done. https://codereview.chromium.org/2692653003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:127: icon.title = Common.UIString('Requests may be blocked'); On 2017/02/22 00:02:46, pfeldman wrote: > In this case you can go even further and report the actual blockings. Otherwise > you deliver something in the middle. We report which requests are blocked in the network log itself. This is supposed to symbolize that new requests may be blocked.
https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js (right): https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:27: var enableRequestBlockingCheckbox = new UI.ToolbarCheckbox( You can now use session setting for this! https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:30: SDK.multitargetNetworkManager.on( And you would not need this.
Description was changed from ========== [Devtools] Added Enable/Disable for request blocking in network This patch adds the ability to enable and disable url request blocking in the Request blocking panel in drawer. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ========== to ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. see: http://imgur.com/a/n1Tbt This patch adds the ability to enable and disable url request blocking in the Request blocking panel in drawer. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ==========
Description was changed from ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. see: http://imgur.com/a/n1Tbt This patch adds the ability to enable and disable url request blocking in the Request blocking panel in drawer. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ========== to ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ==========
Description was changed from ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ========== to ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. * Moved icon notifier from main to network module. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ==========
PTaL https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js (right): https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:27: var enableRequestBlockingCheckbox = new UI.ToolbarCheckbox( On 2017/03/06 18:44:59, pfeldman wrote: > You can now use session setting for this! Done. https://codereview.chromium.org/2692653003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:30: SDK.multitargetNetworkManager.on( On 2017/03/06 18:44:59, pfeldman wrote: > And you would not need this. Done.
lgtm % comments https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:28: Common.moduleSetting('requestBlockingEnabled'), Common.UIString('Enables request blocking'), If you fined setting in the descriptor, titles are propagated automatically, no need for those extra args. https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/module.json (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/module.json:57: "title": "Block matched requests from being sent" I'd just say "Enable request blocking" as you did above (and would drop it from there). https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:694: this._blockedEnabledSetting = Common.moduleSetting('requestBlockingEnabled'); If you use it here, you need to declare it in this module (sdk). We are leaking this UI aspect into sdk here, but I guess it is fine for now - settings are headless.
done. https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/BlockedURLsPane.js:28: Common.moduleSetting('requestBlockingEnabled'), Common.UIString('Enables request blocking'), On 2017/03/07 02:37:56, pfeldman wrote: > If you fined setting in the descriptor, titles are propagated automatically, no > need for those extra args. Done. https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/module.json (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/module.json:57: "title": "Block matched requests from being sent" On 2017/03/07 02:37:56, pfeldman wrote: > I'd just say "Enable request blocking" as you did above (and would drop it from > there). Done. https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/2692653003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:694: this._blockedEnabledSetting = Common.moduleSetting('requestBlockingEnabled'); On 2017/03/07 02:37:56, pfeldman wrote: > If you use it here, you need to declare it in this module (sdk). We are leaking > this UI aspect into sdk here, but I guess it is fine for now - settings are > headless. Done.
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2692653003/#ps380001 (title: "changes")
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2692653003/#ps400001 (title: "fix test")
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2692653003/#ps420001 (title: "headless tests")
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 allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
allada@chromium.org changed reviewers: + alexclarke@chromium.org
+alexclarke@ can I get a review of the headless test please?
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...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
headless/ LGTM
Patchset #1 (id:20001) has been deleted
Patchset #7 (id:340001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #6 (id:400001) has been deleted
Patchset #5 (id:380001) has been deleted
Patchset #5 (id:420001) has been deleted
Patchset #5 (id:440001) has been deleted
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2692653003/#ps460001 (title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src into ADD_ENABLE_DISABLE_REQ…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1489109692579030,
"parent_rev": "4716e3f6c36011134d73a07160e2eb5815abf1ff", "commit_rev":
"0837150277d852ff25f62b5e10f9bb502b708887"}
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1489109692579030,
"parent_rev": "2480b8fee48d2a70bb1dcc9eb4ff37c6a3f95d57", "commit_rev":
"01c043f5f744262981becd3d45a79981ad3f6b67"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. * Moved icon notifier from main to network module. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 ========== to ========== [Devtools] Added Enable/Disable for request blocking in network * Added Enable/Disable button in network blocking drawer. * Persists the network blocking entries between devtools reloads. * Changed code to use setBlockedURLs instead of add/remove counterparts. * Moved icon notifier from main to network module. see: http://imgur.com/a/G0VFI R=pfeldman BUG=691164 Review-Url: https://codereview.chromium.org/2692653003 Cr-Commit-Position: refs/heads/master@{#455986} Committed: https://chromium.googlesource.com/chromium/src/+/01c043f5f744262981becd3d45a7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:460001) as https://chromium.googlesource.com/chromium/src/+/01c043f5f744262981becd3d45a7...
Message was sent while issue was closed.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2692653003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js (right): https://codereview.chromium.org/2692653003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:124: function updateIconVisibility() { Note this does not work in case you do throttling in Device Mode, Performance panel or Application panel. |
