As Addon SDK's PageMod, WebExtensions' content scripts should be detected as new sources in the tab developer tools. Once the content scripts are detected as new sources, it will be possible to list them in the Debugger tool from the Tab developer toolbox and to set breakpoints on them.

Review commit: https://reviewboard.mozilla.org/r/44125/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44125/

https://reviewboard.mozilla.org/r/44125/#review40697 ::: toolkit/components/extensions/ExtensionContent.jsm:335 (Diff revision 1) > + // sandbox metadata is needed to be recognized and supported in > + // the Developer Tools of the tab where the content script is running. > + let innerWindowID = windowId(contentWindow); > + let metadata = { > + WebExtensionContentScript: true, > + 'inner-window-id': innerWindowID, This Sandbox metadata object is then used in the "webbrowser" RDP actor to decide if the new global should be added to the debuggees: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1012

Review commit: https://reviewboard.mozilla.org/r/44127/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44127/

https://reviewboard.mozilla.org/r/44123/#review40715 ::: toolkit/components/extensions/ExtensionContent.jsm:355 (Diff revisions 1 - 2) > + // add the content global to the devtools tracked globals > + this.devtoolsGlobalDetails = { > + global: this.sandbox, > + "inner-window-id": innerWindowID, > + }; > + addContentGlobal(this.devtoolsGlobalDetails); By using "addContentGlobal", we will help the devtools server to keep track of the content scripts' sandboxes associated to a window (which in the long run would make the devtools able to list content scripts injected before the developer toolbox would be opened, which seems that it is not currently implemented, even for the PageMod which was already supported)

https://reviewboard.mozilla.org/r/44125/#review41283 ::: toolkit/components/extensions/ExtensionContent.jsm:334 (Diff revision 1) > } else { > + // sandbox metadata is needed to be recognized and supported in > + // the Developer Tools of the tab where the content script is running. > + let innerWindowID = windowId(contentWindow); > + let metadata = { > + WebExtensionContentScript: true, WebExtensionContentScript metadata doesn't seem to be used anywhere?

(In reply to Alexandre Poirot [:ochameau] from comment #5) > https://reviewboard.mozilla.org/r/44125/#review41283 > > ::: toolkit/components/extensions/ExtensionContent.jsm:334 > (Diff revision 1) > > } else { > > + // sandbox metadata is needed to be recognized and supported in > > + // the Developer Tools of the tab where the content script is running. > > + let innerWindowID = windowId(contentWindow); > > + let metadata = { > > + WebExtensionContentScript: true, > > WebExtensionContentScript metadata doesn't seem to be used anywhere? It is not currently used, but I thought it could be helpful to be able to detect when the content script comes from a WebExtension or a Addon SDK addon. The Addon SDK content script sandboxes has the following metadata [1]: > metadata: { > SDKContentScript: true, > 'inner-window-id': getInnerId(window) > } [1]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#152

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/1-2/

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag https://reviewboard.mozilla.org/r/44125/#review42111 It seems like we should be testing that these messages are actually directed to the extension debugger, rather than just that the sandboxes have the expected metadata. Are there already tests for this in other types of add-ons? Can we do something similar? ::: toolkit/components/extensions/ExtensionContent.jsm:338 (Diff revision 2) > }); > } else { > + // sandbox metadata is needed to be recognized and supported in > + // the Developer Tools of the tab where the content script is running. > + let metadata = { > + WebExtensionContentScript: true, Typically this would be `webExtensionContentScript`. Can we wait to add this until it's needed? ::: toolkit/components/extensions/ExtensionContent.jsm:573 (Diff revision 2) > > + getContentScriptGlobalsForWindow(window) { > + let winId = getInnerWindowID(window); > + let extensions = this.contentScriptWindows.get(winId); > + > + if (extensions instanceof Map) { `if (extensions)` would be preferable. ::: toolkit/components/extensions/ExtensionContent.jsm:574 (Diff revision 2) > + getContentScriptGlobalsForWindow(window) { > + let winId = getInnerWindowID(window); > + let extensions = this.contentScriptWindows.get(winId); > + > + if (extensions instanceof Map) { > + return Array.from(extensions.values()).map((c) => c.sandbox); `return Array.from(extensions.values(), context => context.sandbox)` ::: toolkit/components/extensions/ExtensionContent.jsm:896 (Diff revision 2) > this.globals.get(global).uninit(); > this.globals.delete(global); > }, > + > + getContentScriptGlobalsForWindow(window) { > + return DocumentManager.getContentScriptGlobalsForWindow(window); Why is this necessary? Or, rather, why do we need either of these functions, let alone both? ::: toolkit/components/extensions/test/mochitest/test_ext_contentScript_devtools_metadata.html:18 (Diff revision 2) > +<script type="text/javascript"> > +"use strict"; > + > +add_task(function* test_contentscript_devtools_sandbox_metadata() { > + function contentScript() { > + console.log("contentScript executed"); Is this necessary? It doesn't seem to be used. ::: toolkit/components/extensions/test/mochitest/test_ext_contentScript_devtools_metadata.html:64 (Diff revision 2) > + > + const {ExtensionContent} = SpecialPowers.Cu.import( > + "resource://gre/modules/ExtensionContent.jsm", {} > + ); > + > + let res, metadata; Please declare these at their first assignment. ::: toolkit/components/extensions/test/mochitest/test_ext_contentScript_devtools_metadata.html:66 (Diff revision 2) > + "resource://gre/modules/ExtensionContent.jsm", {} > + ); > + > + let res, metadata; > + > + res = ExtensionContent.getContentScriptGlobalsForWindow(win); This relies on this code running in the same process as the content script, which we shouldn't necessarily rely on.

(In reply to Kris Maglione [:kmag] from comment #8) > Comment on attachment 8737808 [details] > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > ContentScripts in the Tab DevTools Debugger. r?kmag > > https://reviewboard.mozilla.org/r/44125/#review42111 > > It seems like we should be testing that these messages are actually directed > to the extension debugger, rather than just that the sandboxes have the > expected metadata. Are there already tests for this in other types of > add-ons? Can we do something similar? I tried to model this test case on the existent one (which tests AddonSDK's PageMod sandbox metadata): - https://dxr.mozilla.org/mozilla-central/source/devtools/shared/tests/mochitest/test_devtools_extensions.html And limit as much as possible the interdependencies, nevertheless I'd like to add more tests in followups. > > ::: toolkit/components/extensions/ExtensionContent.jsm:338 > (Diff revision 2) > > }); > > } else { > > + // sandbox metadata is needed to be recognized and supported in > > + // the Developer Tools of the tab where the content script is running. > > + let metadata = { > > + WebExtensionContentScript: true, > > Typically this would be `webExtensionContentScript`. How about webExtensionsContentScript? or webextContentScript? > Can we wait to add this until it's needed? I think so, it is not currently used anywhere, I added it to differentiate WebExtensions' ContentScript from Addon SDK ones: > metadata: { > SDKContentScript: true, > 'inner-window-id': getInnerId(window) > } From: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#152 > ::: toolkit/components/extensions/ExtensionContent.jsm:896 > (Diff revision 2) > > this.globals.get(global).uninit(); > > this.globals.delete(global); > > }, > > + > > + getContentScriptGlobalsForWindow(window) { > > + return DocumentManager.getContentScriptGlobalsForWindow(window); > > Why is this necessary? Or, rather, why do we need either of these functions, > let alone both? I defined the method DocumentManager.getContentScriptGlobalsForWindow because the data structure which contains the needed info is intended to be part of the DocumentManager's private state. it is exported as an helper of the exported object only to not expose the entire DocumentManager singleton to the outsite world. I created this new helper after a preliminary discussion about this patch on IRC with :ochameau - the 'content-globals' module (https://dxr.mozilla.org/mozilla-central/source/devtools/server/content-globals.js) is currently unused, and it is probably going to be removed soon - it is preferred an helper exported from the ExtensionContent.jsm which will be integrated in the devtools actor to be able to retrieve the existent WebExtensions ContentScripts of a target window > ::: > toolkit/components/extensions/test/mochitest/ > test_ext_contentScript_devtools_metadata.html:18 > (Diff revision 2) > > +<script type="text/javascript"> > > +"use strict"; > > + > > +add_task(function* test_contentscript_devtools_sandbox_metadata() { > > + function contentScript() { > > + console.log("contentScript executed"); > > Is this necessary? It doesn't seem to be used. it is just a tracing log that I forgot to remove, but it is not necessary, I'll remove it. > ::: > toolkit/components/extensions/test/mochitest/ > test_ext_contentScript_devtools_metadata.html:66 > (Diff revision 2) > > + "resource://gre/modules/ExtensionContent.jsm", {} > > + ); > > + > > + let res, metadata; > > + > > + res = ExtensionContent.getContentScriptGlobalsForWindow(win); > > This relies on this code running in the same process as the content script, > which we shouldn't necessarily rely on. The helper is intended to be always used from the same process (the debugger server code which is always running in the same process as the target tab), I'd like to keep this basic test case as simple as possible, when the test case and the content script are going to run in different processes? any alternative approaches that are you thinking of?

(In reply to Luca Greco [:rpl] from comment #9) > (In reply to Kris Maglione [:kmag] from comment #8) > > Comment on attachment 8737808 [details] > > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > > ContentScripts in the Tab DevTools Debugger. r?kmag > > > > https://reviewboard.mozilla.org/r/44125/#review42111 > > > > It seems like we should be testing that these messages are actually directed > > to the extension debugger, rather than just that the sandboxes have the > > expected metadata. Are there already tests for this in other types of > > add-ons? Can we do something similar? > > I tried to model this test case on the existent one (which tests AddonSDK's > PageMod sandbox metadata): > - > https://dxr.mozilla.org/mozilla-central/source/devtools/shared/tests/ > mochitest/test_devtools_extensions.html > > And limit as much as possible the interdependencies, nevertheless I'd like > to add more tests in followups. I find this test quite good to just check that WebExtension code correctly flags Sandboxes metadata. The only thing that is disturbing is that it lives in /devtools/ whereas it is more a webextension test. May be we could move it there? I'm currently working on providing a template to test Addon toolboxes, which is quite challenging as they live in its own gecko process, not even a child process! I think we should be able to test that from end to end once this is ready. See bug 1261055. > > > > > ::: toolkit/components/extensions/ExtensionContent.jsm:338 > > (Diff revision 2) > > > }); > > > } else { > > > + // sandbox metadata is needed to be recognized and supported in > > > + // the Developer Tools of the tab where the content script is running. > > > + let metadata = { > > > + WebExtensionContentScript: true, > > > > Typically this would be `webExtensionContentScript`. > > How about webExtensionsContentScript? or webextContentScript? > > > > Can we wait to add this until it's needed? > > I think so, it is not currently used anywhere, I added it to differentiate > WebExtensions' ContentScript from Addon SDK ones: +1. I dislike introducing "nice to have, just in case" code that aren't used. Later you then wonder what it is about, and if you have to change it there is no test and wonder what imaginary thing you may break. > > ::: toolkit/components/extensions/ExtensionContent.jsm:896 > > (Diff revision 2) > > > this.globals.get(global).uninit(); > > > this.globals.delete(global); > > > }, > > > + > > > + getContentScriptGlobalsForWindow(window) { > > > + return DocumentManager.getContentScriptGlobalsForWindow(window); > > > > Why is this necessary? Or, rather, why do we need either of these functions, > > let alone both? > > I created this new helper after a preliminary discussion about this patch on > IRC with :ochameau I think it will be clearer if you also attach the related /devtools/ patch. > > toolkit/components/extensions/test/mochitest/ > > test_ext_contentScript_devtools_metadata.html:66 > > (Diff revision 2) > > > + "resource://gre/modules/ExtensionContent.jsm", {} > > > + ); > > > + > > > + let res, metadata; > > > + > > > + res = ExtensionContent.getContentScriptGlobalsForWindow(win); > > > > This relies on this code running in the same process as the content script, > > which we shouldn't necessarily rely on. > > The helper is intended to be always used from the same process (the debugger > server code which is always running > in the same process as the target tab), I'd like to keep this basic test > case as simple as possible, > when the test case and the content script are going to run in different > processes? > any alternative approaches that are you thinking of? I think it is just disturbing. There is some many privileged code involved that you may easily think it runs in parent process. It looks like a mochitest-chrome, whereas it is just a mochitest-plain, running in child process and all privileged code also runs here. I agree it is disturbing, but I don't see what to do differently. May be webextensions test templates are better than this devtools one?

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Looks good to me. But I would like to see how it integrates with the devtools part.

Review commit: https://reviewboard.mozilla.org/r/45767/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45767/

Review commit: https://reviewboard.mozilla.org/r/45769/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45769/

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/2-3/

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/3-4/

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45767/diff/1-2/

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/1-2/

https://reviewboard.mozilla.org/r/44125/#review42111 > Typically this would be `webExtensionContentScript`. > > Can we wait to add this until it's needed? removed in the last revision of the first patch (and from its test case).

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/4-5/

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45767/diff/2-3/

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/2-3/

My apologies Alex and Kris for the amount of "the Review request updated" noise, I noticed that not all the suggestions from the review comments were integrated and so I have pushed two more updates. (In reply to Alexandre Poirot [:ochameau] from comment #10) > I find this test quite good to just check that WebExtension code correctly > flags Sandboxes metadata. > The only thing that is disturbing is that it lives in /devtools/ whereas it > is more a webextension test. May be we could move it there? The new test case is modelled after the one which lives in /devtools/ but it is actually a totally new test file in the /toolkit/components/extensions/ directory. > > any alternative approaches that are you thinking of? > > I think it is just disturbing. There is some many privileged code involved > that you may easily think it runs in parent process. It looks like a > mochitest-chrome, whereas it is just a mochitest-plain, running in child > process and all privileged code also runs here. > I agree it is disturbing, but I don't see what to do differently. > May be webextensions test templates are better than this devtools one? As the one in /devtools/, the new test case is a mochitest-plain test file, I'm open to tweak it further if there is anything that I can do/look into (I added a new test case which test that the source is actually loaded in the debugger panel, but I like that this additional test case does its checks in a simpler way and that it tests the addon unloaded scenario) (In reply to Alexandre Poirot [:ochameau] from comment #11) > Comment on attachment 8737808 [details] > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > ContentScripts in the Tab DevTools Debugger. r?kmag > > Looks good to me. But I would like to see how it integrates with the > devtools part. I've pushed two more patches which add the devtools part (and a new test case): - attachment 8740441 [details], which introduces the new 'ExtensionContent.getContentScriptGlobalsForWindow' helper in the 'webbrowser.js' actor module - attachment 8740442 [details], which add a new test case which ensures that the content script from a webextensions addon is actually loaded in the debugger panel In attachment 870442 I added a new test case as a new mochitest-browser test file in the '/devtools/client/debugger/test/mochitest' dir, mostly because this test dir already contains a number of useful test helper to prepare the test case with a short number of lines of code. The WebExtensions addon has been packaged as an xpi (added as a binary in the last patch, but I added the addon sources as well in the 'addon-source/browser_dbg_addon_webext_contentscript/' dir), as the other addon that are used in the other tests from the same dir.

(In reply to Luca Greco [:rpl] from comment #9) > (In reply to Kris Maglione [:kmag] from comment #8) > > Comment on attachment 8737808 [details] > > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > > ContentScripts in the Tab DevTools Debugger. r?kmag > > > > https://reviewboard.mozilla.org/r/44125/#review42111 > > > > It seems like we should be testing that these messages are actually directed > > to the extension debugger, rather than just that the sandboxes have the > > expected metadata. Are there already tests for this in other types of > > add-ons? Can we do something similar? > > I tried to model this test case on the existent one (which tests AddonSDK's > PageMod sandbox metadata): > - > https://dxr.mozilla.org/mozilla-central/source/devtools/shared/tests/ > mochitest/test_devtools_extensions.html > > And limit as much as possible the interdependencies, nevertheless I'd like > to add more tests in followups. > > > Typically this would be `webExtensionContentScript`. > > How about webExtensionsContentScript? or webextContentScript? We already have a sandbox property called `isWebExtensionContentScript`, so if for some reason we need to add another one, I think it should be as similar as possible. > it is exported as an helper of the exported object only to not expose the > entire DocumentManager singleton to the outsite world. > > I created this new helper after a preliminary discussion about this patch on > IRC with :ochameau > > - the 'content-globals' module > (https://dxr.mozilla.org/mozilla-central/source/devtools/server/content- > globals.js) > is currently unused, and it is probably going to be removed soon > - it is preferred an helper exported from the ExtensionContent.jsm which > will be integrated in the devtools actor to > be able to retrieve the existent WebExtensions ContentScripts of a target > window OK. Can you please add a comment explaining its purpose and where it's meant to be used? > The helper is intended to be always used from the same process (the debugger > server code which is always running in the same process as the target tab), > I'd like to keep this basic test case as simple as possible, when the test > case and the content script are going to run in different processes? In the future, there's likely to be more than one content process, so it's generally best not to assume that any two windows will be in the same one. But since the window in question is created with `window.open`, I guess it should be safe in this case. (In reply to Alexandre Poirot [:ochameau] from comment #10) > The only thing that is disturbing is that it lives in /devtools/ whereas it > is more a webextension test. May be we could move it there? Agree. > I'm currently working on providing a template to test Addon toolboxes, which > is quite challenging as they live in its own gecko process, not even a child > process! > I think we should be able to test that from end to end once this is ready. > See bug 1261055. OK. I agree it makes sense to wait for that bug.

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag https://reviewboard.mozilla.org/r/44125/#review42429 ::: toolkit/components/extensions/ExtensionContent.jsm:894 (Diff revision 5) > uninit(global) { > this.globals.get(global).uninit(); > this.globals.delete(global); > }, > + > + getContentScriptGlobalsForWindow(window) { Please add a comment here, as mentioned in comment 23.

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau https://reviewboard.mozilla.org/r/45767/#review43151 Works great! So cool to finally see content scripts in debugger!! Sorry for the delay, I took some time to play with your patches. ::: devtools/server/actors/webbrowser.js:723 (Diff revision 3) > > this._shouldAddNewGlobalAsDebuggee = this._shouldAddNewGlobalAsDebuggee.bind(this); > > this.makeDebugger = makeDebugger.bind(null, { > - findDebuggees: () => this.windows, > + findDebuggees: () => { > + return [].concat(this.windows, this.webextensionsContentScriptGlobals); Can't we just do: return this.windows.concat(this.webextensionContentScriptGlobals); ? ::: devtools/server/actors/webbrowser.js:816 (Diff revision 3) > /** > + * Getter for the WebExtensions ContentScript globals related to the > + * current tab content's DOM window. > + */ > + get webextensionsContentScriptGlobals() { > + if (this.window) { It might be helpful to say we ignore xpcshell here.

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau https://reviewboard.mozilla.org/r/45769/#review43153 ::: devtools/client/debugger/test/mochitest/browser.ini:472 (Diff revision 3) > [browser_dbg_sources-sorting.js] > skip-if = e10s && debug > [browser_dbg_sources-bookmarklet.js] > skip-if = e10s && debug > +[browser_dbg_sources-webext-contentscript.js] > +skip-if = e10s && debug # TODO: double-check if it is necessary Please do so before landing. It's not a big deal if you have the same skip-if than all other tests. ::: devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js:10 (Diff revision 3) > + > +/** > + * Make sure eval scripts appear in the source list > + */ > + > +const TAB_URL = EXAMPLE_URL + "doc_script_webext_contentscript.html"; That's unfortunate Content scripts don't match data: or about:blank. At least Chrome one doesn't. I imagine we replicated their behavior? But they seem to agree on targeting these URL. https://bugs.chromium.org/p/chromium/issues/detail?id=55084 If we do, we could write simplier tests without custom documents. Please do not try to fix that here ;-) ::: devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js:24 (Diff revision 3) > + gPanel = aPanel; > + gDebugger = gPanel.panelWin; > + gSources = gDebugger.DebuggerView.Sources; > + > + return Task.spawn(function* () { > + yield waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCE_SHOWN); Did you run it on debug build? I'm wondering if with some additional delay due to content scripts... the source will always be ready on time? ::: devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js:29 (Diff revision 3) > + yield waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCE_SHOWN); > + > + is(gSources.values.length, 1, "Should have 1 source"); > + > + let item = gSources.getItemForAttachment(attachment => { > + return attachment.source.url.indexOf("moz-extension") == 0; We now have String.includes to replace indexOf != -1. It's not a strict equivalent here but I think it would be enough? ::: devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js:33 (Diff revision 3) > + let item = gSources.getItemForAttachment(attachment => { > + return attachment.source.url.indexOf("moz-extension") == 0; > + }); > + > + ok(item, "Got the expected WebExtensions ContentScript source"); > + ok(item.attachment.source.url.indexOf(item.attachment.group) >= 0, Same here.

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/5-6/

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45767/diff/3-4/

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/3-4/

https://reviewboard.mozilla.org/r/44125/#review42429 > Please add a comment here, as mentioned in comment 23. Added an inline comment (and fixed an eslint error) in the last pushed version (https://reviewboard.mozilla.org/r/44125/diff/5-6/)

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/6-7/

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45767/diff/4-5/

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/4-5/

(In reply to Luca Greco [:rpl] from comment #31) > Comment on attachment 8737808 [details] > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > ContentScripts in the Tab DevTools Debugger. r=kmag > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/44125/diff/6-7/ Push updated patch with more eslint fixed errors/warnings.

https://reviewboard.mozilla.org/r/45767/#review43151 > Can't we just do: > return this.windows.concat(this.webextensionContentScriptGlobals); > ? sure, fixed in last pushed updates (https://reviewboard.mozilla.org/r/45767/diff/3-5/)

https://reviewboard.mozilla.org/r/45769/#review43153 > Same here. both fixed in the last pushed updates (https://reviewboard.mozilla.org/r/45769/diff/3-5/)

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/5-6/

https://reviewboard.mozilla.org/r/45769/#review43153 > Did you run it on debug build? > I'm wondering if with some additional delay due to content scripts... the source will always be ready on time? I tried the this test case on a debug build (both locally and in a [try push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f20549e76a7)). locally I tried to both run it with "--run-until-failure" and the MOZ_CHAOSMODE set. In the last push I tweaked the test case a bit to ensure that it always remove the addon, close the devtools and reach the "finish()" call, even in case of failure or timeouts). As it seems to run successfully with e10s and debug, I removed the skip-if in the browser.ini file.

https://reviewboard.mozilla.org/r/45769/#review43153 > That's unfortunate Content scripts don't match data: or about:blank. > At least Chrome one doesn't. I imagine we replicated their behavior? > But they seem to agree on targeting these URL. > https://bugs.chromium.org/p/chromium/issues/detail?id=55084 > > If we do, we could write simplier tests without custom documents. > Please do not try to fix that here ;-) Unfortunately "match_about_blank" is not currently implemented (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#190), but I agree that we can re-evaluate this when we will introduce it.

https://reviewboard.mozilla.org/r/45769/#review43153 > Please do so before landing. > It's not a big deal if you have the same skip-if than all other tests. skip-if removed in the last update, I tried both locally and in a try push: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f20549e76a7

https://reviewboard.mozilla.org/r/44123/#review43505 In the last update I tweaked this test to make it better in handling issues (e.g. if something goes wrong it should always "fail and cleanup", 'cause it is always better to get a clear failure than an enigmatic timeout). ::: devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js:38 (Diff revisions 8 - 9) > - yield waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCE_SHOWN); > + yield Promise.race([ > + waitForDebuggerEvents(gPanel, gDebugger.EVENTS.SOURCE_SHOWN), > + waitForTime(4000), > + ]); as an example if the SOURCE_SHOWN event is not received in 4 seconds the test will go on and show the expected failures (and it should always cleans the environment for the next test, e.g. remove the addon if installed, close the debugger panel if it is opened and ensures that "finish" will be called)

(In reply to Alexandre Poirot [:ochameau] from comment #25) > Comment on attachment 8740441 [details] > MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions > ContentScript globals in the Tab DevTools debuggees. r=ochameau > > https://reviewboard.mozilla.org/r/45767/#review43151 > ::: devtools/server/actors/webbrowser.js:816 > (Diff revision 3) > > /** > > + * Getter for the WebExtensions ContentScript globals related to the > > + * current tab content's DOM window. > > + */ > > + get webextensionsContentScriptGlobals() { > > + if (this.window) { > > It might be helpful to say we ignore xpcshell here. I'm not sure about the exact content of the inline comment to put here (e.g. "This getter only retrieve the Sandboxes created by the WebExtensions internals for the Add-on ContentScripts, any Sandbox created by xpcshell is ignored."?)

(In reply to Luca Greco [:rpl] from comment #42) > (In reply to Alexandre Poirot [:ochameau] from comment #25) > > Comment on attachment 8740441 [details] > > MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions > > ContentScript globals in the Tab DevTools debuggees. r=ochameau > > > > https://reviewboard.mozilla.org/r/45767/#review43151 > > ::: devtools/server/actors/webbrowser.js:816 > > (Diff revision 3) > > > /** > > > + * Getter for the WebExtensions ContentScript globals related to the > > > + * current tab content's DOM window. > > > + */ > > > + get webextensionsContentScriptGlobals() { > > > + if (this.window) { > > > > It might be helpful to say we ignore xpcshell here. > > I'm not sure about the exact content of the inline comment to put here (e.g. > "This getter only retrieve the Sandboxes created by the WebExtensions > internals for the Add-on ContentScripts, any Sandbox created by xpcshell is > ignored."?) Keep it short. You are already describing your method in the function comment! Anything similar and as short as that: // Ignore xpcshell runtime which spawn TabActors without windows

Comment on attachment 8737808 [details] MozReview Request: Bug 1261857 - [webext] Support WebExtensions ContentScripts in the Tab DevTools Debugger. r=kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44125/diff/7-8/

Comment on attachment 8740441 [details] MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions ContentScript globals in the Tab DevTools debuggees. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45767/diff/5-6/

Comment on attachment 8740442 [details] MozReview Request: Bug 1261857 - [webext] add Devtools Debugger test case on WebExtensions ContentScript sources. r=ochameau Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45769/diff/6-7/

In the last push: - all patches rebased to a recent fx-team tip - added the additional inline comment as suggestedm - resolved potential conflicts related to changes in the list of the WebExtensions test files and test assets. (In reply to Luca Greco [:rpl] from comment #45) > Comment on attachment 8740441 [details] > MozReview Request: Bug 1261857 - [webext] Add existent WebExtensions > ContentScript globals in the Tab DevTools debuggees. r=ochameau > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/45767/diff/5-6/ added the inline comment as suggested in Comment 43 (In reply to Luca Greco [:rpl] from comment #44) > Comment on attachment 8737808 [details] > MozReview Request: Bug 1261857 - [webext] Support WebExtensions > ContentScripts in the Tab DevTools Debugger. r=kmag > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/44125/diff/7-8/ rebased on a recent fx-team to resolve potential conflicts related to changes in the list of webextensions test cases and test assets.

https://hg.mozilla.org/integration/fx-team/rev/95fc7c29de20 https://hg.mozilla.org/integration/fx-team/rev/ebb346387330 https://hg.mozilla.org/integration/fx-team/rev/a724f54757cb

https://hg.mozilla.org/mozilla-central/rev/95fc7c29de20 https://hg.mozilla.org/mozilla-central/rev/ebb346387330 https://hg.mozilla.org/mozilla-central/rev/a724f54757cb