Last Comment Bug 758113 - rm a bunch of nsIDOMNode stuff in nsHyperTextAccessible
: rm a bunch of nsIDOMNode stuff in nsHyperTextAccessible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 22:02 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-07-02 15:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (21.94 KB, patch)
2012-05-23 22:03 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (20.29 KB, patch)
2012-06-02 18:23 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 758113 - use nsRange* as an argument to HypertextOffsetsToDOMRange() (15.36 KB, patch)
2012-06-05 17:29 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch 2 (22.33 KB, patch)
2012-06-24 15:03 PDT, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-05-23 22:02:08 PDT

    
Comment 1 Trevor Saunders (:tbsaunde) 2012-05-23 22:03:33 PDT
Created attachment 626695 [details] [diff] [review]
wip

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.
Comment 2 Trevor Saunders (:tbsaunde) 2012-06-02 18:23:18 PDT
Created attachment 629536 [details] [diff] [review]
patch
Comment 3 alexander :surkov 2012-06-03 19:28:32 PDT
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
Comment 4 Trevor Saunders (:tbsaunde) 2012-06-03 22:15:30 PDT
> ::: 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 alexander :surkov 2012-06-03 22:33:06 PDT
(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.
Comment 6 Trevor Saunders (:tbsaunde) 2012-06-04 08:04:42 PDT
> > 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 alexander :surkov 2012-06-04 08:12:58 PDT
(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
Comment 8 Trevor Saunders (:tbsaunde) 2012-06-04 08:20:30 PDT
> > 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 alexander :surkov 2012-06-04 08:31:44 PDT
(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.
Comment 10 Trevor Saunders (:tbsaunde) 2012-06-05 17:29:48 PDT
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.

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 alexander :surkov 2012-06-05 22:26:42 PDT
(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?
Comment 12 Trevor Saunders (:tbsaunde) 2012-06-24 15:03:54 PDT
Created attachment 636194 [details] [diff] [review]
patch 2
Comment 13 David Bolter [:davidb] 2012-06-26 08:07:19 PDT
You still use an nsRange right? Did you and Alexander reach agreement (re: comment 11)?
Comment 14 Trevor Saunders (:tbsaunde) 2012-06-26 09:07:12 PDT
(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 David Bolter [:davidb] 2012-06-26 09:11:03 PDT
No log necessary - re-commencing review...
Comment 16 David Bolter [:davidb] 2012-06-26 09:44:19 PDT
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?
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-06-26 09:48:44 PDT
(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 alexander :surkov 2012-06-27 23:10:14 PDT
(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 alexander :surkov 2012-06-27 23:12:05 PDT
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)
Comment 20 Trevor Saunders (:tbsaunde) 2012-06-28 08:44:58 PDT
> @@ +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.
Comment 21 Trevor Saunders (:tbsaunde) 2012-06-28 09:22:23 PDT
> > ::: 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?
Comment 22 Trevor Saunders (:tbsaunde) 2012-07-01 23:24:12 PDT
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/2433bf50d64e
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-07-02 15:30:56 PDT
https://hg.mozilla.org/mozilla-central/rev/2433bf50d64e

Note You need to log in before you can comment on or make changes to this bug.