Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2316383002:
PlzNavigate: only allow main frame resource loads for extensions (Closed)
DescriptionPlzNavigate: only allow main frame resource loads for extensions
This makes these two tests pass:
ExtensionProtocolTest.AllowFrameRequests
ExtensionProtocolTest.IncognitoRequest
They look somewhat comprehensive from looking back at crbug.com/312269
and crbug.com/576867.
But this change also feels vaguely wrong in that I feel like it should
probably be the ExtensionNavigationThrottle doing some blocking. If you
have any feedback on what the tests should be doing instead (creating a
NavigationHandle somehow instead of directly creating URLRequests so
that it gets a full complement of navigation throttles?) it would be
greatly appreciated. :)
R=nasko@chromium.org
BUG=510836
Committed: https://crrev.com/c1dbe802a3ee22c08b1e19fcdd0d1395ab9d18ee
Cr-Commit-Position: refs/heads/master@{#417614}
Patch Set 1 #
Depends on Patchset: Messages
Total messages: 20 (11 generated)
Description was changed from ========== PlzNavigate: only allow main frame resource loads This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ========== to ========== PlzNavigate: only allow main frame resource loads This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... ExtensionProtocolTest.IncognitoRequest: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ==========
Description was changed from ========== PlzNavigate: only allow main frame resource loads This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... ExtensionProtocolTest.IncognitoRequest: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ========== to ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... ExtensionProtocolTest.IncognitoRequest: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ==========
nasko@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding Devlin, who can likely answer the test structure questions better than myself. The fix itself looks like an improvement to me, so it looks good.
On 2016/09/08 00:09:45, nasko (slow) wrote: > Adding Devlin, who can likely answer the test structure questions better than > myself. > > The fix itself looks like an improvement to me, so it looks good. Sorry, I didn't get a chance to get to this today. I'll make sure I take a look tomorrow.
lgtm In regards to the tests, I agree with you that many of these checks should be in a test for the ExtensionNavigationThrottle, but one of those doesn't exist. It looks like it's not terribly difficult to get the required setup - https://cs.chromium.org/chromium/src/chrome/browser/plugins/flash_download_in... seems to be a good example of what to do. That said, there's another bug I'll be working on (crbug.com/645028) that will also require adjusting the ExtensionNavigationThrottle, so I'm happy to take over the test conversion work as part of that since it's orthogonal to this CL. :) CL description nit: when you reference the tests, can you use chromium git urls (i.e. ttps://chromium.googlesource.com/chromium/src/+/<revision>/...) so that they are valid once files change?
Description was changed from ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... ExtensionProtocolTest.IncognitoRequest: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_prot... They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ========== to ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ==========
On 2016/09/09 15:03:28, Devlin wrote: > lgtm > > In regards to the tests, I agree with you that many of these checks should be in > a test for the ExtensionNavigationThrottle, but one of those doesn't exist. It > looks like it's not terribly difficult to get the required setup - > https://cs.chromium.org/chromium/src/chrome/browser/plugins/flash_download_in... > seems to be a good example of what to do. That said, there's another bug I'll > be working on (crbug.com/645028) that will also require adjusting the > ExtensionNavigationThrottle, so I'm happy to take over the test conversion work > as part of that since it's orthogonal to this CL. :) OK, thanks. > > CL description nit: when you reference the tests, can you use chromium git urls > (i.e. ttps://chromium.googlesource.com/chromium/src/+/<revision>/...) so that > they are valid once files change? Removed links, they were mostly noise anyway.
The CQ bit was checked by scottmg@chromium.org
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ 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 scottmg@chromium.org
The CQ bit was checked by scottmg@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 ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ========== to ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 ========== to ========== PlzNavigate: only allow main frame resource loads for extensions This makes these two tests pass: ExtensionProtocolTest.AllowFrameRequests ExtensionProtocolTest.IncognitoRequest They look somewhat comprehensive from looking back at crbug.com/312269 and crbug.com/576867. But this change also feels vaguely wrong in that I feel like it should probably be the ExtensionNavigationThrottle doing some blocking. If you have any feedback on what the tests should be doing instead (creating a NavigationHandle somehow instead of directly creating URLRequests so that it gets a full complement of navigation throttles?) it would be greatly appreciated. :) R=nasko@chromium.org BUG=510836 Committed: https://crrev.com/c1dbe802a3ee22c08b1e19fcdd0d1395ab9d18ee Cr-Commit-Position: refs/heads/master@{#417614} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c1dbe802a3ee22c08b1e19fcdd0d1395ab9d18ee Cr-Commit-Position: refs/heads/master@{#417614} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
