If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add support for inspecting XBL anonymous content

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Developer Tools: Inspector
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: smaug, Assigned: krizsa)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

unspecified
Firefox 35
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 28 obsolete attachments)

150.54 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
8.87 KB, patch
Details | Diff | Splinter Review
26.99 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 646080 [details]
inspecting browser.xul screenshot

Would be nice to be able to see also the anonymous content in the inspector
(perhaps in some "chrome" mode).
That would let one to use inspector also for chrome, at least in certain cases.

Comment 1

5 years ago
triage. Filter on PINKISBEAUTIFUL
Component: Developer Tools → Developer Tools: Inspector
Duplicate of this bug: 842643

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs

Comment 3

5 years ago
Created attachment 737358 [details] [diff] [review]
WIP Patch

So here's something that vaguely works, sharing = caring, etc.

Known issues:
- highlighting chrome nodes when they are selected doesn't work correctly (positions are offset on the content window) because the highlighting code always uses the content window (I suspect this already is a problem even without using a different treewalker, so I'd argue it's a different bug)
- I haven't actually tested this on documents with a high number of childnodes, so my reimplementation of _getVisibleChildren could be buggy.
- variation on the previous: no tests yet. Sorry; that ought to be fixed sometime soon, though.
- editing nodes works in the sense that the nodes are updated, but doesn't in the sense that the UI is not. This is because it's currently using mutation observers to detect changes. I'm hesitant to reimplement this; ideally there'd be a way to use mutation observers inside XBL... but that sounds scary, too. Ideas?
- (possibly obvious given the previous: you won't see changes other code is making to the XBL while you're looking at it in the inspector, either)
- no UI for the hidden pref I added (devtools.inspector.showAnonContent). Ideas?

hg blame claims most of the inspector is the work of dcamp, so asking for some feedback! :-)
Attachment #737358 - Flags: feedback?(dcamp)

Comment 4

5 years ago
Oh, and I forgot: this patch could have been a lot smaller if it wasn't for the fact that inDeepTreeWalker is... funny... about throw-away dom traversal. That is, AIUI:

1) setting currentNode is not implemented
2) it uses stacks to keep track of the node tree, and it will simply return null if you try to get a parentNode that it hasn't seen before.

Therefore, trying to initialize with a random node and not having problems "later" is expensive (as it would involve finding the "root", and then walking until you find your desired currentnode).

Comment 5

5 years ago
(In reply to :Gijs Kruitbosch from comment #3)
> Created attachment 737358 [details] [diff] [review]
> WIP Patch
> 
> So here's something that vaguely works, sharing = caring, etc.
> 
> Known issues:
> - I haven't actually tested this on documents with a high number of
> childnodes, so my reimplementation of _getVisibleChildren could be buggy.
> - variation on the previous: no tests yet. Sorry; that ought to be fixed
> sometime soon, though.

https://tbpl.mozilla.org/?tree=Try&rev=8d87711f1fd7 says this broke a single browser chrome test, so I'll have to look at that, at least.

Comment 6

5 years ago
Created attachment 737759 [details] [diff] [review]
WIP patch v2

This is a little better; but I realized the style views were broken for anon content. This somewhat fixes the Computed and Layout panes; the Rules pane is still completely horked and I couldn't quickly figure out why. I've also fixed the failing test and added some more.
Attachment #737358 - Attachment is obsolete: true
Attachment #737358 - Flags: feedback?(dcamp)
Attachment #737759 - Flags: feedback?(dcamp)

Comment 7

5 years ago
Created attachment 738675 [details] [diff] [review]
WIP patch v3

This now has working style views, which is more or less what I need for my usage. I can probably get the editing stuff and/or UI working on the flight home this weekend if someone lets me know what the "preferred" way of dealing with it would be... As far as I know, that and tests are the only remaining issues (fixing everything related to browser chrome inspection is definitely a separate bug) :-)
Attachment #737759 - Attachment is obsolete: true
Attachment #737759 - Flags: feedback?(dcamp)
Attachment #738675 - Flags: feedback?(dcamp)

Comment 8

4 years ago
Comment on attachment 738675 [details] [diff] [review]
WIP patch v3

Clearing the feedback request, we've talked about this quite a bit on irc.
Attachment #738675 - Flags: feedback?(dcamp)

Updated

4 years ago
Duplicate of this bug: 844753

Comment 10

4 years ago
As discussed on irc, we're going to hold this up for the XBL2 refactoring.
Depends on: 653881
Blocks: 876636
Whiteboard: [see comment 10]

Updated

4 years ago
Blocks: 945450

Updated

4 years ago
Blocks: 859886

Comment 11

4 years ago
So meanwhile, in web land, bug 887538 landed. It bitrotted some WIP that I had that wasn't on here yet, but also, it means there's now not just XBL anon content, but also shadow anon content. Wheee...

Comment 12

4 years ago
Cc'ing gabor with where I think this bug stands:

The inspector currently uses the TreeWalker interface to answer a few questions:

* Parent of a given node
* First/Last child of a given node
* Previous/Next child of a given node.

We use the TreeWalker filter to filter out empty text nodes (which we should probably do optionally, but that's a different bug).

What would be nice (and what Gijs' WIP starts on) is a version of the TreeWalker interface that includes anonymous content.

Comment 13

4 years ago
Created attachment 8356546 [details] [diff] [review]
adjust FlattenedChildIterator, use in inIDeepTreeWalker implementation

So this is my current WIP to make inIDeepTreeWalker be better for our purposes. It works, ish, but it fails a lot of the only test we have for this component, which is layout/inspector/tests/test_bug522601.xhtml. I haven't had time to figure out why, but I think it's because we need a way to 'turn off' entering into anonymous content, and ChildIterator doesn't provide one. I won't be able to look at this again until the coming weekend, so if someone wants to pick this up before then, please feel free.
(Assignee)

Comment 14

4 years ago
I'm going to take a look into this tomorrow and see how much I grasp and where can I help...
(Assignee)

Comment 15

4 years ago
I'm not yet sure what I should do here, so here are a couple of thoughts as I read through the comments and patches...

> Oh, and I forgot: this patch could have been a lot smaller if 

> 2) it uses stacks to keep track of the node tree, and it will simply return
> null if you try to get a parentNode that it hasn't seen before.
> 
> Therefore, trying to initialize with a random node and not having problems
> "later" is expensive (as it would involve finding the "root", and then
> walking until you find your desired currentnode).

What is the importance of the NextNode / PreviousNode for us? Wouldn't be a lot simpler just changing parentNode to check for the node's parent and use that if the stack is empty (and the treewalker was inited with a non-root node) instead of this expensive approach?

(In reply to Dave Camp (:dcamp) from comment #12)
> What would be nice (and what Gijs' WIP starts on) is a version of the
> TreeWalker interface that includes anonymous content.

So do we want to alter the existing TreeWalker or implement a new one that suits better for our needs? What about DOM modifications? that part seems like the hardest problem here... Is the shadow content part solved and we have to focus only on XBL stuff, or shadow contents still should be taken care of as well?

Comment 16

4 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> I'm not yet sure what I should do here, so here are a couple of thoughts as
> I read through the comments and patches...
> 
> > Oh, and I forgot: this patch could have been a lot smaller if 
> 
> > 2) it uses stacks to keep track of the node tree, and it will simply return
> > null if you try to get a parentNode that it hasn't seen before.
> > 
> > Therefore, trying to initialize with a random node and not having problems
> > "later" is expensive (as it would involve finding the "root", and then
> > walking until you find your desired currentnode).
> 
> What is the importance of the NextNode / PreviousNode for us? Wouldn't be a
> lot simpler just changing parentNode to check for the node's parent and use
> that if the stack is empty (and the treewalker was inited with a non-root
> node) instead of this expensive approach?

It's not just about these; parentNode() fails to work above the stack/root item, too. The problem with avoiding this pattern is that a very deep tree/stack in this case is bad memory-complexity-wise, as the current implementation keeps a local list of all the children, and all the children of its parent node, and all the children of its parent node, etc., all the way up to the root. If you open pages with wide trees (ie nodes with very many children), the devtools inspector omits the middle bits. In order to do something like that it'd be nice if we only needed to walk the relevant bits of the tree, not the entire thing...

IIRC there are tests that check the "don't go past the root" behaviour of the walker, so I am somewhat loathe to change that. On the other hand, I imagine the number of consumers is very limited (DOMI and... ?) so perhaps we can just make the change and deal with it. The local copy of the child list memory issue would remain, however...

The last WIP I put up is an attempt to do a new implementation of inIDeepTreeWalker that doesn't have the latter problem, but it suffers from having to 'look up' the position of a node in the children of its parent so it can answer suitably to previousSibling/nextSibling queries (which the current treewalker code in devtools does also use). See the mIterator->Seek() calls in that patch.

> (In reply to Dave Camp (:dcamp) from comment #12)
> > What would be nice (and what Gijs' WIP starts on) is a version of the
> > TreeWalker interface that includes anonymous content.
> 
> So do we want to alter the existing TreeWalker or implement a new one that
> suits better for our needs?

Yes.

> What about DOM modifications? that part seems
> like the hardest problem here... 

I don't understand what you mean here. As long as you have a reference to a DOM Node, you can modify it (I'd imagine you could do that even to the native anon content that gets inserted...!). We'd need to mark ::before and ::after pseudoframes as unmodifiable in the UI, I guess, but otherwise I don't think there'd be a problem.

> Is the shadow content part solved and we
> have to focus only on XBL stuff, or shadow contents still should be taken
> care of as well?

ChildIterator knows about both HTML5 shadow content and XBL content. Offhand, I don't actually recall if it knows about native anon content. Blake?

The GetChildren() call that the current inDeepTreeWalker code uses defers to that iterator, so it also works.
Flags: needinfo?(mrbkap)

Comment 17

4 years ago
(In reply to :Gijs Kruitbosch from comment #16)
> The problem with avoiding this pattern is that a very deep
> tree/stack in this case is bad memory-complexity-wise,

s/memory-//

> as the current
> implementation keeps a local list of all the children, and all the children
> of its parent node, and all the children of its parent node, etc., all the
> way up to the root. If you open pages with wide trees (ie nodes with very
> many children), the devtools inspector omits the middle bits. In order to do
> something like that it'd be nice if we only needed to walk the relevant bits
> of the tree, not the entire thing...
(In reply to :Gijs Kruitbosch from comment #16)
> ChildIterator knows about both HTML5 shadow content and XBL content.
> Offhand, I don't actually recall if it knows about native anon content.
> Blake?

It must know about native anonymous content, since if it didn't we wouldn't render NAC.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 19

4 years ago
Ok, I've been looking into this for a while, but still don't know what to do. Probably because I still know very little about XBL. What I gathered that this is a very important feature for devtools, once shadow content is out, and even before that for inspecting XBL dependent pages.

The problem is with the original inDeepTreeWalker, that it cannot handle being started from a non-root node. The WIP patch is trying, as an alternative solution, using FlattenedChildIterator to implement inDeepTreeWalker (creating a new FlattenedChildIterator each time it goes one level 'down' or 'up' in the tree). Problem with that approach that I'm afraid if it's started from an XBL content, getParent will find the parent in the XBL tree instead of the insertion point in the original one, which would probably be the right thing to do. So it feels like a dead end to me.

I'm also a bit hesitant to implement yet another tree traversal class, I would prefer extending/altering inDeepTreeWalker. So preserving current functionality just some way to set the current node. Boris helped me understanding quite a lot of things in this area, but still could not process all of it. His idea was to:
<bz> I'd love a treewalker impl that walks the flattened tree
<bz> You may want to talk to wchen/mrbkap about how to do it sanely
...
<bz> Really, we should add nsINode flattened tree traversal APIs or something....
<bz> And then this might get simpler

Which I realized I don't fully understand. I couldn't find a definition for parent in the flattened view, nor do I know what "nsINode flattened tree traversal" is (probably I should...). Anyway, I flag some people for need info who I think might be able to give me some help how to start with this, or want to form an opinion / be in the loop.

Finally mutation events for XBL... I don't even want to go there yet.
Flags: needinfo?(wchen)
Flags: needinfo?(mrbkap)
Flags: needinfo?(bzbarsky)
> I couldn't find a definition for parent in the flattened view

I guess Shadow DOM calls it a "composed tree" now.  See http://w3c.github.io/webcomponents/spec/shadow/#composed-trees and you may want to read all of http://w3c.github.io/webcomponents/spec/shadow/#concepts in general.

> nor do I know what "nsINode flattened tree traversal" is

The idea would be to just expose methods on nsINode for traversing the "composed tree" from that node.  As a start, getting its parent in the composed tree and getting its children (this last is handled by FlattenedChildIterator).
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 21

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #20)
> I guess Shadow DOM calls it a "composed tree" now.  See
> http://w3c.github.io/webcomponents/spec/shadow/#composed-trees and you may
> want to read all of http://w3c.github.io/webcomponents/spec/shadow/#concepts
> in general.

Thanks, I also read some XBL docs.

> The idea would be to just expose methods on nsINode for traversing the
> "composed tree" from that node.  As a start, getting its parent in the
> composed tree and getting its children (this last is handled by
> FlattenedChildIterator).

Yes, exactly, getting the parent in the composed tree what we're missing here.
For children FlattenedChildIterator seems perfect, but the parent part can get
very tricky. I don't think I want to reverse that algorithm... Instead, how about
we cache the "composed tree parent" of each nodes when we build up the composed tree?
(if it's != the nodes original parent) Where do we build up the composed tree? (if we
do that at all, but I would _guess_ we do that for presentation at least, no?)
(Assignee)

Comment 22

4 years ago
Gijs just pointed it out on IRC that we have inDOMUtils::GetParentForNode, which at first glance might do exactly this for XBL? Does that also handle shadow trees? If not, would it make sense to extend it?

Comment 23

4 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22)
> Gijs just pointed it out on IRC that we have inDOMUtils::GetParentForNode,
> which at first glance might do exactly this for XBL? Does that also handle
> shadow trees? If not, would it make sense to extend it?

I believe an extension would make sense if it doesn't already work (shouldn't be hard to test).

I don't have cycles in the nearby future to work more on this, so I'm going to assign this one to you. :-)
Assignee: gijskruitbosch+bugs → gkrizsanits
(Assignee)

Comment 24

4 years ago
I actually started to implement a version based on FlattendChildIterator and GetParentForNode, but after having a talk with Blake about it I will probably use nsIContent::GetChildren instead.

Updated

4 years ago
Flags: needinfo?(mrbkap)
Summary: Add support for XBL anonymous content → Add support for inspecting XBL anonymous content
(Assignee)

Comment 25

4 years ago
Ok, this approach seems to work. I've reimplemented the whole class without stack with SetCurrentNode and tests are green. I need to clean up the code and add some tests.
(Assignee)

Comment 26

4 years ago
Created attachment 8369659 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker

I want to write some new tests, and still need to sort out what happens when someone changes the show anonymous content flag during the walk. But other than that, does this look sane to you so far?
Attachment #8369659 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 27

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d0aba9426cc5
Comment on attachment 8369659 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker

> mDOMUtils->GetParentForNode

This will walk right out of Document nodes to the parent iframe.  Is that the behavior we want here?  Or is that never an issue due to mRoot?

>+  // If we have not reached the root, and there we have not found a parent yet,
>+  // we need to progress to the parent document.

Then why not call mDOMUtils->GetParentForNode for both values of mShowAnonymousContent?

Need to think through the rest of this carefully, so not tonight.  :(  Sorry for the lag....
I guess one thing I don't quite understand is what the point is of mSiblings.  Is that an optimization attempt, basically?
(Assignee)

Comment 30

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8369659 [details] [diff] [review]
> setCurrentNode for inDeepTreeWalker
> 
> > mDOMUtils->GetParentForNode
> 
> This will walk right out of Document nodes to the parent iframe.  Is that
> the behavior we want here?  Or is that never an issue due to mRoot?

I think so yes. mRoot should protect us. However... I might want to add an mShowSubDocument flag check here for the case where we set the node above mRoot (or the current node was moved in the DOM above mRoot), but that case is hard to define right anyway...

> 
> >+  // If we have not reached the root, and there we have not found a parent yet,
> >+  // we need to progress to the parent document.
> 
> Then why not call mDOMUtils->GetParentForNode for both values of
> mShowAnonymousContent?

I think I did this wrong. I was not sure that GetParentForNode navigates up to the
parent document, that's why the overcomplications here... if the
inLayoutUtils::GetContainerFor(*doc); bits in GetParentForNode does exactly that,
then this part should be simplified to one GetParentForNode call in all cases with
mShowAnonymousContent, and I will sleep a lot better. 

> 
> Need to think through the rest of this carefully, so not tonight.  :(  Sorry
> for the lag....

Sure. Thanks a lot for spending time on this one.

(In reply to Boris Zbarsky [:bz] from comment #29)
> I guess one thing I don't quite understand is what the point is of
> mSiblings.  Is that an optimization attempt, basically?

Attempt :) yes exactly... I know it's not much but I think it worth it.
I feel bad about creating a FlattenedChildIterator and populating a new
list for each step. This is far from perfect since stepping up on the
tree is expensive, maybe I should do this part more lazily or
come up with a better optimization strategy. But at least for siblings
and children it's not terrible...
(Assignee)

Comment 31

4 years ago
Clearing some outdated flags here. I plan to come back to this problem next week in the hope of finishing it. Currently in the middle of integrating it into the inspector, it's sort of working but there are some glitches with the highlighter... Need to read some more code and fiddle with it some more...
Flags: needinfo?(wchen)
(Assignee)

Comment 32

4 years ago
Comment on attachment 8369659 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker

Review of attachment 8369659 [details] [diff] [review]:
-----------------------------------------------------------------

I've already got the feedback on irc and here I was looking for.
Attachment #8369659 - Flags: feedback?(bzbarsky)
Very exciting; thanks for the work and the update!  Looking forward to seeing this move forward.
(Assignee)

Comment 34

4 years ago
Dave Camp offered me some help with the integration. So I upload some WIP patches for him.

Currently with Shadow content there is an issue, I needed to turn off an assertion in the CSS rule processor. I talked to William Chen about this he has not got the time to look into the problem yet. But without this assertion it seemed to work on my basic tests for now.

With XBL it's a bit more tricky. I can inspect the browser window, but there were some subtle issues. If I launch the browser toolbox while the nightly start page is visible on the main window, inspecting the browser frame works fine, but inspecting the content page does not. But if instead of hovering with the selector one clicks on content, bread crumbs does find the whole parent chain and works fine. And if then you click on the bread crumb button of the parent element, everything starts to work fine...

I tried many things to fix this issue, but failed with it while spending way too much time...

About the integration part there are a few important things. First, there is no filter support in the inDeepTreeWalker implementation. I could implement it, but if we need to filter out text nodes really, maybe we should consider doing it in JS. The re-parenting is only used for SVG documents right now. I init the walker with the main window as the root.

An alternative approach for this whole thing would be to just use the nsINode interface instead of the walker, and exposing only a method that can find parent in the flattened tree and another one that can list the children. But the current walker has the next/prev functionality which can be nice...
(Assignee)

Comment 35

4 years ago
Created attachment 8392122 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v2
Attachment #8369659 - Attachment is obsolete: true
(Assignee)

Comment 36

4 years ago
Created attachment 8392124 [details] [diff] [review]
turning off assertion

This patch is clearly not the way to fix this problem. Only for testing purposes and to speed up the integration process.
(Assignee)

Comment 37

4 years ago
Created attachment 8392127 [details] [diff] [review]
integration v1

This last 3 patches needed for start.
> I needed to turn off an assertion in the CSS rule processor

What was the stack to that assertion triggering?
(In reply to Boris Zbarsky [:bz] from comment #38)
> > I needed to turn off an assertion in the CSS rule processor
> 
> What was the stack to that assertion triggering?

The check in AncestorFilter::AssertHasAllAncestors was asserting. From the stack traces that Gabor sent me, it looked like it was caused by <shadow> insertion points. Similar to web component's <content> and xbl's <children>, it doesn't get a frame. One way to fix this could be to do what we do for the other insertion points and explicitly put them into the ancestor filter. Although I'm not sure that would make sense in this case because you can't style children of <shadow> via the <shadow> element. Children of <shadow> are always distributed somewhere or ignored, it doesn't have the fallback content behavior of <content>.

Comment 40

4 years ago
Created attachment 8407759 [details] [diff] [review]
devtools-integration-part1

This new version works a bit better (it at least loads up pages) but when I descend into a node with a shadow root I trigger an assertion:

[66757] ###!!! ASSERTION: aFrame must be first continuation: '!aFrame->GetPrevContinuation()', file /Users/dcamp/moz/fx/mozilla/layout/base/nsLayoutUtils.cpp, line 1028
Assertion failure: !HasSlots() (nsNodeUtils::LastRelease was not called?), at /Users/dcamp/moz/fx/mozilla/content/base/src/nsINode.cpp:142
Attachment #8392127 - Attachment is obsolete: true
Attachment #8407759 - Flags: feedback?(gkrizsanits)
(Assignee)

Comment 41

4 years ago
Hmm... this is fun. I didn't have this problem the last time, maybe something changed in the ShadowRoot since than. So we have this stack:
(gdb) backtrace
#0  0x00007ffff1e72c0e in JSContext::runtime (this=0x1)
    at /home/gabor/development/mozilla-central/js/src/jscntxt.h:408
#1  0x00007ffff229a9a7 in AssertHeapIsIdleOrIterating (cx=0x1)
    at /home/gabor/development/mozilla-central/js/src/jsapi.cpp:181
#2  0x00007ffff22ab0e3 in JS::CurrentGlobalOrNull (cx=0x1)
    at /home/gabor/development/mozilla-central/js/src/jsapi.cpp:1410
#3  0x00007fffef2a9753 in mozilla::dom::WrapNativeISupportsParent<nsISupports>
    (cx=0x1, p=0xdb2a20, cache=0x0)
    at ../../dist/include/mozilla/dom/BindingUtils.h:1272
#4  0x00007fffef2b388f in mozilla::dom::WrapNativeParentHelper<nsISupports, false>::Wrap (cx=0x1, parent=0xdb2a20, cache=0x0)
    at ../../dist/include/mozilla/dom/BindingUtils.h:1342
#5  0x00007fffef81b1bb in mozilla::dom::WrapNativeParent<nsISupports> (cx=0x1, 
    p=0xdb2a20, cache=0x0, useXBLScope=false)
    at ../../dist/include/mozilla/dom/BindingUtils.h:1356
#6  0x00007fffef81aacb in mozilla::dom::WrapNativeParent<nsISupports*> (cx=
    0x1, p=@0x7ffffffea540: 0xdb2a20)
    at ../../dist/include/mozilla/dom/BindingUtils.h:1382
#7  0x00007fffef803423 in mozilla::dom::ShadowRootBinding::Wrap (aCx=0x1, 
    aObject=0x2940cc0, aCache=0x2940cc8)
    at /home/gabor/development/mozilla-central/obj-x86_64-unknown-linux-gnu/dom/bindings/ShadowRootBinding.cpp:494
#8  0x00007ffff0191ed2 in mozilla::dom::ShadowRootBinding::Wrap<mozilla::dom::Sh---Type <return> to continue, or q <return> to quit---
adowRoot> (aCx=0x1, aObject=0x2940cc0)
    at ../../../dist/include/mozilla/dom/ShadowRootBinding.h:52
#9  0x00007ffff0174d05 in mozilla::dom::ShadowRoot::WrapObject (this=
    0x2940cc0, aCx=0x1)
    at /home/gabor/development/mozilla-central/content/base/src/ShadowRoot.cpp:102
#10 0x00007fffefaa6d93 in nsIFrame::GetFirstChild (this=0x2940cc0, aListID=
    mozilla::layout::kPrincipalList)
    at /home/gabor/development/mozilla-central/layout/generic/nsIFrame.h:1091
#11 0x00007fffefaa6dbb in nsIFrame::GetFirstPrincipalChild (this=0x2940cc0)
    at /home/gabor/development/mozilla-central/layout/generic/nsIFrame.h:1098
#12 0x00007ffff098f1af in GetFirstChildFrame (aFrame=0x2940cc0, aContent=
    0xcdcdcdcd00000802)
    at /home/gabor/development/mozilla-central/layout/base/nsLayoutUtils.cpp:872
#13 0x00007ffff098f842 in nsLayoutUtils::GetBeforeFrame (aFrame=0x2940cc0)
    at /home/gabor/development/mozilla-central/layout/base/nsLayoutUtils.cpp:1030
#14 0x00007ffff015cd46 in mozilla::dom::FragmentOrElement::GetChildren (this=
    0x2940cc0, aFilter=0)
    at /home/gabor/development/mozilla-central/content/base/src/FragmentOrElement.cpp:646
#15 0x00007ffff0c073f2 in inDeepTreeWalker::SetCurrentNode (this=0x15757a0, 
---Type <return> to continue, or q <return> to quit---
    aCurrentNode=0x26b8cd0, aSiblings=0x0)
    at /home/gabor/development/mozilla-central/layout/inspector/inDeepTreeWalker.cpp:159
#16 0x00007ffff0c072b2 in inDeepTreeWalker::SetCurrentNode (this=0x15757a0, 
    aCurrentNode=0x26b8cd0)
    at /home/gabor/development/mozilla-central/layout/inspector/inDeepTreeWalker.cpp:138
#17 0x00007ffff0c07617 in inDeepTreeWalker::ParentNode (this=0x15757a0, 
    _retval=0x7ffffffeaa38)
    at /home/gabor/development/mozilla-central/layout/inspector/inDeepTreeWalker.cpp:193
#18 0x00007fffee461091 in NS_InvokeByIndex (that=0x15757a0, methodIndex=8, 
    paramCount=1, params=0x7ffffffeaa38)
    at /home/gabor/development/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164
#19 0x00007fffefb97ba3 in CallMethodHelper::Invoke (this=0x7ffffffea9f0)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2392
#20 0x00007fffefb958da in CallMethodHelper::Call (this=0x7ffffffea9f0)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1733
#21 0x00007fffefb7c2ac in XPCWrappedNative::CallMethod (ccx=..., mode=
    XPCWrappedNative::CALL_METHOD)


The jump from #10 to #9 makes no sense. Unless because of a bad casting... And as it turns out in FragmentOrElement::GetChildren the GetPrimaryFrame() call on a ShadowRoot returns itself... Which makes no sense since it should at least return an nsIFrame... Then ofc when we start calling nsIFrame functions on a ShadowRoot bad things start to happen...

So the question is who calls SetPrimaryFrame with a ShadowRoot* ? William does any of this make sense / ring any bells for you? Or should I dig further to see if there are some deeper memory corruption somewhere?
Flags: needinfo?(wchen)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #41)
> So the question is who calls SetPrimaryFrame with a ShadowRoot* ? William
> does any of this make sense / ring any bells for you? Or should I dig
> further to see if there are some deeper memory corruption somewhere?
This is a symptom of bug 992521 related to ShadowRoot lying about being in document.

http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsIContent.h#879

mPrimaryFrame happens to be in a union type that stores the frame for nodes in document and the subtree root otherwise.
Flags: needinfo?(wchen)
(Assignee)

Comment 43

4 years ago
> This is a symptom of bug 992521 related to ShadowRoot lying about being in
> document.

Yupp this seems like it.
Depends on: 992521
No longer depends on: 653881
Blocks: 920141
(Assignee)

Comment 44

3 years ago
Created attachment 8420831 [details] [diff] [review]
shadowroot should not be in doc

A quick workaround for the problem until Bug 992521 will be resolved to unblock Dave.
(Assignee)

Comment 45

3 years ago
Comment on attachment 8407759 [details] [diff] [review]
devtools-integration-part1

Review of attachment 8407759 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the feedback flag for now as I think it was about the crash.
Attachment #8407759 - Flags: feedback?(gkrizsanits)
Attachment #8407759 - Attachment description: integration v2 → devtools-integration-part1
Created attachment 8422421 [details] [diff] [review]
devtools-integration-part2.patch

A patch for devtools integration I've been working on for before and after elements in Bug 920141.  This can be applied in order to also inspect anonymous content so hopefully it can help.  It should be applied after the following patches:

Attachment 8392122 [details] [diff]: setCurrentNode for inDeepTreeWalker. v2
Attachment 8392124 [details] [diff]: turning off assertion
Attachment 8420831 [details] [diff]: shadowroot should not be in doc
Attachment 8407759 [details] [diff]: devtools-integration-part1

Some changes:
* Adds a whitespace filter in JS as mentioned in Comment 34.
* Shows anonymous content in markup view (for instance, inspecting buttons on chrome://browser/content/devtools/scratchpad.xul)
* Adds support for markup view / rule view / computed styles / font inspector / box model tab for anon content and before/after.

A few things that are still issues that I'm still looking into:
* Markup view doesn't traverse iframes.  I'm pretty sure this a frontend problem only, since I can still inspect nodes inside of frames, and the proper CSS rules show up in the rule view, for instance.
* I always see "some nodes were hidden" button at the top of the markup view, and pressing it doesn't do anything.
* When inspecting pseudo element content like on http://fiddle.jshell.net/bgrins/PRQ85/show/, I see the following assertion: 
"ASSERTION: we should be in a pseudo-element that is expected to contain elements (:before): 'nsCSSPseudoElements::PseudoElementContainsElements(pseudo)', file /layout/style/nsComputedDOMStyle.cpp, line 675"
Attachment #738675 - Attachment is obsolete: true
Created attachment 8422423 [details]
inspecting-scratchpad-xul-screenshot.png
Attachment #646080 - Attachment is obsolete: true
Found a workaround for the lack of iframe inspection, so this walker is working well and starting to get pretty close for the front-end.  A few issues I've noticed with the latest deepTreeWalker patch applied:

1) The contentDocument is not being returned as the firstChild of an iframe (there is an easy workaround in js)
2) The doctype node seems to be ignored - previousSibling is failing to return the doctype node as sibling for the html element
3) The shadowRoot document fragment is not being returned as a child of the host.  I'm not sure if this is the expected behavior, but it would be helpful for displaying the shadow content under a document fragment.  This should be able to be worked around in js, but is a bit trickier than iframes since I think the shadow content is as children to the host.  Ideally we will be able to represent the normal content and the shadow content differently.

Comment 49

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #48)
> 2) The doctype node seems to be ignored - previousSibling is failing to
> return the doctype node as sibling for the html element

This is odd; in normal DOM (like this webpage's DOM) document.documentElement.previousSibling == document.doctype

This should be workaround-able in JS, too, in any case. Are you perhaps filtering for element type nodes? The doctype has its own nodeType (10, DOCUMENT_TYPE_NODE).
(In reply to :Gijs Kruitbosch from comment #49)
> (In reply to Brian Grinstead [:bgrins] from comment #48)
> > 2) The doctype node seems to be ignored - previousSibling is failing to
> > return the doctype node as sibling for the html element
> 
> This is odd; in normal DOM (like this webpage's DOM)
> document.documentElement.previousSibling == document.doctype
> 
> This should be workaround-able in JS, too, in any case. Are you perhaps
> filtering for element type nodes? The doctype has its own nodeType (10,
> DOCUMENT_TYPE_NODE).

Ah, looks like whatToShow is ignored by inDeepTreeWalker thttp://dxr.mozilla.org/mozilla-central/source/layout/inspector/inDeepTreeWalker.cpp?#24.

We are passing in nsIDOMNodeFilter.SHOW_ALL in all cases except for the breadcrumbs, where we pass Ci.nsIDOMNodeFilter.SHOW_ELEMENT.  It should be easy to add filtering for extra nodes in JS, but if nodes are not being are not being returned we will have to work around on a case-by-case basis as you say.
(Assignee)

Comment 51

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #46)
> 
> 1) The contentDocument is not being returned as the firstChild of an iframe
> (there is an easy workaround in js)
> 2) The doctype node seems to be ignored - previousSibling is failing to
> return the doctype node as sibling for the html element
> 3) The shadowRoot document fragment is not being returned as a child of the
> host.  I'm not sure if this is the expected behavior, but it would be
> helpful for displaying the shadow content under a document fragment.  This
> should be able to be worked around in js, but is a bit trickier than iframes
> since I think the shadow content is as children to the host.  Ideally we
> will be able to represent the normal content and the shadow content
> differently.

I fixed 1) and 2) but 3) is a bit more tricky. I think what we see now is in sync with
the old spec, but I checked the current version of the spec and of course it has
completely changed. William, what is your opinion about changing the shadow iterator
part here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/ChildIterator.cpp#116 ?
Instead of getting the oldest shadow root and iterate its children, I think we should first iterate all
the shadow roots since now those are ending up in the flattened tree by spec... (http://w3c.github.io/webcomponents/spec/shadow/#example-tree-of-trees)
Flags: needinfo?(wchen)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #51)
> William, what is your opinion about changing the shadow
> iterator
> part here:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/ChildIterator.
> cpp#116 ?
> Instead of getting the oldest shadow root and iterate its children, I think
> we should first iterate all
> the shadow roots since now those are ending up in the flattened tree by
> spec...
> (http://w3c.github.io/webcomponents/spec/shadow/#example-tree-of-trees)
Shadow roots don't end up as the child of the host in the flattened tree (or as the spec calls it, the composed tree) http://w3c.github.io/webcomponents/spec/shadow/#distribution-results. Also, only the children of the youngest shadow roots appear as children of the host, children of the older shadow root only appear in the composed tree if there is a <shadow> element in the younger shadow tree, in which case, children of the older shadow root appear in place of the <shadow> element in the younger shadow tree.

We can't change the ChildIterator because it needs to match the composed tree, we use it to create the frame tree.
Flags: needinfo?(wchen)
(Assignee)

Comment 53

3 years ago
(In reply to William Chen [:wchen] from comment #52)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #51)
> Shadow roots don't end up as the child of the host in the flattened tree (or
> as the spec calls it, the composed tree)

Indeed, you are right. So the question is than what should the inspector show. We have two options, showing the tree of trees somehow, in which case each shadow host might have more than one shadow root as children, etc. Or showing the composed/flattened tree, which is pretty much what can be seen on the screen as the end result (as it represents the frame tree). The current version is the implementation of the 2nd option, and I would vote for this version. Simply because the other one is a lot easier to imagine from the source, the composed/flattened tree is the live one, that users actually see on the screen in runtime. If we want to have the first option instead, that might be very difficult to achieve (need some research to tell).
(Assignee)

Comment 54

3 years ago
I think there are two approaches. Either I add two methods to inDeepTreeWalker: isShadowHost and isInsertationPoint and let the inspector add some pseudo nodes (let's say #shadow-root and #content-selector) when it finds these special nodes... which is arguably very hacky... but probably not too hard.

Or I can hack inDeepTreeWalker further, so it includes the shadow-roots and inseration points as nodes into the walk. This might be hard and will further complicate inDeepTreeWalkers logic, but I think this is the right way to do it.

Before I jump into implementing this, I wonder what people think about this.

NOTE: the end result should be the same in a sense that neither of them will show younger shadow trees. Just the oldest one, which I think the one that is shown (still struggling with the spec so I can be wrong here...). Frankly, I'm not sure how to inspect younger shadow trees since for those we should re-run the distribution algorithm and ignore the older ones, so I think it's better to let them stay hidden. But I can be totally wrong about the role of older vs younger shadow trees so I'm asking for some help from William to clarify their role and give me some hints what to do with them during inspection.
Flags: needinfo?(wchen)
Flags: needinfo?(bgrinstead)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #54)
> NOTE: the end result should be the same in a sense that neither of them will
> show younger shadow trees. Just the oldest one, which I think the one that
> is shown (still struggling with the spec so I can be wrong here...).
> Frankly, I'm not sure how to inspect younger shadow trees since for those we
> should re-run the distribution algorithm and ignore the older ones, so I
> think it's better to let them stay hidden. But I can be totally wrong about
> the role of older vs younger shadow trees so I'm asking for some help from
> William to clarify their role and give me some hints what to do with them
> during inspection.

I think your terms are backwards.  The most recently added tree (aka the youngest tree) is the one that is being rendered - http://www.w3.org/TR/shadow-dom/#multiple-shadow-trees.  It could be valuable to be able to inspect an older tree from the markup view (while displaying it greyed out or similar), but the visible tree is more important when inspecting the page.
(In reply to Brian Grinstead [:bgrins] from comment #55)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #54)
> > NOTE: the end result should be the same in a sense that neither of them will
> > show younger shadow trees. Just the oldest one, which I think the one that
> > is shown (still struggling with the spec so I can be wrong here...).
> > Frankly, I'm not sure how to inspect younger shadow trees since for those we
> > should re-run the distribution algorithm and ignore the older ones, so I
> > think it's better to let them stay hidden. But I can be totally wrong about
> > the role of older vs younger shadow trees so I'm asking for some help from
> > William to clarify their role and give me some hints what to do with them
> > during inspection.
> 
> I think your terms are backwards.  The most recently added tree (aka the
> youngest tree) is the one that is being rendered -
> http://www.w3.org/TR/shadow-dom/#multiple-shadow-trees.  It could be
> valuable to be able to inspect an older tree from the markup view (while
> displaying it greyed out or similar), but the visible tree is more important
> when inspecting the page.

And a case where we would definitely want to be able to inspect older shadow roots is if the youngest shadow root had a <shadow> insertion point, and the older one is visible on the page.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #54)
> I think there are two approaches. Either I add two methods to
> inDeepTreeWalker: isShadowHost and isInsertationPoint and let the inspector
> add some pseudo nodes (let's say #shadow-root and #content-selector) when it
> finds these special nodes... which is arguably very hacky... but probably
> not too hard.
> 
> Or I can hack inDeepTreeWalker further, so it includes the shadow-roots and
> inseration points as nodes into the walk. This might be hard and will
> further complicate inDeepTreeWalkers logic, but I think this is the right
> way to do it.
> 
> Before I jump into implementing this, I wonder what people think about this.

Just to summarize how I understand how the different approaches would work with regards to shadow roots (I'm not clear on how or if we would want to treat insertion points differently in the inspector).

Approach 1: When processing a node for the inspector, the shadow children would be returned as children of the node, and they would look like siblings to non-shadow chlidren.  I would call walker.isShadowHost(node), and if so, I would inject a new document fragment element (named #shadow-root) inside of the markup tree, then put all of the shadow children underneath this element.  I'm not clear on how we would deal with more than one shadow roots in this case - would the children from all shadow roots be returned as children of the node, or only the youngest?

Approach 2: When processing a node for the inspector, the shadow root would be returned as a document fragment child of node, alongside any non-shadow children that the node has.  That shadow root's children were children of node in approach 1.  If there were more than one root, they would each be included in the children set of node.

If my understanding is right, then approach 2 seems ideal to me primarily because it seems more straightforward for dealing with multiple shadow roots.  But, if there was a way to deal with multiple shadow roots in approach 1, that could work too.
Flags: needinfo?(bgrinstead)
There are two different trees that developers might want to inspect, the composed tree (where the shadow root and insertion points are transparent) and the "light DOM" tree which is just the basic parent/child relationship between the nodes.

I am against approach 2 because it confuses these two type of trees. Children of a host node are very different from the shadow root, likewise, children of an insertion point are very different from the nodes that have been distributed to the insertion point.

I like approach 1 because it give us more flexibility on how we decide to show the shadow root(s) and the node distributed to insertion points. The best way to convey this information might not be pretending there is a parent/child relation like in approach 2.

As for multiple generations of shadow root, the youngest is definitely the most important to show because it is what appears in the composed tree. It might make more sense to associate the older shadow root with the <shadow> insertion point instead of the host because that is where the older shadow root is inserted in the composed tree.
Flags: needinfo?(wchen)
(Assignee)

Comment 59

3 years ago
Seems like I set up a nice trap for myself because I was talking about two conceptual options in Comment 53 and then two implementation options in Comment 54 for composed tree version... So now approach 1 and approach 2 is kind of ambiguous... But anyway, first I will answer both comments than try to summarize what we need, and let's worry about implementation details later. That's usually the easy part :)

(In reply to Brian Grinstead [:bgrins] from comment #57)

> Approach 1: When processing a node for the inspector, the shadow children
> would be returned as children of the node, and they would look like siblings
> to non-shadow chlidren.  I would call walker.isShadowHost(node), and if so,
> I would inject a new document fragment element (named #shadow-root) inside
> of the markup tree, then put all of the shadow children underneath this
> element.  I'm not clear on how we would deal with more than one shadow roots
> in this case - would the children from all shadow roots be returned as
> children of the node, or only the youngest?

When I say insert that extra node, all I mean is that we have to do some visual
representation of the fact that we are entering the shadow realm (or in case of
insertation point we're leaving it and that case we also want to show the selector
filter probably because we are such a nice people) So it's just an extra pseudo
item in the view that is not associated with any real node.

So that should look like:

...
  > node
    > #shadow-root                   <--- this is not an actual node
      > shadow-child-of-node
      ...
    > #shadow-root2                  <--- this is the older shadow root, it cannot be opened (inactive)

selecting #shadow-root would be different then selecting a node, since there is no actual node associated with it. It's just a visual aid. However in the future it can be extended to let's say, in another window, we
get a "light DOM" view of the underlying shadow DOM from it.

For insertation point we would do something similar. And when #insertation-point is selected we would see the selector filter for example. For #shadow we could point somehow which #shadow-root is it fetching it's elements.

> If there were more than one root, they would each be
> included in the children set of node.

Thing is, we have to be very careful that what we show is helping people to understand the concepts of the shadow DOM and not confusing them. So I'm with William on this one, that showing the children of the older shadow DOM here would be highly confusing, since those nodes will be distributed somewhere else, or in some case won't be at all. So I would put there a placeholder for the older shadow roots, but nothing more. And later on I would use those placeholders to get a view of the "light DOM" view of that shadow DOM but in a different window, completely separated. So in the main inspector view we would show the distributed tree (with some decorating pseudo items in the view) and nothing else.
(Assignee)

Comment 60

3 years ago
(In reply to William Chen [:wchen] from comment #58)
> best way to convey this information might not be pretending there is a
> parent/child relation like in approach 2.

Approach 2 might be a dead end anyway, since inDeepTreeWalker should stay
compatible with the original version... Regardless, I fully agree. What do
you think about the pseudo items in the view of the inspector as representation?
Another approach might be to use colors... or simply add some extra window that
contains info about where that node is actually coming from... I kind of like
the extra item in the view approach as separators, but maybe even that is
confusing if we don't make it obvious somehow that those are not actual nodes...

> 
> As for multiple generations of shadow root, the youngest is definitely the
> most important to show because it is what appears in the composed tree. It
> might make more sense to associate the older shadow root with the <shadow>
> insertion point instead of the host because that is where the older shadow
> root is inserted in the composed tree.

We could do both... I think it's an important detail to show that there is an
older shadow-host there, we just should not let it be traversed if it's not
the youngest one.
(Assignee)

Comment 61

3 years ago
Created attachment 8447877 [details] [diff] [review]
inDeepTreeWalker fixup

A couple of fixups in the inDeepTreeWalker. Hopefully I can get back this bug later this week and get it into shape...
For devtools, I'd be happy to land this just with support for ::before / ::after / XBL anon content and deal with shadow DOM inspection in another bug.  That would surely limit the scope of the changes here, at least on the frontend, and postpone some of the bigger UX decisions that need to be made for how to represent / inspect shadow DOM.
(Assignee)

Comment 63

3 years ago
Created attachment 8448006 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v3

Rebased and merged version of the inDeepTreeWalker patches.
Attachment #8392122 - Attachment is obsolete: true
Attachment #8447877 - Attachment is obsolete: true
(Assignee)

Comment 64

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #62)
> For devtools, I'd be happy to land this just with support for ::before /
> ::after / XBL anon content and deal with shadow DOM inspection in another
> bug.  That would surely limit the scope of the changes here, at least on the
> frontend, and postpone some of the bigger UX decisions that need to be made
> for how to represent / inspect shadow DOM.

Sounds like a plan, I will try to clean up the patches for a review this week one
remaining issue is that assertion removal hack... that cannot be landed for sure.
Not sure if it's still an issue or not... and actually it's shadow DOM related so
it should not be a blocker for ::before ::after stuff.

William, did/will you have time to look into the issue from Comment 39?
Flags: needinfo?(wchen)
Created attachment 8448056 [details] [diff] [review]
inDeepTreeWalker-lastchild-iframe.patch

The lastest v3 patch for inDeepTreeWalker wasn't including the document fragment as the last child for an iframe, so this updates it to do so.
Comment on attachment 8392124 [details] [diff] [review]
turning off assertion

Marking as obsolete - I'm not seeing any issues with assertions anymore even without this patch applied.
Attachment #8392124 - Attachment is obsolete: true
Created attachment 8448060 [details] [diff] [review]
devtools-integration-part2.patch

Rebased frontend patch
Attachment #8422421 - Attachment is obsolete: true
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #64)
> William, did/will you have time to look into the issue from Comment 39?
It should be fixed now by bug 1017798.
Flags: needinfo?(wchen)
Created attachment 8448250 [details] [diff] [review]
devtools-anon.patch

A single updated patch to apply on top of the inDeepTreeWalker work that adds ::before/::after/anon inspection to devtools
Attachment #8407759 - Attachment is obsolete: true
Attachment #8422423 - Attachment is obsolete: true
Attachment #8448060 - Attachment is obsolete: true
Whiteboard: [see comment 10]
(Assignee)

Comment 70

3 years ago
Comment on attachment 8420831 [details] [diff] [review]
shadowroot should not be in doc

Should be also obsolete.
Attachment #8420831 - Attachment is obsolete: true
Created attachment 8450230 [details] [diff] [review]
devtools-anon.patch

Patrick, can you do an initial review of these changes (except for the tests, which I'm still working on)?  I have Attachment 8448006 [details] [diff] and Attachment 8448056 [details] [diff] applied in front of this.
Attachment #8450230 - Flags: feedback?(pbrosset)
Attachment #8448250 - Attachment is obsolete: true
Depends on: 1034110
Comment on attachment 8450230 [details] [diff] [review]
devtools-anon.patch

Review of attachment 8450230 [details] [diff] [review]:
-----------------------------------------------------------------

Great to see this moving forward!

I've reviewed the code changes, but not the tests as you said you were still working on them.
See my comments below.
My feedback is positive globally, I don't see any problem with the approach you've chosen. I just have a few comments here and there about the details. Also I think the markup-view could benefit from some minor refactoring. Also, styles.js could benefit from some renaming/rework of the code in addElementsRule as its trying to mix 2 things that are pretty different right now.

I've also applied and testing the patches locally. Here are some things I've noticed:

- We need to file a follow-up bug for 835896 to make sure the full-text search also finds matches in pseudo-elements

- I've managed a few times to get an blank markup-view, with no errors in the console. Hard to say precisely how I've done it, but just open the inspector and navigate to a few sites (entering a query in the url bar and pressing enter to go to google did it for me a few times).

- I had an exception when modifying one of the selectors in the rule-view, with a pseudo-element selected in the markup-view. Steps: Select a pseudo, focus a selector in the rule-view, change it, commit, select another element, and then the pseudo element again.
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: Line must be greater than or equal to 1, got -1
Full stack: SourceMapConsumer_findMapping@resource://gre/modules/devtools/SourceMap.jsm:293:1
SourceMapConsumer_originalPositionFor@resource://gre/modules/devtools/SourceMap.jsm:326:21
StyleSheetActor<.getOriginalLocation</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/stylesheets.js:774:1
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7

*************************
console.error: 
  Message: TypeError: Line must be greater than or equal to 1, got -1
  Stack:
    SourceMapConsumer_findMapping@resource://gre/modules/devtools/SourceMap.jsm:293:1
SourceMapConsumer_originalPositionFor@resource://gre/modules/devtools/SourceMap.jsm:326:21
StyleSheetActor<.getOriginalLocation</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/stylesheets.js:774:1
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7

SourceMapConsumer_findMapping@resource://gre/modules/devtools/SourceMap.jsm:293:1
SourceMapConsumer_originalPositionFor@resource://gre/modules/devtools/SourceMap.jsm:326:21
StyleSheetActor<.getOriginalLocation</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/stylesheets.js:774:1
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
console.error: 
  unknownError
console.error: 
  unknownError

I got that error on codepen.com but wasn't able to reproduce it on http://bgrins.github.io/devtools-demos/inspector/pseudo.html.

- On http://bgrins.github.io/devtools-demos/inspector/pseudo.html select a ::before pseudo-element, delete the :before part of both selectors in the rule-view: the view is empty and selecting another element and the ::before element again leads to the rule-view showing the message: "No element selected."

- The css transform highlighter doesn't highlight pseudo elements, which is weird because if I followed your changes correctly, when a ::before/::after element is selected, the rule-view actually thinks the element is a real element, not a pseudo one. Therefore http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1231 should be false, and so the highlighter should be shown, but I haven't debugged nor looked into the details. And in any case this can be done via a follow-up if you want.

- All pseudo-elements selectors in the rule-view appear greyed-out, as if they didn't match the node.

- Can you think of a way we could have the highlighter element picker be able to pick pseudo-elements? Not saying we should do it, it's a separate bug of its own, but while you're into this, you might have ideas.

::: browser/devtools/fontinspector/font-inspector.js
@@ +92,5 @@
>      let contentDocument = node.ownerDocument;
>  
>      // We don't get fonts for a node, but for a range
>      let rng = contentDocument.createRange();
> +    rng.selectNodeContents(node);

Can you tell me why this is related to your patch? Was it failing for pseudo-elements? I'm not very familiar with these range functions.

::: browser/devtools/framework/selection.js
@@ +222,5 @@
>      let rawNode = null;
>      if (node.isLocal_toBeDeprecated()) {
>        rawNode = node.rawNode();
>      }
>      if (rawNode) {

Not for this patch, but I'm suspecting we could just get rid of this condition block by now. It's a pity that you need to evolve/maintain this branch for this bug, knowing that the whole thing will go away.
Do you think you could try not calling node.isLocal_toBeDeprecated() at all and directly jumping to the while(node) {...} remote part below?

::: browser/devtools/inspector/inspector-panel.js
@@ +406,5 @@
>      if (reason !== "navigateaway" &&
>          this.selection.node &&
>          this.selection.isElementNode()) {
> +      try { // Anonymous nodes do not have a selector in the document
> +        this.selectionCssSelector = CssLogic.findCssSelector(this.selection.node);

This should be a follow-up bug, but I would tend to say we should store the node to which the pseudo-element is attached here, or the parent of the anonymous node.

::: browser/devtools/markupview/markup-view.js
@@ +276,5 @@
>        }
>        parent = parent.parentNode;
>      }
>  
> +    if (container && container._isImagePreviewTarget) {

See my comment about introducing a new type of objects to the MarkupView below.
If we end up doing it, this condition could be rewritten in a more self-explanatory way because we wouldn't be testing if some private property exists, but rather we would be testing the type of container.

@@ +581,5 @@
>          // should do this).
>          type = "childList";
>          target = mutation.targetParent;
> +        mutation.removed = [];
> +        mutation.added = [];

Can you explain why you needed to add these?

@@ +1309,5 @@
>    this.elt.container = this;
>    this.children.container = this;
>  
> +  if (aNode.isPseudoElement) {
> +    this.expander.remove();

Pseudo-elements are so different from tags that I think we should be using a separate template from template-container to render them. Indeed, they're not editable, they're not containers, they can't be expanded ...
If we did that, it would mean pseudos would have a simpler template and we wouldn't have to remove the expander either.
In fact, we should probably have a separate class altogether for this new types of things in the markup-view. Probably not named MarkupPseudo though, something more generic as we may want to reuse it later for other things. MarkupReadOnlyLeaf, or something like that that sucks less.
That way, we also wouldn't have to worry about if these methods are a problem for pseudoelements: isPreviewable, _prepareImagePreview, _isImagePreviewTarget, copyImageDataUri, hasChildren, expanded, _onToggle, _onMouseDown, ... etc ...
This new class would be pretty simple. We just need to make sure it implements enough of a common API with MarkupContainer so that this change is almost transparent to MarkupView.
Also, while doing this, I guess DoctypeEditor should probably belong to this new class rather than to MarkupContainer.

::: browser/devtools/styleinspector/test/head.js
@@ +143,5 @@
>    info("Selecting the node " + nodeOrSelector);
>    let node = getNode(nodeOrSelector);
>    let updated = inspector.once("inspector-updated");
> +  if (node._form) {
> +    inspector.selection.setNodeFront(node, reason);

This is a good addition, especially with e10s down the pipe, we'll need to move away from using setNode at all. However, the 'selectNode(nodeOrSelector, ...)' signature is a bit less self explanatory now that nodeOrSelector can not only be a node or a selector but also a NodeFront. Let's maybe just rename this param to 'data' and explain in the jsdoc comment the various types that data can be.

::: toolkit/devtools/server/actors/highlighter.js
@@ +959,2 @@
>      let tagName = node.tagName;
> +    if (tagName === "_moz_generated_content_before") {

I'm ok with this, but another way (not sure which is best) would be to show the parent tagName and use the pseudoClassesBox to display the ::before/::after.
The advantage is it would give more information to the user.

@@ +1316,5 @@
> +  }
> +
> +  // Is the node connected to the document? Using getBindingParent adds
> +  // support for anonymous elements generated by a node in the document.
> +  let bindingParent = new LayoutHelpers(doc.defaultView).getRootBindingParent(node);

It occurred to me that LayoutHelpers is a class we need to instantiate only because of 1 method: isTopLevelWindow (ok, this method is then used in many places throughout the class, but it still feels to me everytime that LayoutHelpers should just be a collection of standalone functions). Anyway, for another bug.

::: toolkit/devtools/server/actors/inspector.js
@@ +1985,5 @@
>        let targetNode = change.target;
>        let mutation = {
>          type: change.type,
>          target: targetActor.actorID,
> +        numChildren: targetActor.numChildren

Why have you moved setting this property here?
It was only set for 'childList' mutation type before. Do we need the information on the client-side in all cases?

@@ +2063,5 @@
>        type: "childList",
>        target: frameActor.actorID,
>        added: [],
> +      removed: [],
> +      numChildren: 1

Same here, and also for the following calls to 'queueMutation'. I fail to see where this is needed on the client-side.

::: toolkit/devtools/server/actors/styles.js
@@ +22,5 @@
>  // but no associated rule) we fake a rule with the following style id.
>  const ELEMENT_STYLE = 100;
>  exports.ELEMENT_STYLE = ELEMENT_STYLE;
>  
> +const PSEUDO_ELEMENTS = [":first-line", ":first-letter", ":-moz-selection"];

This array is exported and used in rule-view.js.
When debugging an older target, it means the server will have [":first-line", ":first-letter", ":before", ":after", ":-moz-selection"] and the client [":first-line", ":first-letter", ":-moz-selection"].
Will this be a problem?

@@ +363,3 @@
>    {
> +    let element = node.rawNode;
> +    let pseudoElements = inherited ? [null] : [null, ...PSEUDO_ELEMENTS];

I've always been a bit puzzled with pseudoElements here, it's an array but the first item is null, and it turns out it's not really an array of pseudo elements, it's just a convenience to loop.
I think it should be renamed, although I can't find a good name for it :)

@@ +372,5 @@
> +    // pseudo.  Then we want to lie to the view and tell it that it isn't a pseudo
> +    // style so that it shows up normally.
> +    if (isBeforeOrAfter) {
> +      element = element.parentNode;
> +      pseudoElements = [form.isBeforePseudoElement ? ":before" : ":after"];

This part, along with 'pseudoElement: isBeforeOrAfter ? null : pseudo,' in 'rules.push' at the end of the function is sufficiently different from what the function used to do that I think it deserves its own function. It would make it easier to read and perhaps simplify what I've had a hard time understanding with the 'pseudoElements' array.

::: toolkit/devtools/styleinspector/css-logic.js
@@ +177,5 @@
>      this._matchedSelectors = null;
>      let win = this.viewedDocument.defaultView;
> +
> +    // Handle computed styles on pseudo by reading style rules
> +    // on the parent node with proper pseudo arg to getCSSStyleRules.

s/getCSSStyleRules/getComputedStyle

@@ +609,5 @@
> +          elementToRead = element.parentNode;
> +        } else if (element.nodeName == "_moz_generated_content_after") {
> +          pseudoToRead = ":after";
> +          elementToRead = element.parentNode;
> +        }

These 9 lines are repeated exactly in the 2 functions. Good candidate for a new function in this file.
Attachment #8450230 - Flags: feedback?(pbrosset) → feedback+
> - The css transform highlighter doesn't highlight pseudo elements, which is
> weird because if I followed your changes correctly, when a ::before/::after
> element is selected, the rule-view actually thinks the element is a real
> element, not a pseudo one. Therefore
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> styleinspector/rule-view.js#1231 should be false, and so the highlighter
> should be shown, but I haven't debugged nor looked into the details. And in
> any case this can be done via a follow-up if you want.
> 
> - All pseudo-elements selectors in the rule-view appear greyed-out, as if
> they didn't match the node.

It turns out DOMUtils.selectorMatchesElement does not support pseudo elements.  I tried adding a new method to CssLogic that relied on mozMatchesSelector on the parent and stripped the ::before/::after string, but it failed in quite a few predictable ways. Comments like http://dxr.mozilla.org/mozilla-central/source/layout/inspector/inDOMUtils.cpp?from=selectorMatchesElement#374 make me think we would need to make some changes to SelectorMatchesElement/SelectorListMatches.  Boris, can you think of a way to check if a pseudo element matches a selector?
Flags: needinfo?(bzbarsky)
Filed bug 1037519 per IRC conversation with Brian.
Depends on: 1037519
Flags: needinfo?(bzbarsky)
Created attachment 8456260 [details] [diff] [review]
devtools-anon2.patch

Thanks for the review, I've left comments inline:

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #72)
> Comment on attachment 8450230 [details] [diff] [review]
> devtools-anon.patch
> 
> Review of attachment 8450230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great to see this moving forward!
> 
> I've reviewed the code changes, but not the tests as you said you were still
> working on them.
> See my comments below.
> My feedback is positive globally, I don't see any problem with the approach
> you've chosen. I just have a few comments here and there about the details.
> Also I think the markup-view could benefit from some minor refactoring.
> Also, styles.js could benefit from some renaming/rework of the code in
> addElementsRule as its trying to mix 2 things that are pretty different
> right now.
> 
> I've also applied and testing the patches locally. Here are some things I've
> noticed:
> 
> - We need to file a follow-up bug for 835896 to make sure the full-text
> search also finds matches in pseudo-elements
> 
> - I've managed a few times to get an blank markup-view, with no errors in
> the console. Hard to say precisely how I've done it, but just open the
> inspector and navigate to a few sites (entering a query in the url bar and
> pressing enter to go to google did it for me a few times).

OK, I've seen this too.  We've had this issue even without patches applied in a variety of ways (like Bug 1036324) so I'm not 100% sure it is due to these patches, but I can continue investigating.

> - I had an exception when modifying one of the selectors in the rule-view,
> with a pseudo-element selected in the markup-view. Steps: Select a pseudo,
> focus a selector in the rule-view, change it, commit, select another
> element, and then the pseudo element again.
> 
> I got that error on codepen.com but wasn't able to reproduce it on
> http://bgrins.github.io/devtools-demos/inspector/pseudo.html.
> 
> - On http://bgrins.github.io/devtools-demos/inspector/pseudo.html select a
> ::before pseudo-element, delete the :before part of both selectors in the
> rule-view: the view is empty and selecting another element and the ::before
> element again leads to the rule-view showing the message: "No element
> selected."
>

Yes, this will not work well until we come up with a way to detect mutations for pseudo elements in Bug 1034110.  Disabled editing and adding selectors for any anonymous nodes for now, until we can find a way around that.

> - Can you think of a way we could have the highlighter element picker be
> able to pick pseudo-elements? Not saying we should do it, it's a separate
> bug of its own, but while you're into this, you might have ideas.

Not off the top of my head, but this would be nice if we could.  I notice the same problem when inspecting shadow DOM.

> ::: browser/devtools/fontinspector/font-inspector.js
> @@ +92,5 @@
> >      let contentDocument = node.ownerDocument;
> >  
> >      // We don't get fonts for a node, but for a range
> >      let rng = contentDocument.createRange();
> > +    rng.selectNodeContents(node);
> 
> Can you tell me why this is related to your patch? Was it failing for
> pseudo-elements? I'm not very familiar with these range functions.

For whatever reason it throws an exception when calling selectNode (I'm guessing because the native anonymous content is not a valid target for that function).  However, it works fine with selectNodeContents().

> ::: browser/devtools/framework/selection.js
> @@ +222,5 @@
> >      let rawNode = null;
> >      if (node.isLocal_toBeDeprecated()) {
> >        rawNode = node.rawNode();
> >      }
> >      if (rawNode) {
> 
> Not for this patch, but I'm suspecting we could just get rid of this
> condition block by now. It's a pity that you need to evolve/maintain this
> branch for this bug, knowing that the whole thing will go away.
> Do you think you could try not calling node.isLocal_toBeDeprecated() at all
> and directly jumping to the while(node) {...} remote part below?
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +406,5 @@
> >      if (reason !== "navigateaway" &&
> >          this.selection.node &&
> >          this.selection.isElementNode()) {
> > +      try { // Anonymous nodes do not have a selector in the document
> > +        this.selectionCssSelector = CssLogic.findCssSelector(this.selection.node);
> 
> This should be a follow-up bug, but I would tend to say we should store the
> node to which the pseudo-element is attached here, or the parent of the
> anonymous node.

Yeah, this would be nice.  I guess we could store the css selector and the pseudo in selectionCssSelector.  Filed Bug 1037437 as a follow up.

> @@ +1309,5 @@
> >    this.elt.container = this;
> >    this.children.container = this;
> >  
> > +  if (aNode.isPseudoElement) {
> > +    this.expander.remove();
> 
> Pseudo-elements are so different from tags that I think we should be using a
> separate template from template-container to render them. Indeed, they're
> not editable, they're not containers, they can't be expanded ...
> If we did that, it would mean pseudos would have a simpler template and we
> wouldn't have to remove the expander either.
> In fact, we should probably have a separate class altogether for this new
> types of things in the markup-view. Probably not named MarkupPseudo though,
> something more generic as we may want to reuse it later for other things.
> MarkupReadOnlyLeaf, or something like that that sucks less.
> That way, we also wouldn't have to worry about if these methods are a
> problem for pseudoelements: isPreviewable, _prepareImagePreview,
> _isImagePreviewTarget, copyImageDataUri, hasChildren, expanded, _onToggle,
> _onMouseDown, ... etc ...
> This new class would be pretty simple. We just need to make sure it
> implements enough of a common API with MarkupContainer so that this change
> is almost transparent to MarkupView.
> Also, while doing this, I guess DoctypeEditor should probably belong to this
> new class rather than to MarkupContainer.

I've refactored this in a separate patch I'll attach.

> ::: browser/devtools/markupview/markup-view.js
> @@ +276,5 @@
> >        }
> >        parent = parent.parentNode;
> >      }
> >  
> > +    if (container && container._isImagePreviewTarget) {
> 
> See my comment about introducing a new type of objects to the MarkupView
> below.
> If we end up doing it, this condition could be rewritten in a more
> self-explanatory way because we wouldn't be testing if some private property
> exists, but rather we would be testing the type of container.

Done in the separate patch, can now check `container instanceof MarkupElementContainer`.

> ::: browser/devtools/styleinspector/test/head.js
> @@ +143,5 @@
> >    info("Selecting the node " + nodeOrSelector);
> >    let node = getNode(nodeOrSelector);
> >    let updated = inspector.once("inspector-updated");
> > +  if (node._form) {
> > +    inspector.selection.setNodeFront(node, reason);
> 
> This is a good addition, especially with e10s down the pipe, we'll need to
> move away from using setNode at all. However, the
> 'selectNode(nodeOrSelector, ...)' signature is a bit less self explanatory
> now that nodeOrSelector can not only be a node or a selector but also a
> NodeFront. Let's maybe just rename this param to 'data' and explain in the
> jsdoc comment the various types that data can be.

Done

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +959,2 @@
> >      let tagName = node.tagName;
> > +    if (tagName === "_moz_generated_content_before") {
> 
> I'm ok with this, but another way (not sure which is best) would be to show
> the parent tagName and use the pseudoClassesBox to display the
> ::before/::after.
> The advantage is it would give more information to the user.

Good idea, I've updated this to show the parent node's information along with ::before/::after as a pseudo class.

> @@ +1316,5 @@
> > +  }
> > +
> > +  // Is the node connected to the document? Using getBindingParent adds
> > +  // support for anonymous elements generated by a node in the document.
> > +  let bindingParent = new LayoutHelpers(doc.defaultView).getRootBindingParent(node);
> 
> It occurred to me that LayoutHelpers is a class we need to instantiate only
> because of 1 method: isTopLevelWindow (ok, this method is then used in many
> places throughout the class, but it still feels to me everytime that
> LayoutHelpers should just be a collection of standalone functions). Anyway,
> for another bug.

Yeah, for these things I wish it was just a utility function.  I've moved this directly onto the LayoutHelpers instead of the prototype.  I suspect we can move quite a few of the other functions to do the same.

> @@ +581,5 @@
> >          // should do this).
> >          type = "childList";
> >          target = mutation.targetParent;
> > +        mutation.removed = [];
> > +        mutation.added = [];
> 
> Can you explain why you needed to add these?
> 
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1985,5 @@
> >        let targetNode = change.target;
> >        let mutation = {
> >          type: change.type,
> >          target: targetActor.actorID,
> > +        numChildren: targetActor.numChildren
> 
> Why have you moved setting this property here?
> It was only set for 'childList' mutation type before. Do we need the
> information on the client-side in all cases?
> 
> @@ +2063,5 @@
> >        type: "childList",
> >        target: frameActor.actorID,
> >        added: [],
> > +      removed: [],
> > +      numChildren: 1
> 
> Same here, and also for the following calls to 'queueMutation'. I fail to
> see where this is needed on the client-side.
> 

OK, I've updated the access to numChildren to be more inline with how it used to work. The reason I was passing numChildren: 1 here is that it was setting undefined directly on the _front for an iframe and was causing it not to show any children.  Now I'm just checking to make sure that it is set before overriding _form.numChildren.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +22,5 @@
> >  // but no associated rule) we fake a rule with the following style id.
> >  const ELEMENT_STYLE = 100;
> >  exports.ELEMENT_STYLE = ELEMENT_STYLE;
> >  
> > +const PSEUDO_ELEMENTS = [":first-line", ":first-letter", ":-moz-selection"];
> 
> This array is exported and used in rule-view.js.
> When debugging an older target, it means the server will have
> [":first-line", ":first-letter", ":before", ":after", ":-moz-selection"] and
> the client [":first-line", ":first-letter", ":-moz-selection"].
> Will this be a problem?

Hm, yeah.  The server will pass the ::before/::after as rules and they will not be marked overridden (which is the only thing the frontend does with it).

The easiest way to fix this I think is to keep the full list on the server, and skip ::before/::after when iterating there.  I've done this with a sepearate PSEUDO_ELEMENTS_TO_READ array.

Now the rule view would look for ::before/::after to mark overridden, and on a new server it just won't find any (since they never get passed in).

> @@ +363,3 @@
> >    {
> > +    let element = node.rawNode;
> > +    let pseudoElements = inherited ? [null] : [null, ...PSEUDO_ELEMENTS];
> 
> I've always been a bit puzzled with pseudoElements here, it's an array but
> the first item is null, and it turns out it's not really an array of pseudo
> elements, it's just a convenience to loop.
> I think it should be renamed, although I can't find a good name for it :)
> 
> @@ +372,5 @@
> > +    // pseudo.  Then we want to lie to the view and tell it that it isn't a pseudo
> > +    // style so that it shows up normally.
> > +    if (isBeforeOrAfter) {
> > +      element = element.parentNode;
> > +      pseudoElements = [form.isBeforePseudoElement ? ":before" : ":after"];
> 
> This part, along with 'pseudoElement: isBeforeOrAfter ? null : pseudo,' in
> 'rules.push' at the end of the function is sufficiently different from what
> the function used to do that I think it deserves its own function. It would
> make it easier to read and perhaps simplify what I've had a hard time
> understanding with the 'pseudoElements' array.
> 

Yes, that was confusing.  I've refactored this into _getAllElementRules / _getElementRules, hopefully this is clearer.

> 
> @@ +609,5 @@
> > +          elementToRead = element.parentNode;
> > +        } else if (element.nodeName == "_moz_generated_content_after") {
> > +          pseudoToRead = ":after";
> > +          elementToRead = element.parentNode;
> > +        }
> 
> These 9 lines are repeated exactly in the 2 functions. Good candidate for a
> new function in this file.

I've made a new function, called CssLogic.getStyleableElementAndPseudo.  We should come up with a better name for it though.
Attachment #8450230 - Attachment is obsolete: true
Created attachment 8456264 [details] [diff] [review]
devtools-anon-markup-refactor.patch

Patrick, this is the refactor for the markup view.  It is built on top of Attachment 8456260 [details] [diff]. This creates a MarkupContainer object that defines most of the functionality, and is inherited by:

* MarkupTextContainer (text nodes, comments)
* MarkupElementContainer (elements)
* MarkupReadOnlyContainer (doctype, pseudo elements, any other node)
Attachment #8456264 - Flags: feedback?(pbrosset)
This one has been kicking around for a year now.  Is there any chance that a basic version (for example, that shows anon children) can land NOW, and the Shadow / Pseudo stuff can wait?
(In reply to Gregg Lind (User Research - Test Pilot) from comment #77)
> This one has been kicking around for a year now.  Is there any chance that a
> basic version (for example, that shows anon children) can land NOW, and the
> Shadow / Pseudo stuff can wait?

If you look more carefully, active progress is definitely being made here!  Patches are being reviewed, so I don't think we're too far from having something.  See comment 64 where a similar concession was already agreed to.
Comment on attachment 8456264 [details] [diff] [review]
devtools-anon-markup-refactor.patch

Review of attachment 8456264 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely F+! It looks a lot better/cleaner/more extensible with this refactor.
Attachment #8456264 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 80

3 years ago
Created attachment 8458624 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v4

I think this version is ready for a review... I hope it's not terrible.
Attachment #8448006 - Attachment is obsolete: true
Attachment #8458624 - Flags: review?(bzbarsky)
Attachment #8448056 - Attachment is obsolete: true
Attachment #8356546 - Attachment is obsolete: true
Created attachment 8458667 [details] [diff] [review]
devtools-anon-temp.patch

Gabor, with the latest patch applied there seems to be an exception when setting currentNode to a subdocument.  I've attached a frontend patch with some extra logging - you can see the error when trying to expand one of the iframes at http://bgrins.github.io/devtools-demos/inspector/iframe.html.

It logs this out this when expanding an iframe:

"Setting current node " <iframe src="mutations.html"> " on document: " HTMLDocument → http://localhost/devtools-demos/inspector/iframe.html inspector.js:2824

"Setting current node " HTMLDocument → http://localhost/devtools-demos/inspector/mutations.html " on document: " HTMLDocument → http://localhost/devtools-demos/inspector/iframe.html inspector.js:2824

Exception { message: "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [inIDeepTreeWalker.currentNode]", result: 2147500037, name: "NS_ERROR_FAILURE", filename: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js", lineNumber: 2825, columnNumber: 0, inner: null, data: null, stack: "DocumentWalker@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2825:2
documentWalker@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2801:9
WalkerActor<.children</filteredWalker@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1288:13
WalkerActor<.children<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1313:25
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:943:18
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1223:14
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:545:10
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:13
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:13
", location: XPCWrappedNative_NoHelper } protocol.js:854
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 82

3 years ago
Comment on attachment 8458624 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v4

Review of attachment 8458624 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/inspector/inDeepTreeWalker.cpp
@@ +164,5 @@
> +  // Sanity check...
> +  nsCOMPtr<nsINode> newNode = do_QueryInterface(aCurrentNode);
> +  nsCOMPtr<nsINode> current = do_QueryInterface(mCurrentNode);
> +  if (newNode->GetCurrentDoc() != current->GetCurrentDoc()) {
> +    return NS_ERROR_FAILURE;

It's probably because this check... I will remove it for now. What I would like to check if the argument here is at least from the same document tree, and not from some totally unrelated document (a document from a different tab for example). But that check might be too complicated and does not worth it...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #82)
> Comment on attachment 8458624 [details] [diff] [review]
> setCurrentNode for inDeepTreeWalker. v4
> 
> Review of attachment 8458624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/inspector/inDeepTreeWalker.cpp
> @@ +164,5 @@
> > +  // Sanity check...
> > +  nsCOMPtr<nsINode> newNode = do_QueryInterface(aCurrentNode);
> > +  nsCOMPtr<nsINode> current = do_QueryInterface(mCurrentNode);
> > +  if (newNode->GetCurrentDoc() != current->GetCurrentDoc()) {
> > +    return NS_ERROR_FAILURE;
> 
> It's probably because this check... I will remove it for now. What I would
> like to check if the argument here is at least from the same document tree,
> and not from some totally unrelated document (a document from a different
> tab for example). But that check might be too complicated and does not worth
> it...

Yep, that was it - I've removed that check locally and it works fine.  We definitely want to be able to traverse up documents (at least if mShowSubDocuments is true), so maybe you could keep in the check if that bit isn't true.  Not sure if there is a better way to check for an element in a totally unrelated tab/document.
I'm not likely to get to this before Tuesday.  :(
Created attachment 8460336 [details] [diff] [review]
devtools-anon3.patch

Rebased, and folded the markup view refactors
Attachment #8456260 - Attachment is obsolete: true
Attachment #8456264 - Attachment is obsolete: true
Attachment #8458667 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 86

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #83)
> Yep, that was it - I've removed that check locally and it works fine.  We
> definitely want to be able to traverse up documents

Don't worry about it I will remove that part it's just a bug on my side.

> isn't true.  Not sure if there is a better way to check for an element in a
> totally unrelated tab/document.

Not sure if it's an important check at all, I'll let Boris decide it during the review.
Flags: needinfo?(gkrizsanits)
Created attachment 8465005 [details] [diff] [review]
devtools-anon4.patch

Rebased and ready for review
Attachment #8460336 - Attachment is obsolete: true
Attachment #8465005 - Flags: review?(pbrosset)
Blocks: 892935
I'm sorry for the terrible lag here.  I will make sure to do this ASAP when I get back.  :(
Comment on attachment 8465005 [details] [diff] [review]
devtools-anon4.patch

Review of attachment 8465005 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!
I've made a few comments below, but mostly related to coding style/formatting, so nothing that can block R+.
Also, I think one test is missing: a computedview test that checks the content when pseudo elements are selected (like the one you added for the rule-view).

::: browser/devtools/markupview/markup-view.js
@@ +1394,5 @@
>        this.elt.classList.add("not-displayed");
>      }
>    },
>  
> +  /* Other containers can choose to implement these */

The risk with comments that aren't linked to any specific properties of the prototype like this one is that they end up lost as we make changes to the class, move things around, add functions, ...

I think you should instead add a comment like "Container sub-classes may implement this method" in each method jsdoc comment where it makes sense.

::: browser/devtools/markupview/test/browser_markupview_anonymous_03.js
@@ +31,5 @@
> +  info ("Checking the <select> shadow element");
> +  let shadowChild2 = children.nodes[2];
> +  yield checkMenuEditingDisabled(shadowChild2, inspector);
> +
> +  Services.prefs.clearUserPref("dom.webcomponents.enabled");

The problem with clearing the pref here is that this code might not always be executed (if the test fails).
I think one way is to do it in the .then() handlers of the test async function.
Or, you can always do it in the registerCleanupFunction block in head.js.

::: browser/devtools/markupview/test/head.js
@@ +473,5 @@
> +/**
> + * Check to see if the inspector menu items for editing are disabled.
> + * Things like Edit As HTML, Delete Node, etc.
> + */
> +function* checkMenuEditingDisabled(nodefront, inspector) {

I'm not very comfortable with adding generator functions to head.js, it might be just fine, but my first reaction was: functions here should be as easy to use as possible and should dictate any requirements on the caller (here the caller must be a generator function handled in a Task).
I think any async function in head.js should just return a promise (possibly using Task.async to make writing these functions easier (see addNewAttributes for instance)).

@@ +480,5 @@
> +  let pasteHTMLMenuItem = inspector.panelDoc.getElementById("node-menu-pasteouterhtml");
> +  let menu = inspector.nodemenu;
> +  yield selectNode(nodefront, inspector);
> +  yield reopenMenu(menu);
> +  ok (deleteMenuItem.hasAttribute("disabled"), "Menu item is disabled");

Also I think check functions in head.js should return a boolean and have an option to turn off assertions. This would make the function easier to use in a variety of test cases.

let isEditingMenuDisabled = Task.async(function*(nodeFront, inspector, assert) {
  let deleteMenuItem = inspector.panelDoc.getElementById("node-menu-delete");
  let editHTMLMenuItem = inspector.panelDoc.getElementById("node-menu-edithtml");
  let pasteHTMLMenuItem = inspector.panelDoc.getElementById("node-menu-pasteouterhtml");
  
  let menu = inspector.nodemenu;
  yield selectNode(nodefront, inspector);
  yield reopenMenu(menu);

  let isDeleteMenuDisabled = deleteMenuItem.hasAttribute("disabled");
  let isEditHTMLMenuDisabled = editHTMLMenuItem.hasAttribute("disabled");
  let isPasteHTMLMenuDisabled = pasteHTMLMenuItem.hasAttribute("disabled");

  if (assert) {
    ok(isDeleteMenuDisabled, "Menu item is disabled");
    ok(isEditHTMLMenuDisabled, "Menu item is disabled");
    ok(isPasteHTMLMenuDisabled, "Menu item is disabled");
  }

  return isDeleteMenuDisabled && isEditHTMLMenuDisabled && isPasteHTMLMenuDisabled;
});

Or something like this anyway...
This could also be merged with checkMenuEditingEnabled.

::: browser/devtools/styleinspector/rule-view.js
@@ +1827,3 @@
>        toolbox.target.client.traits.selectorEditable &&
>        this.rule.domRule.type !== ELEMENT_STYLE &&
>        this.rule.domRule.type !== Ci.nsIDOMCSSRule.KEYFRAME_RULE

Missing ';' at the end of this condition.

::: toolkit/devtools/LayoutHelpers.jsm
@@ +498,5 @@
>  };
> +
> +/**
> + * Traverse getBindingParent until arriving upon the bound element
> + * responsible for the generation of the specified node.

Maybe add
@see https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/DOM_Interfaces#getBindingParent
in this comment as this isn't trivial

@@ +523,5 @@
> + * Determine whether a node is anonymous by determining if there
> + * is a bindingParent.
> + *
> + * @param {DOMNode} node
> + * @return {bool}

nit: Other boolean-returning methods in this file seem to be using this instead: "@return {Boolean}" (same remark for other functions below).

@@ +532,5 @@
> +};
> +
> +/**
> + * Determine whether a node is native anonymous content (as opposed
> + * to XBL anonymous or shadow DOM).

The term "native anonymous" might not be straight forward to everyone. Even if the comment says these nodes are neither XBL nor shadow, I think it's worth saying what native anonymous nodes are exactly here.

@@ +539,5 @@
> + * @return {bool}
> + *
> + */
> +LayoutHelpers.isNativeAnonymous = function(node) {
> +  let doc = node.ownerDocument;

These ~9 lines are repeated in all 3 isXXXX methods. Can you create a simple helper function in the file instead.

::: toolkit/devtools/server/actors/inspector.js
@@ +3061,4 @@
>  };
>  
> +function isXULDocument(doc) {
> +  const XUL_NS = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';

nit: Move this const to the top of the file next to XHTML_NS.

::: toolkit/devtools/server/actors/styles.js
@@ +423,5 @@
>  
> +      if (inherited) {
> +        // Don't include inherited rules if none of its properties
> +        // are inheritable.
> +        let hasInherited = Array.prototype.some.call(domRule.style, prop => {

Can be written:

let hasInherited = [...domRule.style].some(prop => DOMUtils.isInheritedProperty(prop));

::: toolkit/devtools/server/tests/mochitest/test_inspector-anonymous.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Global comments about this new file:
- Can you add 'info("...")' calls at the beginning of each 'addTest....' test step functions? So it's easier to debug.
- There are a few extra empty lines here and there.
- I think you could wrap the whole code in window.onload to avoid storing things on the global scope and having to nullify them at the end.

@@ +57,5 @@
> +  runNextTest();
> +});
> +
> +addAsyncTest(function* testPseudo() {
> +

nit: Extra empty line here
Attachment #8465005 - Flags: review?(pbrosset) → review+
Created attachment 8471806 [details] [diff] [review]
devtools-anon5.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #89)
> Comment on attachment 8465005 [details] [diff] [review]
> devtools-anon4.patch
> 
> Review of attachment 8465005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> I've made a few comments below, but mostly related to coding
> style/formatting, so nothing that can block R+.
> Also, I think one test is missing: a computedview test that checks the
> content when pseudo elements are selected (like the one you added for the
> rule-view).
> 

Added browser_computedview_pseudo-element_01.js to cover basic functionality in computed view with pseudo elements.  I've also updated the code and test for browser_inspector_breadcrumbs.js to check for ::before/::after in the breadcrumbs text (it was showing _moz_generated_content_before previously).

Also, there were a few places where getComputedStyle() was being called with a pseudo element on accident.  This was causing an assertion spew: 'we should be in a pseudo-element that is expected to contain elements'.  I've added a new  new CssLogic.getComputedStyle(node) function that performs necessary checks and gets computed style on the parent with pseudo as a second parameter if needed.  I've added tests for this function plus getBindingElementAndPseudo in test_css-logic.html.

> ::: browser/devtools/markupview/markup-view.js
> @@ +1394,5 @@
> >        this.elt.classList.add("not-displayed");
> >      }
> >    },
> >  
> > +  /* Other containers can choose to implement these */
> 
> The risk with comments that aren't linked to any specific properties of the
> prototype like this one is that they end up lost as we make changes to the
> class, move things around, add functions, ...
> 
> I think you should instead add a comment like "Container sub-classes may
> implement this method" in each method jsdoc comment where it makes sense.
> 

Yeah, removed the comment - actually none of these really end up getting reimplemented anymore (the shared MarkupContainer implementation is used even by the MarkupElementContainer)

> ::: browser/devtools/markupview/test/browser_markupview_anonymous_03.js
> @@ +31,5 @@
> > +  info ("Checking the <select> shadow element");
> > +  let shadowChild2 = children.nodes[2];
> > +  yield checkMenuEditingDisabled(shadowChild2, inspector);
> > +
> > +  Services.prefs.clearUserPref("dom.webcomponents.enabled");
> 
> The problem with clearing the pref here is that this code might not always
> be executed (if the test fails).
> I think one way is to do it in the .then() handlers of the test async
> function.
> Or, you can always do it in the registerCleanupFunction block in head.js.
> 

Moved to the registerCleanupFunction in head.js

> ::: browser/devtools/markupview/test/head.js
> @@ +473,5 @@
> > +/**
> > + * Check to see if the inspector menu items for editing are disabled.
> > + * Things like Edit As HTML, Delete Node, etc.
> > + */
> > +function* checkMenuEditingDisabled(nodefront, inspector) {
> 
> I'm not very comfortable with adding generator functions to head.js, it
> might be just fine, but my first reaction was: functions here should be as
> easy to use as possible and should dictate any requirements on the caller
> (here the caller must be a generator function handled in a Task).
> I think any async function in head.js should just return a promise (possibly
> using Task.async to make writing these functions easier (see
> addNewAttributes for instance)).
> @@ +480,5 @@
> > +  let pasteHTMLMenuItem = inspector.panelDoc.getElementById("node-menu-pasteouterhtml");
> > +  let menu = inspector.nodemenu;
> > +  yield selectNode(nodefront, inspector);
> > +  yield reopenMenu(menu);
> > +  ok (deleteMenuItem.hasAttribute("disabled"), "Menu item is disabled");
> 
> Also I think check functions in head.js should return a boolean and have an
> option to turn off assertions. This would make the function easier to use in
> a variety of test cases.
> 
> let isEditingMenuDisabled = Task.async(function*(nodeFront, inspector,
> assert) {
>   let deleteMenuItem = inspector.panelDoc.getElementById("node-menu-delete");
>   let editHTMLMenuItem =
> inspector.panelDoc.getElementById("node-menu-edithtml");
>   let pasteHTMLMenuItem =
> inspector.panelDoc.getElementById("node-menu-pasteouterhtml");
>   
>   let menu = inspector.nodemenu;
>   yield selectNode(nodefront, inspector);
>   yield reopenMenu(menu);
> 
>   let isDeleteMenuDisabled = deleteMenuItem.hasAttribute("disabled");
>   let isEditHTMLMenuDisabled = editHTMLMenuItem.hasAttribute("disabled");
>   let isPasteHTMLMenuDisabled = pasteHTMLMenuItem.hasAttribute("disabled");
> 
>   if (assert) {
>     ok(isDeleteMenuDisabled, "Menu item is disabled");
>     ok(isEditHTMLMenuDisabled, "Menu item is disabled");
>     ok(isPasteHTMLMenuDisabled, "Menu item is disabled");
>   }
> 
>   return isDeleteMenuDisabled && isEditHTMLMenuDisabled &&
> isPasteHTMLMenuDisabled;
> });
> 
> Or something like this anyway...
> This could also be merged with checkMenuEditingEnabled.
> 

Reorganized this a bit as suggested.

> @@ +532,5 @@
> > +};
> > +
> > +/**
> > + * Determine whether a node is native anonymous content (as opposed
> > + * to XBL anonymous or shadow DOM).
> 
> The term "native anonymous" might not be straight forward to everyone. Even
> if the comment says these nodes are neither XBL nor shadow, I think it's
> worth saying what native anonymous nodes are exactly here.
> 

Updated comments for all three functions (XBL/native/shadow DOM) to include more info / links about the content.

> @@ +539,5 @@
> > + * @return {bool}
> > + *
> > + */
> > +LayoutHelpers.isNativeAnonymous = function(node) {
> > +  let doc = node.ownerDocument;
> 
> These ~9 lines are repeated in all 3 isXXXX methods. Can you create a simple
> helper function in the file instead.
> 

Added LayoutHelpers.getBindingParent() that handles these couple of cases and returns the parent variable.

> ::: toolkit/devtools/server/tests/mochitest/test_inspector-anonymous.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> Global comments about this new file:
> - Can you add 'info("...")' calls at the beginning of each 'addTest....'
> test step functions? So it's easier to debug.
> - There are a few extra empty lines here and there.
> - I think you could wrap the whole code in window.onload to avoid storing
> things on the global scope and having to nullify them at the end.
> 
> @@ +57,5 @@
> > +  runNextTest();
> > +});
> > +
> > +addAsyncTest(function* testPseudo() {
> > +
> 
> nit: Extra empty line here

Cleaned this test up
Attachment #8465005 - Attachment is obsolete: true
Attachment #8471806 - Flags: review+
Blocks: 1053898
Gabor, I'm seeing some failures due to assertions in a test using the new walker at this push: https://tbpl.mozilla.org/?tree=Try&rev=fbc884efedc3.  For instance, see just above the error message at this link for the assertion: https://tbpl.mozilla.org/php/getParsedLog.php?id=45839774&tree=Try&full=1#error0.

05:58:28     INFO -  26916 INFO [Parent 1788] ###!!! ASSERTION: These should always be in sync!: 'slowNode == node', file /builds/slave/try-l64-d-00000000000000000000/build/content/base/src/nsINode.cpp, line 269
05:58:28     INFO -  26917 INFO mozilla::dom::Element::UnbindFromTree(bool, bool) [content/base/src/Element.cpp:1460]
05:58:28     INFO -  26918 INFO mozilla::dom::Element::UnbindFromTree(bool, bool) [content/base/src/Element.cpp:1523]
05:58:28     INFO -  26919 INFO nsXBLBinding::UninstallAnonymousContent(nsIDocument*, nsIContent*) [dom/xbl/nsXBLBinding.cpp:256]
05:58:28     INFO -  26920 INFO nsXBLBinding::cycleCollection::Unlink(void*) [dom/xbl/nsXBLBinding.cpp:146]
05:58:28     INFO -  26921 INFO nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3269]
05:58:28     INFO -  26922 INFO nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) [xpcom/base/nsCycleCollector.cpp:3596]
05:58:28     INFO -  26923 INFO nsCycleCollector_collectSlice(long) [tools/profiler/GeckoProfilerImpl.h:370]
05:58:28     INFO -  26924 INFO nsJSContext::RunCycleCollectorSlice() [dom/base/nsJSEnvironment.cpp:1969]
05:58:28     INFO -  26925 INFO nsTimerImpl::Fire() [xpcom/threads/nsTimerImpl.cpp:619]
05:58:28     INFO -  26926 INFO nsTimerEvent::Run() [xpcom/base/nsAutoPtr.h:836]
05:58:28     INFO -  26927 INFO nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:770]
05:58:28     INFO -  26928 INFO NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:265]
05:58:28     INFO -  26929 INFO mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:100]
05:58:28     INFO -  26930 INFO MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:230]
05:58:28     INFO -  26931 INFO MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:504]
05:58:28     INFO -  26932 INFO nsBaseAppShell::Run() [widget/xpwidgets/nsBaseAppShell.cpp:166]
05:58:28     INFO -  26933 INFO nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:279]
05:58:28     INFO -  26934 INFO XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4022]
05:58:28     INFO -  26935 INFO XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4092]
05:58:28     INFO -  26936 INFO XRE_main [toolkit/xre/nsAppRunner.cpp:4307]
05:58:28     INFO -  26937 INFO do_main [browser/app/nsBrowserApp.cpp:282]
05:58:28     INFO -  26938 INFO main [browser/app/nsBrowserApp.cpp:645]

Do you have any insight into what might be causing this?  Here is the test: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/mochitest/test_inspector-mutations-frameload.html
Flags: needinfo?(gkrizsanits)
(In reply to Brian Grinstead [:bgrins] from comment #91)
> Do you have any insight into what might be causing this?  Here is the test:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/
> mochitest/test_inspector-mutations-frameload.html

Also, I don't see it on every test run locally, but if I run it with a repeat arg it pretty consistently fails after a few times.

mach mochitest --repeat 10 toolkit/devtools/server/tests/mochitest/test_inspector-mutations-frameload.html
(Assignee)

Comment 93

3 years ago
I have no idea what this assertion means, will try to debug it and figure it out... thanks for reporting it!
(Assignee)

Comment 94

3 years ago
I could not reproduce this, than it occurred to me that your patch that causing this has not been landed yet... could you please file that patch to the bug so I can check it out?
Flags: needinfo?(gkrizsanits) → needinfo?(bgrinstead)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #94)
> I could not reproduce this, than it occurred to me that your patch that
> causing this has not been landed yet... could you please file that patch to
> the bug so I can check it out?

I just did a fresh try push at https://tbpl.mozilla.org/?tree=Try&rev=0305625ab317

If you apply these two patches, you should be able to see the errors: 

https://hg.mozilla.org/try/raw-rev/8838f54c3045 <- your patch, but with support for iframes as Comment 81
https://hg.mozilla.org/try/raw-rev/cec449d942c1 <- updated devtools frontend patch
Flags: needinfo?(bgrinstead)

Updated

3 years ago
Blocks: 1055351
(Assignee)

Updated

3 years ago
Depends on: 1056841
Blocks: 1036324
Created attachment 8481264 [details] [diff] [review]
devtools-anon6.patch

Rebased with a new try push: https://tbpl.mozilla.org/?tree=Try&rev=df09e1c1ebd3.  Gabor, FYI the inDeepTreeWalker is not building on Linux debug:

/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.h:25:32: error: 'nsINodeList' has not been declared
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.h:37:12: error: 'nsINodeList' was not declared in this scope
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.h:37:23: error: template argument 1 is invalid
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:176:1: error: prototype for 'nsresult inDeepTreeWalker::SetCurrentNode(nsIDOMNode*, nsINodeList*)' does not match any in class 'inDeepTreeWalker'
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.h:24:12: error: candidates are: nsresult inDeepTreeWalker::SetCurrentNode(nsIDOMNode*, int*)
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:158:1: error: virtual nsresult inDeepTreeWalker::SetCurrentNode(nsIDOMNode*)
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:245:50: error: no matching function for call to 'inDeepTreeWalker::SetCurrentNode(nsCOMPtr<nsIDOMNode>&, nsCOMPtr<nsINodeList>&)'
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:273:31: error: base operand of '->' is not a pointer
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:283:67: error: base operand of '->' is not a pointer
/builds/slave/try-lx-d-000000000000000000000/build/layout/inspector/inDeepTreeWalker.cpp:287:31: error: base operand of '->' is not a pointer
Attachment #8471806 - Attachment is obsolete: true
Attachment #8481264 - Flags: review+
needinfo? for Comment 96
Flags: needinfo?(gkrizsanits)
(Assignee)

Comment 98

3 years ago
It's missing a |class nsINodeList;| or an |#include "nsINodeList.h"|. Our build system is a bit weird these days, on 64 bits linux it builds just fine... Anyway, it's nothing really serious.
Flags: needinfo?(gkrizsanits)
I bet you'll see the errors locally if you temporarily move the C++ file in question to a SOURCES block in the moz.build file instead of UNIFIED_SOURCES.
Blocks: 1061462
Comment on attachment 8458624 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v4

I'm really sorry this took so long.  I had to page in what this code is actually doing...

>+++ b/layout/inspector/inDeepTreeWalker.cpp
>+GetChildren(nsIDOMNode* aParent, bool aShowAnonymousContent)
>+    children = currentAsContent->GetChildren(aShowAnonymousContent ?
>+                                             nsIContent::eAllChildren :
>+                                             nsIContent::eAllButXBL);

This is a behavior change, right?  In the !aShowAnonymousContent case we used to do GetChildNodes(), so would exclude :before/:after and native anon content.

Why is this behavior change OK?  The fact that this would expose native anon content worries me greatly, since I have no confidence in the consumers of this clas being able to handle that sanely.

>+    // XXX I don't think GetChildNodes can fail, should we do the nsresult dance here?
>+    aParent->GetChildNodes(getter_AddRefs(children));

GetChildNodes can't fail correct.  If you had an nsINode, you could use ChildNodes() instead....  That would incidentally return you an nsINodeList, so might be worth doing it that way.

>+  // XXX: is there always an nsINodeList interface? Can this be a static cast?

Yes, and "I'd guess no" in that order.

> inDeepTreeWalker::SetCurrentNode(nsIDOMNode* aCurrentNode)
>+  if (!mCurrentNode && !aCurrentNode) {

Why is this a condition that's interesting to us?  This allows passing a null aCurrentNode as long as mCurrentNode is not null, right?

>+  if (newNode->GetCurrentDoc() != current->GetCurrentDoc()) {

And then this would crash.

Please add a test.

Also, are we sure we want to compare newNode to current as opposed to our root?  What happens, exactly, if our current stops being a descendant of our root?  Or do we not live past tree mutations?  Certainly the code that interchangeably uses live and non-live lists for mSiblings seems to assume we don't live across tree mutations or something.

>+inDeepTreeWalker::SetCurrentNode(nsIDOMNode* aCurrentNode,
>+    MOZ_ASSERT(currentAsContent);

What guarantees mCurrentNode is not a document?  I don't think anything does.

>+    mCurrentIndex = mSiblings->IndexOf(currentAsContent);

What happens if this returns -1?  For example, what if mShowAnonymousContent is false and we're passed some XBL anon content?

> inDeepTreeWalker::ParentNode(nsIDOMNode** _retval)
>+  if (!mCurrentNode || mCurrentNode == mRoot) {

In particular, what happens _here_ if mCurrentNode stopped being a descendant of mRoot?

>+// FirstChild and LastChild are very similar methods, this is the generic
>+// version for internal use.

Then why is it NS_IMETHODIMP?  Should just be nsresult.

> With aReverse = true it returns the LastChild.

How about calling it EdgeChild, then?

>+inDeepTreeWalker::FirstChild(nsIDOMNode** _retval, bool aReverse)
>+  if (children && children->Length() > 0) {

This is also a behavior change, right?  If I have:

  <iframe>Hey, you have frames disabled</iframe>

then in the mShowSubDocuments the old code would walk from the iframe to the kids of the document inside the iframe, afaict, while your new code walks to the textnode child of the iframe, again afaict.  Please add a test for this.

Furthermore, even if the length is 0, the old code would walk to the _kids_ of the document, while the new code walks to the document itself...  This is symmetric with the new ParentNode(), but not compatible with what the code does right now.

Also, in the |children && children->Length() == 0| case you do walk down to the document, but keep the same "children".  Which seems pretty fishy, though I _think_ it only impacts Previous/NextSibling, which should return null for the document anyway.

>+    fchild = do_QueryInterface(children->Item(aReverse ? children->Length() - 1 : 0));

Why not IsDOMNode() here, instead of QI?

> inDeepTreeWalker::NextSibling(nsIDOMNode **_retval)
>+  if (!mCurrentNode || !mSiblings || mCurrentIndex + 2 > mSiblings->Length()) {

I think for that last condition, |mCurrentIndex + 1 >= mSiblings->Length()| would be clearer.

> inDeepTreeWalker::NextNode(nsIDOMNode **_retval)
>+  if (!mCurrentNode) {
>+    return NS_ERROR_FAILURE;

PreviousNode() returns NS_OK in this case.  Why the difference in behavior?

>+++ b/layout/inspector/inDeepTreeWalker.h
>+  NS_IMETHODIMP FirstChild(nsIDOMNode** _retval, bool aReverse);

Headers use NS_IMETHOD.  Though again, this should just be "nsresult".
Attachment #8458624 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 101

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #100)
> Comment on attachment 8458624 [details] [diff] [review]
> setCurrentNode for inDeepTreeWalker. v4
> 
> I'm really sorry this took so long.  I had to page in what this code is
> actually doing...

Thanks for looking into this despite the fact how busy you are and how time consuming this task is.

> 
> >+++ b/layout/inspector/inDeepTreeWalker.cpp
> >+GetChildren(nsIDOMNode* aParent, bool aShowAnonymousContent)
> >+    children = currentAsContent->GetChildren(aShowAnonymousContent ?
> >+                                             nsIContent::eAllChildren :
> >+                                             nsIContent::eAllButXBL);
> 
> This is a behavior change, right?  In the !aShowAnonymousContent case we
> used to do GetChildNodes(), so would exclude :before/:after and native anon
> content.
> 
> Why is this behavior change OK?  The fact that this would expose native anon
> content worries me greatly, since I have no confidence in the consumers of
> this clas being able to handle that sanely.

I don't think it's OK, I will fix it.

> Also, are we sure we want to compare newNode to current as opposed to our
> root?  What happens, exactly, if our current stops being a descendant of our
> root?  Or do we not live past tree mutations?  Certainly the code that
> interchangeably uses live and non-live lists for mSiblings seems to assume
> we don't live across tree mutations or something.

Maybe it's a stupid idea but I tried to mimic the behavior of the TreeWalker as specified:
"we have moved the currentNode out from under the TreeWalker's root node. This does not invalidate the TreeWalker; it may still be used to navigate relative to the currentNode. Calling its parentNode() operation, for example, would move it to the <subtree/> element, even though that too is outside the original root node. However, if the TreeWalker's navigation should take it back into the original root node's subtree -- for example, if rather than calling parentNode() we called nextNode(), moving the TreeWalker to the <twRoot/> element -- the root node will "recapture" the TreeWalker, and prevent it from traversing back out."
http://www.w3.org/TR/DOM-Level-2-Traversal-Range/traversal.html#TreeWalker

It might be better just to make sure that newNode is the descendant of the root or throw... I think I would prefer to throw, but since we are trying very similar things as TreeWalker which is spec-ed this way I'm not sure.

How to handle mutations is the 1 Million dollar question in many cases. I would assume that the original stack based version fails to do that gracefully so I'm not sure if it's an actual goal really. If it is I _think_ this version might have a better chance to handle it, if we could reset the cached nodelist every time a mutation in those nodes occur. Problem is ofc that I don't know how reliable are mutation events for anon nodes, or how much feasible this idea is in general in practice. I don't have high hoped, unless we just kill the cached list.

Brian, what is our current strategy to deal with mutations? I know you were working on this...

> 
> >+inDeepTreeWalker::SetCurrentNode(nsIDOMNode* aCurrentNode,
> >+    MOZ_ASSERT(currentAsContent);
> 
> What guarantees mCurrentNode is not a document?  I don't think anything does.

Nothing, I actually thought we can/should iterate over documents as well, since they are nodes too (and probably misinterpreted the original version too)... Based on your comments it seems that I was wrong.

> >+inDeepTreeWalker::FirstChild(nsIDOMNode** _retval, bool aReverse)
> >+  if (children && children->Length() > 0) {
> 
> This is also a behavior change, right?  If I have:
> 
>   <iframe>Hey, you have frames disabled</iframe>
> 
> then in the mShowSubDocuments the old code would walk from the iframe to the
> kids of the document inside the iframe, afaict, while your new code walks to
> the textnode child of the iframe, again afaict.  Please add a test for this.

Nice catch! I totally forgot about this case as a possibility, test is coming.

> 
> Furthermore, even if the length is 0, the old code would walk to the _kids_
> of the document, while the new code walks to the document itself...  This is
> symmetric with the new ParentNode(), but not compatible with what the code
> does right now.
> 

Right, so we must skip documents in the traversal as earlier stated.
Flags: needinfo?(bgrinstead)
> Maybe it's a stupid idea but I tried to mimic the behavior of the TreeWalker as specified:

That seems fine.  As long as we're not trying to rely on the rootnode comparison for safety guarantees (like avoiding crashes), what you have in the patch is reasonable.  Just double-check that we're not thus relying on it?

> I would assume that the original stack based version fails to do that gracefully 

I believe that's correct, and its consumers are sort of designed to deal with that, kinda (e.g. by doing all their walking at once, without page script able to run).  That said, the point of this bug is to add more consumers of this class, right?  Are those consumers going to be ok with the behavior?  We definitely want to at least document this gotcha.

> Nothing, I actually thought we can/should iterate over documents as well

Sure, but documents don't QI to nsIContent.  And your code is asserting currentAsContent is not null.

I guess what guarantees is is that mSiblings has nonzero length, which should never be the case when mCurrentNode is a document...  I missed that bit when reading this.  :(

> Right, so we must skip documents in the traversal as earlier stated.

At least if we want to keep backwards compat.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #101)
> (In reply to Boris Zbarsky [:bz] from comment #100)
> > Comment on attachment 8458624 [details] [diff] [review]
> > setCurrentNode for inDeepTreeWalker. v4
> > 
> > I'm really sorry this took so long.  I had to page in what this code is
> > actually doing...
> 
> Thanks for looking into this despite the fact how busy you are and how time
> consuming this task is.
> 
> > 
> > >+++ b/layout/inspector/inDeepTreeWalker.cpp
> > >+GetChildren(nsIDOMNode* aParent, bool aShowAnonymousContent)
> > >+    children = currentAsContent->GetChildren(aShowAnonymousContent ?
> > >+                                             nsIContent::eAllChildren :
> > >+                                             nsIContent::eAllButXBL);
> > 
> > This is a behavior change, right?  In the !aShowAnonymousContent case we
> > used to do GetChildNodes(), so would exclude :before/:after and native anon
> > content.
> > 
> > Why is this behavior change OK?  The fact that this would expose native anon
> > content worries me greatly, since I have no confidence in the consumers of
> > this clas being able to handle that sanely.
> 
> I don't think it's OK, I will fix it.
> 
> > Also, are we sure we want to compare newNode to current as opposed to our
> > root?  What happens, exactly, if our current stops being a descendant of our
> > root?  Or do we not live past tree mutations?  Certainly the code that
> > interchangeably uses live and non-live lists for mSiblings seems to assume
> > we don't live across tree mutations or something.
> 
> Maybe it's a stupid idea but I tried to mimic the behavior of the TreeWalker
> as specified:
> "we have moved the currentNode out from under the TreeWalker's root node.
> This does not invalidate the TreeWalker; it may still be used to navigate
> relative to the currentNode. Calling its parentNode() operation, for
> example, would move it to the <subtree/> element, even though that too is
> outside the original root node. However, if the TreeWalker's navigation
> should take it back into the original root node's subtree -- for example, if
> rather than calling parentNode() we called nextNode(), moving the TreeWalker
> to the <twRoot/> element -- the root node will "recapture" the TreeWalker,
> and prevent it from traversing back out."
> http://www.w3.org/TR/DOM-Level-2-Traversal-Range/traversal.html#TreeWalker
> 
> It might be better just to make sure that newNode is the descendant of the
> root or throw... I think I would prefer to throw, but since we are trying
> very similar things as TreeWalker which is spec-ed this way I'm not sure.
> 
> How to handle mutations is the 1 Million dollar question in many cases. I
> would assume that the original stack based version fails to do that
> gracefully so I'm not sure if it's an actual goal really. If it is I _think_
> this version might have a better chance to handle it, if we could reset the
> cached nodelist every time a mutation in those nodes occur. Problem is ofc
> that I don't know how reliable are mutation events for anon nodes, or how
> much feasible this idea is in general in practice. I don't have high hoped,
> unless we just kill the cached list.
> 
> Brian, what is our current strategy to deal with mutations? I know you were
> working on this...

In Bug 1034110 we've discussed perhaps adding a new chrome-only API to MutationObserver.  I've summarized the requirements for such an API with various forms of anonymous content here: https://bugzilla.mozilla.org/show_bug.cgi?id=1034110#c19.

I can summarize how things work with the deep tree walker / mutations on non-anonymous elements if that helps:

Basically, a new deep tree walker is instantiated any time nodes get requested by the frontend.  And it only exists during the execution of any single function call (we aren't sharing the same instance for the entire inspector).  The frontend always provides a 'current node' to traverse, so the backend just makes a new walker, sets the current node, then returns whatever they wanted (children, parents, siblings, etc).  As these nodes are discovered by a deep tree walker (in the WalkerActor [0]) they are added to a refMap that ties a DOMNode to an actor in the _ref function [1].

Now, when a document node is encountered it starts up a MutationObserver to listen for changes.  When these changes happen (in onMutations [2]), we don't actually care about them unless if there is an actor stored in the refMap.  If there is an actor for that node, we will just notify the client with some basic information to quickly update the UI.  Which in the case of a mutation that caused added or removed nodes, will include only which actors have been added / removed (nothing to do with the deep tree walker).

If the client makes another request for a node after it's been moved around due to mutations, it doesn't really matter with regards to the walker, because we will go through the exact same steps as two paragraphs above - make a brand new deep tree walker and set this node as the current node.

If we used a similar mechanism for detecting mutations within anonymous content, then I don't think we would have any special requirements for the deep tree walker's state after a mutation happens.

[0]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#1073
[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#1184
[2]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2178
Flags: needinfo?(bgrinstead)
Duplicate of this bug: 1066672
(Assignee)

Comment 105

3 years ago
Created attachment 8492131 [details] [diff] [review]
Fixes for setCurrentNode for inDeepTreeWalker. v4

I think I addressed all the issues, and also added a new flag for including the document nodes in the walk optionally (it's off by default to preserve backward compatibility).

I upload two patches. This one is a diff since the last version, and the next will be the merged version of v4 and this fixup patch. Not sure which is the easier to review, but I guess the best if you have both.
(Assignee)

Comment 106

3 years ago
Created attachment 8492133 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v5
Attachment #8492133 - Flags: review?(bzbarsky)
Comment on attachment 8492133 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v5

>+GetChildren(nsIDOMNode* aParent,
>+      nsCOMPtr<nsIDOMNodeList> children = parentAsContent->GetChildren(nsIContent::eAllChildren);
>+      ret = do_QueryInterface(children);

This seems like a very complicated way to write:

  ret = parentAsContent->GetChildren(nsIContent::eAllChildren);

The general logic in this method doesn't seem right to me in the mShowDocumentsAsNodes case.  Getting the children of an <iframe> in that case should end up with null, I'd think (or a list containing just the document or something...).

You work around this in EdgeChild, but not in SetCurrentNode; it would be better to not have to work around it at all, I'd think.

>+      // Restore state first.
>+      mCurrentNode = tmpCurrent;
>+      mSiblings = aSiblings;

  mSiblings = tmpSiblings;

no?  Please add tests that would have caught this!

>+inDeepTreeWalker::EdgeChild(nsIDOMNode** _retval, bool aFront)
>+    // GetChildren bellow, will skip the document node from

"below".  But see comments above.  This code might need to change depending on how those are addressed.
Attachment #8492133 - Flags: review?(bzbarsky) → review-
And thank you for the interdiff.  It was quite helpful!
(Assignee)

Comment 109

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #107)
> The general logic in this method doesn't seem right to me in the
> mShowDocumentsAsNodes case.  Getting the children of an <iframe> in that
> case should end up with null, I'd think (or a list containing just the
> document or something...).
> 
> You work around this in EdgeChild, but not in SetCurrentNode; it would be
> better to not have to work around it at all, I'd think.

The logic I think is correct but very messy and I wish I could do better. The right thing to do in the iframe case would be to store a list that has only one element the document. Problem is that nsINodeList is not really a node list but a content list (the name was quite misleading for me). So I cannot use it to store a document. Instead I just do a workaround in EdgeChild, so First/LastChild and NextNode will be correct and Next/PreviousSibling work too. It's not a nice solution to say the least...

I see two ways to make it less tricky, one is to get rid of this option, and just handle it the devtools code (like it was for the old version) or to use nsIDOMNodeList interface for the cache (which is a bit more clumsy). I'll talk to Brian if it worth the effort or shall I just remove showDocumentsAsNodes and save us from the extra complexity it comes with.

Brian, do you mind if the walker, unlike in the last patch you worked with, would skip the subdocument nodes from the walk? So for an iframe node the children were the children of the subdocument, insted of the subdocument itself. Would that be a lot of trouble to workaround? (note, that the original walker and the original devtools code before all these patches worked like that)
Flags: needinfo?(bgrinstead)
> The logic I think is correct

How so, in the SetCurrentNode case?
(Assignee)

Comment 111

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #110)
> > The logic I think is correct
> 
> How so, in the SetCurrentNode case?

If SetCurrentNode is called with a sub document as the arg then GetParent and GetChildren won't be called, because there is a nodeType != nsIDOMNode::DOCUMENT_NODE check in SetCurrentNode (that's actually the workaround there). So it will set the document as the current, null for siblings and -1 for the index. And when nextNode or firstChild is called GetChildren will just detect that its a document and handle it accordingly. For iframe args SetCurrentNode is nothing special.
> because there is a nodeType != nsIDOMNode::DOCUMENT_NODE check in SetCurrentNode

Oh, indeed.  I see.

Yes, this is the case I was worrying about, and you're right that it's handled correctly, albeit non-obviously.  I'm OK with the code as-is with some nice comments added about how it works.
(Assignee)

Comment 113

3 years ago
Created attachment 8493608 [details] [diff] [review]
Fixes for setCurrentNode for inDeepTreeWalker. v5

(In reply to Boris Zbarsky [:bz] from comment #112)
> handled correctly, albeit non-obviously.  I'm OK with the code as-is with
> some nice comments added about how it works.

Alright let's land it then as it is. I've added some comments, and some more tests too. This is the interdiff, the merged version will come shortly.
Attachment #8492131 - Attachment is obsolete: true
(Assignee)

Comment 114

3 years ago
Created attachment 8493610 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v6
Attachment #8458624 - Attachment is obsolete: true
Attachment #8492133 - Attachment is obsolete: true
Attachment #8493610 - Flags: review?(bzbarsky)
(Assignee)

Comment 115

3 years ago
Brian, you will have to use the walkerSubDocument.showDocumentsAsNodes = true; if you want the walker to include document nodes in the walk.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 116

3 years ago
 https://tbpl.mozilla.org/?tree=Try&rev=843984e88ba2
Comment on attachment 8493610 [details] [diff] [review]
setCurrentNode for inDeepTreeWalker. v6

r=me.  Thanks!
Attachment #8493610 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 118

3 years ago
There were quite a lot of intermittent failures on try, but nothing that seemed related to this patch... let's see if it sticks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33605277b51
https://hg.mozilla.org/mozilla-central/rev/f33605277b51
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35

Updated

3 years ago
Depends on: 1072980

Updated

3 years ago
No longer depends on: 1072980

Comment 120

3 years ago
Shouldn't this bug stay opened ? The front-end patch hasn't landed yet.
Please do the front-end work in a separate bug.  This one is already way too long.
There is already a number of front-end bugs in the "blocks" list. I think bug 920141 will be the first candidate to be fixed.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #122)
> There is already a number of front-end bugs in the "blocks" list. I think
> bug 920141 will be the first candidate to be fixed.

Yes, I'll move the frontend patch into Bug 920141
Depends on: 1079195

Updated

3 years ago
Depends on: 1097991
Depends on: 1208544
You need to log in before you can comment on or make changes to this bug.