Closed
Bug 758113
Opened 13 years ago
Closed 13 years ago
rm a bunch of nsIDOMNode stuff in nsHyperTextAccessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: tbsaunde, Assigned: tbsaunde)
Details
Attachments
(1 file, 3 obsolete files)
22.33 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
also has been in my queue for a wile, maybe I'll actually have time soon, or maybe someone will want to work on it first.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #626695 -
Attachment is obsolete: true
Attachment #629536 -
Flags: review?(surkov.alexander)
Comment 3•13 years ago
|
||
Comment on attachment 629536 [details] [diff] [review]
patch
Review of attachment 629536 [details] [diff] [review]:
-----------------------------------------------------------------
canceling review until we polish an approach
::: accessible/src/generic/HyperTextAccessible.cpp
@@ +2236,2 @@
> Accessible* aAccessible,
> + nsINode** aNode,
you don't addref it and it breaks the convention. If anybody passes getter_AddRefs for nsCOMPtr<nsINode> then extra release is called.
They have DOMPoint struct but they use nsIDOMNode there. I'd suggest to add our version of DOMPoint struct under a11y namespace.
btw, the same applied to HypertextOffsetsToDOMRange. You can do DOMRange. Or use two out DOMPoint. The second approach might be cleaner.
@@ +2268,3 @@
> NS_ENSURE_STATE(content);
>
> + nsIContent* parent = content->GetParent();
while you are here can you change parent to parentContent
::: accessible/src/generic/HyperTextAccessible.h
@@ +146,5 @@
> * @param aNode [out] start node
> * @param aOffset [out] offset inside the start node
> */
> nsresult HypertextOffsetToDOMPoint(PRInt32 aHTOffset,
> + nsINode** aNode, PRInt32 *aOffset);
it seems a dead method, makes sense to get rid it
@@ +165,2 @@
> PRInt32 *aStartOffset,
> + nsINode **aEndNode,
type* name
@@ +362,5 @@
>
> // Helpers
> nsresult GetDOMPointByFrameOffset(nsIFrame* aFrame, PRInt32 aOffset,
> Accessible* aAccessible,
> + nsINode** aNode, PRInt32 *aNodeOffset);
type* name
Attachment #629536 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 4•13 years ago
|
||
> ::: accessible/src/generic/HyperTextAccessible.cpp
> @@ +2236,2 @@
> > Accessible* aAccessible,
> > + nsINode** aNode,
>
> you don't addref it and it breaks the convention. If anybody passes
> getter_AddRefs for nsCOMPtr<nsINode> then extra release is called.
it certainly weird for an xpcom thing, but in the rest of the world I don't think its that odd. Its true that would cause an extra Release, on the other hand not using getter_AddRefs can cause an unbalanced AddRef() so I'm not convinced the problem is all that different.
> They have DOMPoint struct but they use nsIDOMNode there. I'd suggest to add
> our version of DOMPoint struct under a11y namespace.
do you mean just struct DOMPoint { nsINode* node; PRInt32 offset; ]; ? that doesn't seem much cleaner to me, but if you like it I guess I'm fine with it
> btw, the same applied to HypertextOffsetsToDOMRange. You can do DOMRange. Or
> use two out DOMPoint. The second approach might be cleaner.
I suspect nsRange might work nicely for the second.
Comment 5•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > you don't addref it and it breaks the convention. If anybody passes
> > getter_AddRefs for nsCOMPtr<nsINode> then extra release is called.
>
> it certainly weird for an xpcom thing, but in the rest of the world I don't
> think its that odd.
it seems it's a weird COM thing, for non COM rest world it's not odd at all since there's no Addref/Release there.
> Its true that would cause an extra Release, on the
> other hand not using getter_AddRefs can cause an unbalanced AddRef() so I'm
> not convinced the problem is all that different.
it's convention, if somebody does something like &comptr.get() then it should be considered as a bug.
> > They have DOMPoint struct but they use nsIDOMNode there. I'd suggest to add
> > our version of DOMPoint struct under a11y namespace.
>
> do you mean just struct DOMPoint { nsINode* node; PRInt32 offset; ]; ? that
> doesn't seem much cleaner to me, but if you like it I guess I'm fine with it
I wouldn't use DOMPoint if we had nice alternative.
> > btw, the same applied to HypertextOffsetsToDOMRange. You can do DOMRange. Or
> > use two out DOMPoint. The second approach might be cleaner.
>
> I suspect nsRange might work nicely for the second.
I'd say it's overkill since it's more than two pointers and two offsets (for example iirc they keep the range alive by watching tree mutations). Also if you go with DOMPoint and nsRange both then there's no nice conversion from nsRange to DOMPoint you'd need when you call GetDOMPointByFrameOffset from HypertextOffsetToDOMPoint.
Assignee | ||
Comment 6•13 years ago
|
||
> > Its true that would cause an extra Release, on the
> > other hand not using getter_AddRefs can cause an unbalanced AddRef() so I'm
> > not convinced the problem is all that different.
>
> it's convention, if somebody does something like &comptr.get() then it
> should be considered as a bug.
I was thinking of cases like &rawPtr where it may or may not be a bug.
> > > They have DOMPoint struct but they use nsIDOMNode there. I'd suggest to add
> > > our version of DOMPoint struct under a11y namespace.
> >
> > do you mean just struct DOMPoint { nsINode* node; PRInt32 offset; ]; ? that
> > doesn't seem much cleaner to me, but if you like it I guess I'm fine with it
>
> I wouldn't use DOMPoint if we had nice alternative.
Well, it seems the only method that would need it is GetDOMPointByFrameOffset() but I'm not sure what to do with that. It seems small enough when you ignore the boiler plate that inlining it in the two places its used right next to each other might not be too bad.
> > > btw, the same applied to HypertextOffsetsToDOMRange. You can do DOMRange. Or
> > > use two out DOMPoint. The second approach might be cleaner.
> >
> > I suspect nsRange might work nicely for the second.
>
> I'd say it's overkill since it's more than two pointers and two offsets (for
> example iirc they keep the range alive by watching tree mutations). Also if
> you go with DOMPoint and nsRange both then there's no nice conversion from
> nsRange to DOMPoint you'd need when you call GetDOMPointByFrameOffset from
> HypertextOffsetToDOMPoint.
Well, I think if I restructure things a little I can make HypertextOffsetsToDOMRange() take a nsRange* instead of all the out args.
Comment 7•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > Its true that would cause an extra Release, on the
> > > other hand not using getter_AddRefs can cause an unbalanced AddRef() so I'm
> > > not convinced the problem is all that different.
> >
> > it's convention, if somebody does something like &comptr.get() then it
> > should be considered as a bug.
>
> I was thinking of cases like &rawPtr where it may or may not be a bug.
not sure I follow, anyway out XPCOM argument should be addrefed.
> > I wouldn't use DOMPoint if we had nice alternative.
>
> Well, it seems the only method that would need it is
> GetDOMPointByFrameOffset() but I'm not sure what to do with that. It seems
> small enough when you ignore the boiler plate that inlining it in the two
> places its used right next to each other might not be too bad.
you could use it for HypertextOffsetsToDOMRange as well.
> Well, I think if I restructure things a little I can make
> HypertextOffsetsToDOMRange() take a nsRange* instead of all the out args.
it seems an overkill for methods like HyperTextAccessible::ScrollSubstringTo
Assignee | ||
Comment 8•13 years ago
|
||
> > Well, I think if I restructure things a little I can make
> > HypertextOffsetsToDOMRange() take a nsRange* instead of all the out args.
>
> it seems an overkill for methods like HyperTextAccessible::ScrollSubstringTo
except that I think all the cases that call it end up creating a range with the data anyway so we'd just be shuffling around where that happens a little I believe.
Comment 9•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > it seems an overkill for methods like HyperTextAccessible::ScrollSubstringTo
>
> except that I think all the cases that call it end up creating a range with
> the data anyway so we'd just be shuffling around where that happens a little
> I believe.
it should depend on kind of range, if you talk about nsRange that watches for tree mutations then we don't need it.
Assignee | ||
Comment 10•13 years ago
|
||
I decided to try the nsRange idea, and think I like how it turned out.
However I really don't have any good ideas for GetDOMPointByframe()
This patch should apply on top of the previous one, note the one XXX part should become a lot nicer with nsTypedSelection.
Comment 11•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Created attachment 630381 [details] [diff] [review]
> bug 758113 - use nsRange* as an argument to HypertextOffsetsToDOMRange()
>
> I decided to try the nsRange idea, and think I like how it turned out.
I really don't like to use nsRange when we don't need it. HypertextOffsetsToDOMRange should return two DOM points (a DOM range), nsRange is overkill.
> However I really don't have any good ideas for GetDOMPointByframe()
Why don't you like DOMPoint approach I suggested above?
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #629536 -
Attachment is obsolete: true
Attachment #630381 -
Attachment is obsolete: true
Attachment #636194 -
Flags: review?(dbolter)
Comment 13•13 years ago
|
||
You still use an nsRange right? Did you and Alexander reach agreement (re: comment 11)?
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to David Bolter [:davidb] from comment #13)
> You still use an nsRange right? Did you and Alexander reach agreement (re:
> comment 11)?
yeah, when he realized we were always goingto end up involving a range ultimately anyway. I thought that was in the bug, ubt I guess it was on irc, I can try to find the log if you like.
Comment 15•13 years ago
|
||
No log necessary - re-commencing review...
Comment 16•13 years ago
|
||
Comment on attachment 636194 [details] [diff] [review]
patch 2
Review of attachment 636194 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed and passing tests. Note I don't know this code very well. Do we have test coverage?
::: accessible/src/base/nsCoreUtils.h
@@ +136,5 @@
> * @param aEndNode end node of a range
> * @param aEndOffset an offset inside the end node
> * @param aScrollType the place a range should be scrolled to
> */
> + static nsresult ScrollSubstringTo(nsIFrame *aFrame, nsRange* aRange,
Please update comment.
@@ +150,5 @@
> * @param aEndOffset an offset inside the end node
> * @param aVertical how to align vertically, specified in percents, and when.
> * @param aHorizontal how to align horizontally, specified in percents, and when.
> */
> + static nsresult ScrollSubstringTo(nsIFrame *aFrame, nsRange* aRange,
Please update comment.
::: accessible/src/generic/HyperTextAccessible.cpp
@@ +522,5 @@
> // For text nodes, aNodeOffset comes in as a character offset
> // Text offset will be added at the end, if we find the offset in this hypertext
> // We want the "skipped" offset into the text (rendered text without the extra whitespace)
> + NS_ASSERTION(aNode->IsNodeOfType(nsINode::eCONTENT), "not content node!");
> + nsIFrame *frame = static_cast<nsIContent*>(aNode)->GetPrimaryFrame();
Aside: for some reason I balked thinking the static_cast operated on the return value.
@@ +1829,5 @@
> domSel->GetRangeAt(aSelectionNum, getter_AddRefs(range));
> NS_ENSURE_TRUE(range, NS_ERROR_FAILURE);
> }
>
> + // XXX bug 758112 will make this nice, but for now just take the easy dumb approach
That bug passed review, correct? (It isn't clear to me what your next step is)
::: accessible/src/generic/HyperTextAccessible.h
@@ +158,5 @@
> * @param aStartOffset [out] start offset of the range
> * @param aEndNode [out] end node of the range
> * @param aEndOffset [out] end offset of the range
> */
> nsresult HypertextOffsetsToDOMRange(PRInt32 aStartHTOffset,
Please update comment.
::: accessible/src/msaa/TextLeafAccessibleWrap.cpp
@@ +158,5 @@
> + nsRefPtr<nsRange> range = new nsRange();
> + if (NS_FAILED(range->SetStart(mContent, aStartIndex)))
> + return E_FAIL;
> +
> + if (NS_FAILED(range->SetStart(mContent, aEndIndex)))
SetEnd?
Attachment #636194 -
Flags: review?(dbolter) → review+
Comment 17•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > ::: accessible/src/generic/HyperTextAccessible.cpp
> > @@ +2236,2 @@
> > > Accessible* aAccessible,
> > > + nsINode** aNode,
> >
> > you don't addref it and it breaks the convention. If anybody passes
> > getter_AddRefs for nsCOMPtr<nsINode> then extra release is called.
>
> it certainly weird for an xpcom thing, but in the rest of the world I don't
> think its that odd. Its true that would cause an extra Release, on the
> other hand not using getter_AddRefs can cause an unbalanced AddRef() so I'm
> not convinced the problem is all that different.
One AddRef too many is a leak. One Release too many is a critical security bug. I'd say there is a difference.
Comment 18•13 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> (In reply to David Bolter [:davidb] from comment #13)
> > You still use an nsRange right? Did you and Alexander reach agreement (re:
> > comment 11)?
>
> yeah, when he realized we were always goingto end up involving a range
> ultimately anyway. I thought that was in the bug, ubt I guess it was on
> irc, I can try to find the log if you like.
yes, we talked on irc. We always end up with DOM ranges, I still think it's overkill but it's how DOM layer works.
Comment 19•13 years ago
|
||
Comment on attachment 636194 [details] [diff] [review]
patch 2
Review of attachment 636194 [details] [diff] [review]:
-----------------------------------------------------------------
::: accessible/src/base/nsCoreUtils.cpp
@@ +257,5 @@
> return false;
> }
>
> nsresult
> +nsCoreUtils::ScrollSubstringTo(nsIFrame *aFrame, nsRange* aRange,
type* name, here and below
::: accessible/src/generic/HyperTextAccessible.h
@@ +359,5 @@
>
> // Helpers
> nsresult GetDOMPointByFrameOffset(nsIFrame* aFrame, PRInt32 aOffset,
> Accessible* aAccessible,
> + mozilla::a11y::DOMPoint* aPoint);
(not looking into details) why don't you return DOMPoint as return value? (iirc that was idea)
Assignee | ||
Comment 20•13 years ago
|
||
> @@ +257,5 @@
> > return false;
> > }
> >
> > nsresult
> > +nsCoreUtils::ScrollSubstringTo(nsIFrame *aFrame, nsRange* aRange,
>
> type* name, here and below
yeah, arg I hate when there are other pointers from the ones I change on a line :/
> ::: accessible/src/generic/HyperTextAccessible.h
> @@ +359,5 @@
> >
> > // Helpers
> > nsresult GetDOMPointByFrameOffset(nsIFrame* aFrame, PRInt32 aOffset,
> > Accessible* aAccessible,
> > + mozilla::a11y::DOMPoint* aPoint);
>
> (not looking into details) why don't you return DOMPoint as return value?
> (iirc that was idea)
ok, I don't remember understanding it that way, although it certainly does make more sense to add the DOMPoint struct then. It seems like a good idea, but I'd sort of prefer to get this landed and then fix other parts of this mess like returning the struct.
> r=me with comments addressed and passing tests. Note I don't know this code
> very well. Do we have test coverage?
some certainly editabletext/ or something like that, I certainly had to debug a test failure or two at some point.
> @@ +522,5 @@
> > // For text nodes, aNodeOffset comes in as a character offset
> > // Text offset will be added at the end, if we find the offset in this hypertext
> > // We want the "skipped" offset into the text (rendered text without the extra whitespace)
> > + NS_ASSERTION(aNode->IsNodeOfType(nsINode::eCONTENT), "not content node!");
> > + nsIFrame *frame = static_cast<nsIContent*>(aNode)->GetPrimaryFrame();
>
> Aside: for some reason I balked thinking the static_cast operated on the
> return value.
yeah, I'll probably just change that to AsContent() since it exists now.
> @@ +1829,5 @@
> > domSel->GetRangeAt(aSelectionNum, getter_AddRefs(range));
> > NS_ENSURE_TRUE(range, NS_ERROR_FAILURE);
> > }
> >
> > + // XXX bug 758112 will make this nice, but for now just take the easy dumb approach
>
> That bug passed review, correct? (It isn't clear to me what your next step
> is)
it depends on this to some degree I think, any way that's the order in my tree.
> ::: accessible/src/msaa/TextLeafAccessibleWrap.cpp
> @@ +158,5 @@
> > + nsRefPtr<nsRange> range = new nsRange();
> > + if (NS_FAILED(range->SetStart(mContent, aStartIndex)))
> > + return E_FAIL;
> > +
> > + if (NS_FAILED(range->SetStart(mContent, aEndIndex)))
>
> SetEnd?
iirc nsRange makes the start and end equivelent when you set one, and haven't yet set the second, but sure it makes sense.
Assignee | ||
Comment 21•13 years ago
|
||
> > ::: accessible/src/msaa/TextLeafAccessibleWrap.cpp
> > @@ +158,5 @@
> > > + nsRefPtr<nsRange> range = new nsRange();
> > > + if (NS_FAILED(range->SetStart(mContent, aStartIndex)))
> > > + return E_FAIL;
> > > +
> > > + if (NS_FAILED(range->SetStart(mContent, aEndIndex)))
> >
> > SetEnd?
>
> iirc nsRange makes the start and end equivelent when you set one, and
> haven't yet set the second, but sure it makes sense.
oh, no, I'm just a clown (red nose and floppy shoes included). Have I mentioned recently how anoying it is to not have tests for this stuff?
Assignee | ||
Comment 22•13 years ago
|
||
Comment 23•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Updated•8 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•