STR: 0. Open Firefox and the browser toolbox (or browser content toolbox in e10s mode) 1. Open https://bugzilla.mozilla.org/attachment.cgi?id=8545930 2. Inspect the video ER: Be able to inspect the video controls AR: only <source> shows up as a kid of the <video>, nothing else.

We would need to get rid of the skip filter on the deepTreeWalker here https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#3394 > 0. Open Firefox and the browser toolbox (or browser content toolbox in e10s mode) Can you clarify what you mean by "browser content toolbox in e10s mode"? Are you wanting to see this content in the normal toolbox when in e10s? That would cause it to show up for all users once e10s is enabled by default.

And we can check if we are in the BT from the Inspector actor via: this.tabActor.isRootActor

(In reply to Brian Grinstead [:bgrins] from comment #1) > We would need to get rid of the skip filter on the deepTreeWalker here > https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/ > actors/inspector.js#3394 > > > 0. Open Firefox and the browser toolbox (or browser content toolbox in e10s mode) > > Can you clarify what you mean by "browser content toolbox in e10s mode"? > Are you wanting to see this content in the normal toolbox when in e10s? > That would cause it to show up for all users once e10s is enabled by default. Hm, interesting. I thought that the browser content toolbox (separate entry in the developer tools submenu under 'tools') was separate. And indeed it is, and it has frame scripts and stuff... but no inspector. Why not and can we use the inspector there to differentiate, or is that a silly idea? If so, I guess I'd want this in the content toolbox but behind a hidden pref.

(In reply to :Gijs Kruitbosch from comment #3) > (In reply to Brian Grinstead [:bgrins] from comment #1) > > We would need to get rid of the skip filter on the deepTreeWalker here > > https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/ > > actors/inspector.js#3394 > > > > > 0. Open Firefox and the browser toolbox (or browser content toolbox in e10s mode) > > > > Can you clarify what you mean by "browser content toolbox in e10s mode"? > > Are you wanting to see this content in the normal toolbox when in e10s? > > That would cause it to show up for all users once e10s is enabled by default. > > Hm, interesting. I thought that the browser content toolbox (separate entry > in the developer tools submenu under 'tools') was separate. And indeed it > is, and it has frame scripts and stuff... but no inspector. Why not and can > we use the inspector there to differentiate, or is that a silly idea? > > If so, I guess I'd want this in the content toolbox but behind a hidden pref. Sorry, I misunderstood what you meant - I don't think any change to the Browser Content Toolbox would have to be carried over to the content toolbox. I think it's only purpose is to debug frame scripts, so adding an inspector tab wouldn't make sense there. I think the reasonable path forward based on the description is to not skip native anonymous content in the Inspector Actor when this.tabActor.isRootActor is true, and then use the Browser Toolbox when needing to inspect this content.

What do you think? I also used this as a chance to do some refactoring around the DocumentWalker interface and instantiations (removed ability to call it without new since all of the non-test initializations are happening in WalkerActor.getDocumentWalker). Try is down, but checked markupview and toolkit/devtools/server/tests mochitests locally and all seemed good.

(In reply to Brian Grinstead [:bgrins] from comment #4) > (In reply to :Gijs Kruitbosch from comment #3) > > (In reply to Brian Grinstead [:bgrins] from comment #1) > > > We would need to get rid of the skip filter on the deepTreeWalker here > > > https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/ > > > actors/inspector.js#3394 > > > > > > > 0. Open Firefox and the browser toolbox (or browser content toolbox in e10s mode) > > > > > > Can you clarify what you mean by "browser content toolbox in e10s mode"? > > > Are you wanting to see this content in the normal toolbox when in e10s? > > > That would cause it to show up for all users once e10s is enabled by default. > > > > Hm, interesting. I thought that the browser content toolbox (separate entry > > in the developer tools submenu under 'tools') was separate. And indeed it > > is, and it has frame scripts and stuff... but no inspector. Why not and can > > we use the inspector there to differentiate, or is that a silly idea? > > > > If so, I guess I'd want this in the content toolbox but behind a hidden pref. > > Sorry, I misunderstood what you meant - I don't think any change to the > Browser Content Toolbox would have to be carried over to the content > toolbox. I think it's only purpose is to debug frame scripts, so adding an > inspector tab wouldn't make sense there. > > I think the reasonable path forward based on the description is to not skip > native anonymous content in the Inspector Actor when > this.tabActor.isRootActor is true, and then use the Browser Toolbox when > needing to inspect this content. That means one can't inspect it in e10s mode though...

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to Brian Grinstead [:bgrins] from comment #4) > > (In reply to :Gijs Kruitbosch from comment #3) > > > (In reply to Brian Grinstead [:bgrins] from comment #1) > > > > Can you clarify what you mean by "browser content toolbox in e10s mode"? > > > > Are you wanting to see this content in the normal toolbox when in e10s? > > > > That would cause it to show up for all users once e10s is enabled by default. > > > > > > Hm, interesting. I thought that the browser content toolbox (separate entry > > > in the developer tools submenu under 'tools') was separate. And indeed it > > > is, and it has frame scripts and stuff... but no inspector. Why not and can > > > we use the inspector there to differentiate, or is that a silly idea? > > > > > > If so, I guess I'd want this in the content toolbox but behind a hidden pref. > > > > Sorry, I misunderstood what you meant - I don't think any change to the > > Browser Content Toolbox would have to be carried over to the content > > toolbox. I think it's only purpose is to debug frame scripts, so adding an > > inspector tab wouldn't make sense there. > > > > I think the reasonable path forward based on the description is to not skip > > native anonymous content in the Inspector Actor when > > this.tabActor.isRootActor is true, and then use the Browser Toolbox when > > needing to inspect this content. > > That means one can't inspect it in e10s mode though... Interesting. Ryan, should we be able to use the Browser Toolbox Inspector to inspect page contents in e10s mode, or is that something that would have to be enabled in the Browser Content Toolbox?

(In reply to Brian Grinstead [:bgrins] from comment #7) > Interesting. Ryan, should we be able to use the Browser Toolbox Inspector > to inspect page contents in e10s mode, or is that something that would have > to be enabled in the Browser Content Toolbox? No, you can't reach into remote / e10s content frames via the regular Browser Toolbox. We could make the Browser Content Toolbox gain an Inspector, but the Browser Content Toolbox is really a big hack that we threw together for add-on / browser devs so they at least had *something*. I am hesitant to let it grow many new features when there is an alternate path. Can't we add this feature to the regular, in-tab toolbox, but default it off behind a toolbox option anyone can enable, similar to "Show browser styles"? That way it's more flexible overall, and should work in regular vs. e10s tabs as well.

(In reply to J. Ryan Stinnett [:jryans] from comment #8) > (In reply to Brian Grinstead [:bgrins] from comment #7) > > Interesting. Ryan, should we be able to use the Browser Toolbox Inspector > > to inspect page contents in e10s mode, or is that something that would have > > to be enabled in the Browser Content Toolbox? > > No, you can't reach into remote / e10s content frames via the regular > Browser Toolbox. > > We could make the Browser Content Toolbox gain an Inspector, but the Browser > Content Toolbox is really a big hack that we threw together for add-on / > browser devs so they at least had *something*. I am hesitant to let it grow > many new features when there is an alternate path. > > Can't we add this feature to the regular, in-tab toolbox, but default it off > behind a toolbox option anyone can enable, similar to "Show browser styles"? > That way it's more flexible overall, and should work in regular vs. e10s > tabs as well. Yeah, I guess that would be a better solution here. We will need to pass in the pref to the WalkerActor when the toolbox is opened, so changing the pref would require a toolbox restart. In the case of non-e10s BT the pref can be set on by default so it would work just like the currently attached patch does.

Hides this functionality behind "devtools.inspector.showAllAnonymousContent" pref instead of checking to see if we're in the Browser Toolbox

Comment on attachment 8546268 [details] [diff] [review] nativeanon-browser-toolbox.patch Review of attachment 8546268 [details] [diff] [review]: ----------------------------------------------------------------- Nice! This works well, and even shows the style of native anon content when the "browser styles" option is set. I just have a few comments around the code and comments of the document walker. r=me with these things fixed and a green try build. ::: browser/devtools/markupview/test/browser_markupview_anonymous_04.js @@ +24,5 @@ > + ok (!select.isAnonymous, "The select element is not anonymous"); > + > + let selectChildren = yield inspector.walker.children(select); > + > + is (selectChildren.nodes.length, 1, "<select> a single native child"); typo: <select> *has* a single native child. @@ +26,5 @@ > + let selectChildren = yield inspector.walker.children(select); > + > + is (selectChildren.nodes.length, 1, "<select> a single native child"); > + > + for (let node of selectChildren.nodes) { Maybe this test should be inspecting a <video controls> element instead, which has a far more complex inner markup, so as to exercise a bit more the walker' ability to walk through native anonymous content. ::: toolkit/devtools/server/actors/inspector.js @@ +3114,5 @@ > > return this._walkerPromise; > }, { > + request: { > + options: Arg(0, "nullable:json") Weird that the getWalker method always accepted an options argument but it was never defined as part of the request property. I wonder if this is going to cause a backward compat. problem when debugging older servers? Could you check if protocol.js will throw if an unexpected request is passed? @@ +3309,2 @@ > */ > +function DocumentWalker(node, rootWin, filter=standardNodeFilter, whatToShow) { Arguments with default values should always be last in the list, otherwise it's impossible to pass a value to whatToShow without passing one to filter. It looks like you need to define a default value for whatToShow too (Ci.nsIDOMNodeFilter.SHOW_ALL). By the way, why did you change the order of the 2 last arguments? @@ +3312,4 @@ > throw new Error("Got an invalid root window in DocumentWalker"); > } > > + whatToShow = whatToShow || Ci.nsIDOMNodeFilter.SHOW_ALL; If Ci.nsIDOMNodeFilter.SHOW_ALL is the default value of whatToShow in the function declaration, then this is unneeded. @@ +3316,5 @@ > this.walker = Cc["@mozilla.org/inspector/deep-tree-walker;1"].createInstance(Ci.inIDeepTreeWalker); > this.walker.showAnonymousContent = true; > this.walker.showSubDocuments = true; > this.walker.showDocumentsAsNodes = true; > + this.walker.init(rootWin.document, whatToShow || Ci.nsIDOMNodeFilter.SHOW_ALL); whatToShow has already been defined earlier, no need to check it again. @@ +3381,5 @@ > } > > +// A filter to be used in Browser Toolbox, since we don't care > +// about filtering out native anonymous content > +function whitespaceNodeFilter(aNode) { I think the standardNodeFilter function should come first in the file and then the whitespaceNodeFilter function. It's more logical this way. Both of these functions's jsdoc comments should be in the form of /**... rather than //... to be like other functions, and it should say in more details what each of these functions do filter in or out. Maybe something like: /** * This DeepTreeWalker filter skips whitespace text nodes and anonymous * content with the exception of ::before and ::after and anonymous content * in XUL document (needed to show all elements in the browser toolbox). */ function standardTreeWalkerFilter() /** * This DeepTreeWalker filter is like standardTreeWalkerFilter except that * it also includes all anonymous content (like internal form controls). */ function allAnonymousContentTreeWalkerFilter()

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11) > Comment on attachment 8546268 [details] [diff] [review] > nativeanon-browser-toolbox.patch > > Review of attachment 8546268 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice! This works well, and even shows the style of native anon content when > the "browser styles" option is set. > I just have a few comments around the code and comments of the document > walker. > r=me with these things fixed and a green try build. > Patch incoming with the following notes: > ::: browser/devtools/markupview/test/browser_markupview_anonymous_04.js > @@ +24,5 @@ > > + ok (!select.isAnonymous, "The select element is not anonymous"); > > + > > + let selectChildren = yield inspector.walker.children(select); > > + > > + is (selectChildren.nodes.length, 1, "<select> a single native child"); > > typo: <select> *has* a single native child. > > @@ +26,5 @@ > > + let selectChildren = yield inspector.walker.children(select); > > + > > + is (selectChildren.nodes.length, 1, "<select> a single native child"); > > + > > + for (let node of selectChildren.nodes) { > > Maybe this test should be inspecting a <video controls> element instead, > which has a far more complex inner markup, so as to exercise a bit more the > walker' ability to walk through native anonymous content. > Done > ::: toolkit/devtools/server/actors/inspector.js > @@ +3114,5 @@ > > > > return this._walkerPromise; > > }, { > > + request: { > > + options: Arg(0, "nullable:json") > > Weird that the getWalker method always accepted an options argument but it > was never defined as part of the request property. > I wonder if this is going to cause a backward compat. problem when debugging > older servers? Could you check if protocol.js will throw if an unexpected > request is passed? > Checked on 2.0 simulator and the markup view works fine (of course, it doesn't show the native anon content but there are no errors). > @@ +3309,2 @@ > > */ > > +function DocumentWalker(node, rootWin, filter=standardNodeFilter, whatToShow) { > > Arguments with default values should always be last in the list, otherwise > it's impossible to pass a value to whatToShow without passing one to filter. > It looks like you need to define a default value for whatToShow too > (Ci.nsIDOMNodeFilter.SHOW_ALL). > By the way, why did you change the order of the 2 last arguments? > > @@ +3312,4 @@ > > throw new Error("Got an invalid root window in DocumentWalker"); > > } > > > > + whatToShow = whatToShow || Ci.nsIDOMNodeFilter.SHOW_ALL; > > If Ci.nsIDOMNodeFilter.SHOW_ALL is the default value of whatToShow in the > function declaration, then this is unneeded. > > @@ +3316,5 @@ > > this.walker = Cc["@mozilla.org/inspector/deep-tree-walker;1"].createInstance(Ci.inIDeepTreeWalker); > > this.walker.showAnonymousContent = true; > > this.walker.showSubDocuments = true; > > this.walker.showDocumentsAsNodes = true; > > + this.walker.init(rootWin.document, whatToShow || Ci.nsIDOMNodeFilter.SHOW_ALL); > > whatToShow has already been defined earlier, no need to check it again. > Reverted the param ordering change and using default function values for both filter and whatToShow > @@ +3381,5 @@ > > } > > > > +// A filter to be used in Browser Toolbox, since we don't care > > +// about filtering out native anonymous content > > +function whitespaceNodeFilter(aNode) { > > I think the standardNodeFilter function should come first in the file and > then the whitespaceNodeFilter function. It's more logical this way. > > Both of these functions's jsdoc comments should be in the form of /**... > rather than //... to be like other functions, and it should say in more > details what each of these functions do filter in or out. > > Maybe something like: > > /** > * This DeepTreeWalker filter skips whitespace text nodes and anonymous > * content with the exception of ::before and ::after and anonymous content > * in XUL document (needed to show all elements in the browser toolbox). > */ > function standardTreeWalkerFilter() > > /** > * This DeepTreeWalker filter is like standardTreeWalkerFilter except that > * it also includes all anonymous content (like internal form controls). > */ > function allAnonymousContentTreeWalkerFilter() Done

Updated patch. Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8a7f2d4c4a52