Respect enable extenstions setting in Sources and QuickOpenModal - #7705
Conversation
jasonLaster
left a comment
There was a problem hiding this comment.
Thanks! This looks great!
Perhaps we can add some unit tests n SourcesTree.spec.js and QuickOpen.spec.js
| sources = sources.filter( | ||
| source => !(source.url && source.url.startsWith("moz-extension://")) | ||
| ); | ||
| } |
There was a problem hiding this comment.
i think we could move this flag to a selector like getRelativeSources, which will just decide what is shown in the tree and modal.
| relativeUrl: source.url, | ||
| isPrettyPrinted: false, | ||
| isWasm: false, | ||
| isExtension: source.url && source.url.startsWith("moz-extension://"), |
There was a problem hiding this comment.
Does this address the extensions from Chrome browser?
There was a problem hiding this comment.
No, I've just fixes that - fixed regexp and moved this set to common source action creator
|
@jasonLaster Currently filtering extensions is the responsibility of I've prepared the reducers test here, but I'm not sure if it's a good approach. Edit: I'll update snapshots (missing |
|
this looks good |
|
failed test: devtools/client/debugger/new/test/mochitest/browser_dbg-content-script-sources.js two mochitests fail -- hopefully the errors are easy to follow... |
| } | ||
|
|
||
| export function isUrlExtension(url: string) { | ||
| return /^(chrome|moz)-extension:\/\//.test(url); |
There was a problem hiding this comment.
@konradk Just a heads up: I also have at least one extension (namely User Agent Switcher) whose scheme seems to end by only one trailing slash and not two. The above regex tests with two trailing slashes at the end.
See screenshot:
Pinging also @AnshulMalik because I think this is probably relevant for https://bugzilla.mozilla.org/show_bug.cgi?id=1518221 as well.
Note sure how to handle this (and why this is under blob://) but testing against
// one trailing slash
return /^(chrome|moz)-extension:\//.test(url);and not
// two trailing slashes
return /^(chrome|moz)-extension:\/\//.test(url);would be worthwhile.
If so, probably also good if a test would be added for this edge case.
There was a problem hiding this comment.
@gerdneuman Nice catch, I will fix that.
There was a problem hiding this comment.
Not sure why it's blob and has just one /
263c8f0 to
1a0d2fe
Compare
|
Excellent work @konradk ! Thank you! |
Fixes #7531
Summary of Changes
Test Plan
My questions:
newSourcesaction, because that's only action I've seen in development mode. Is there any other action I should edit - e.g.newSource?url.startsWith("moz-extension://"))?To do: