Fix some confusing naming and casts in DirectionalityUtils.cpp

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

This node argument may point to any node type, and is not necessarily
an Element.  Its value is only useful when it's pointing to a text node,
but calling it a text node before we have checked its type is confusing.
I don't really understand this patch, since afaict we only ever pass a text node to aChangedNode for ResetTextNodeDirection. We always check HasTextNodeDirectionalityMap before calling it. HasTextNodeDirectionalityMap only returns true if a nsTextNodeDirectionalityMap was constructed for the node, which only ever happens from AddEntryToMap, which is only ever called with the result of WalkDescendantsSetDirectionFromText (which only returns a text node). Could we make HasTextNodeDirectionalityMap assert that the node type is nsIDOMNode::TEXT_NODE? Maybe we can also make WalkDescendantsSetDirectionFromText return a nsTextNode* and nsTextNodeDirectionalityMap and AddEntryToMap take one?

At that point we could make TextNodeChangedDirection take a nsTextNode* (nsGenericDOMDataNode::SetTextInternal checks the nodetype before calling it) and then all the callers of ResetTextNodeDirection pass in a nsTextNode* for aChangedNode afaict.
Flags: needinfo?(ehsan)
(In reply to Peter Van der Beken [:peterv] from comment #2)
> I don't really understand this patch, since afaict we only ever pass a text
> node to aChangedNode for ResetTextNodeDirection. We always check
> HasTextNodeDirectionalityMap before calling it. HasTextNodeDirectionalityMap
> only returns true if a nsTextNodeDirectionalityMap was constructed for the
> node, which only ever happens from AddEntryToMap, which is only ever called
> with the result of WalkDescendantsSetDirectionFromText (which only returns a
> text node).

D'oh, I was missing this part!  Sorry...

> Could we make HasTextNodeDirectionalityMap assert that the node
> type is nsIDOMNode::TEXT_NODE? Maybe we can also make
> WalkDescendantsSetDirectionFromText return a nsTextNode* and
> nsTextNodeDirectionalityMap and AddEntryToMap take one?
> 
> At that point we could make TextNodeChangedDirection take a nsTextNode*
> (nsGenericDOMDataNode::SetTextInternal checks the nodetype before calling
> it) and then all the callers of ResetTextNodeDirection pass in a nsTextNode*
> for aChangedNode afaict.

Will do!
Flags: needinfo?(ehsan)
Comment on attachment 8731276 [details] [diff] [review]
Fix some confusing naming and casts in DirectionalityUtils.cpp

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

I don't really understand this patch, since afaict we only ever pass a text node to aChangedNode for ResetTextNodeDirection. We always check HasTextNodeDirectionalityMap before calling it. HasTextNodeDirectionalityMap only returns true if a nsTextNodeDirectionalityMap was constructed for the node, which only ever happens from AddEntryToMap, which is only ever called with the result of WalkDescendantsSetDirectionFromText (which only returns a text node). Could we make HasTextNodeDirectionalityMap assert that the node type is nsIDOMNode::TEXT_NODE, and maybe we can also make WalkDescendantsSetDirectionFromText return a nsTextNode* and nsTextNodeDirectionalityMap and AddEntryToMap take one?

At that point we could make TextNodeChangedDirection take a nsTextNode* (nsGenericDOMDataNode::SetTextInternal checks the nodetype before calling it) and then all the callers of ResetTextNodeDirection pass in a nsTextNode* for aChangedNode afaict.
Attachment #8731276 - Flags: review?(peterv) → review-
Comment on attachment 8732380 [details] [diff] [review]
Use the nsTextNode concrete type in several places in DirectionalityUtils.cpp instead of nsINode and nsIContent

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

Thanks for doing this.
Attachment #8732380 - Flags: review?(peterv) → review+
Backed out for Reftest failures at least on Linux x64 debug, e.g. dir_auto-set-contained-dir-L.html

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f112e7dad6f7

First non-busted run after push (which also shows the failures): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=52195522dbac
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=24440903&repo=mozilla-inbound
17:10:46     INFO -  REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/bidi/dirAuto/dir_auto-contained-dir-L-ref.html | 531 / 1712 (31%)
17:10:46     INFO -  ++DOMWINDOW == 54 (0x7f94d3023c00) [pid = 1128] [serial = 1144] [outer = 0x7f94d9455400]
17:10:46     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/bidi/dirAuto/dir_auto-set-contained-dir-L.html | image comparison (==), max difference: 255, number of differing pixels: 3284
Flags: needinfo?(ehsan)
D'oh, I made such a rookie mistake: used IsNodeOfType() instead of NodeType().  Embarrassing!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #9)
> D'oh, I made such a rookie mistake: used IsNodeOfType() instead of
> NodeType().  Embarrassing!

Hmpf, sorry for missing that.
https://hg.mozilla.org/mozilla-central/rev/337789a5f9c6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.