Closed Bug 1181297 Opened 5 years ago Closed 5 years ago

Add a dom::Element debug descriptor


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox42 --- fixed


(Reporter: BenWa, Assigned: BenWa)


(Blocks 1 open bug)



(1 file, 4 obsolete files)

I'm trying to debug my way to what element is responsible for things like reflows/restyle/transition and it's a bit painful. Adding a debug descriptor for the element would help.
Attached patch patch (obsolete) — Splinter Review
This approach uses something similar to the graphics logging code. It has the benefits that it can work with std::out or printf by using ToString(aElement).

By instrumenting 'PresShell::AttributeWillChange' and friends with:
+  std::string elemDesc = ToString(*aElement);
+  printf("Changed %s\n", elemDesc.c_str());

I get:

Changed xul:slider.scrollbar.HTML
Changed xul:scrollbarbutton.scrollbar.HTML
Changed xul:scrollbarbutton.scrollbar.HTML
Changed xul:treecol['restore'].xul:treecols.xul:tree['tabList'].div.div.body.html
Changed xul:treecol['title'].xul:treecols.xul:tree['tabList'].div.div.body.html
Changed xul:scrollbar.xul:treerows.xul:stack.xul:tree['tabList'].div.div.body.html

which is useful in quickly identifying which nodes are causing frames that I don't want.
Attachment #8630671 - Flags: review?(bugs)
Assignee: nobody → bgirard
Blocks: 1180916
Comment on attachment 8630671 [details] [diff] [review]

Are you sure you don't need to forward declare in .h and #include in .cpp?
STL streams are used rarely in dom/*

>+mozilla::dom::operator<<(std::ostream& aStream, const Element& aElement)
>+  nsString elemDesc;
This should be nsAutoString. Less mallocing

>+  const Element* curr = &aElement;
>+  while (curr) {
>+    nsString tagName;
>+    nsString id;
>+    curr->GetTagName(tagName);
>+    curr->GetId(id);
>+    if (!elemDesc.IsEmpty()) {
>+      elemDesc = elemDesc + NS_LITERAL_STRING(".");
>+    }
>+    elemDesc = elemDesc + tagName;
>+    if (!id.IsEmpty()) {
>+      elemDesc = elemDesc + NS_LITERAL_STRING("['") + id +
>+                 NS_LITERAL_STRING("']");
>+    }
>+    curr = curr->GetParentElement();
>+  }
This doesn't cross XBL or Shadow DOM boundaries, but perhaps that is fine?
Also this doesn't deal with namespaces, but that is probably also just fine.

>+  nsCString str = NS_ConvertUTF16toUTF8(elemDesc);
I would write
NS_ConvertUTF16toUTF8 str(elemDesc);

But, I think you want this on nsINode.h/.cpp
and use GetLocalName and not GetTagName
and before doing GetId, do
if (curr->IsElement()) {

curr should be nsINode*, and traversing to parent
would be GetParentNode().

const Element& aElement
argument would become
const Node& aNode

This all so that one doesn't need to care about which node to print. 
One often needs to deal with textnodes for example.

>+  /**
>+   * Print a debugger friendly descriptor of this element. This will describe
>+   * the position of this element in the document.
>+   */
>+  friend std::ostream& operator<<(std::ostream& aStream, const Element& aMatrix);
That latter argument sure isn't a matrix

So, move this to nsINode.h/.cpp and make those rather trivial changes, r+
If you think you need another review, feel free to ask.
Attachment #8630671 - Flags: review?(bugs) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8630671 - Attachment is obsolete: true
Attachment #8631069 - Flags: review+
Attached patch patch v3 (obsolete) — Splinter Review
I'm not familiar with DOMString, the conversion to nsString seems a bit 'wordy'. AsAString didn't work for me, it hit 'MOZ_ASSERT(!mStringBuffer, "We already have a stringbuffer?");'. Does this seem ok?
Attachment #8631069 - Attachment is obsolete: true
Attachment #8631080 - Flags: review?(bugs)
Comment on attachment 8631080 [details] [diff] [review]
patch v3

There is const nsString& nsINode::LocalName() const.

So, remove nsString tagName; and DOMString domTagName; and then
const nsString& localName = curr->LocalName();
and then use localName, and not tagName

For id use nsAutoString.

DOMString is mostly for bindings code to have faster string handling in
Attachment #8631080 - Flags: review?(bugs) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #8631080 - Attachment is obsolete: true
Attached patch patch v4Splinter Review
Attachment #8631197 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.