969532 - add TextRange stub

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

just some objects to be filled later

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

* use nsTArray internally

Comment 2

[alexander :surkov (:asurkov)]

I think it makes sense to rename start/endAnchor to start/endContainer to stay closer to DOM range terminology.

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8378514 [details] [diff] [review]
patch2

>+class TextRange MOZ_FINAL
>+{
>+public:
>+  TextRange(HyperTextAccessible* aRoot,
>+            Accessible* aStartAnchor, int32_t aStartOffset,
>+            Accessible* aEndAnchor, int32_t aEndOffset);
>+
>+  Accessible* StartAnchor() const { return mStartAnchor; }
>+  int32_t StartOffset() const { return mStartOffset; }
>+  Accessible* EndAnchor() const { return mEndAnchor; }
>+  int32_t EndOffset() const { return mEndOffset; }

I'm tempted to just make the members public and const and drop the accessors.

>+
>+  NS_INLINE_DECL_REFCOUNTING(TextRange)

why do you need to ref count it?

>+already_AddRefed<a11y::TextRange>

why do you need a11y:: all over?

>+  /**
>+   * Return a range that encloses the text control or the document this
>+   * accessible belongs to.

so its as if you get the doc or the root accessible forthe control and then do TextRange((root, 0), (root, end)) ?

>+  /**
>+   * Return an array of disjoint ranges for selected text within the text control
>+   * or the document this accessible belongs to.

death to multiple range selections!

>+   * Return a range containing the given accessible.

is this ((aChild, 0), (aChild, end)) ?

>+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp

it seems like this dir is starting to have way too much glue :(

>+xpcTextRange::xpcTextRange(a11y::TextRange* aRange) : mRange(aRange)

why a11y:: ?

>+// nsISupports
>+NS_IMPL_ISUPPORTS1(xpcTextRange, xpcTextRange)

shouldn't that be xptTextRange, nsITextRange? (and btw wouldn't nsIAccessibleTextRange be better incase someone else has a text range?)

>+
>+// nsITextRange
>+
>+NS_IMETHODIMP
>+xpcTextRange::GetStartAnchor(nsIAccessible** aAnchor)
>+{
>+  NS_ENSURE_ARG_POINTER(aAnchor);
>+  *aAnchor = static_cast<nsIAccessible*>(mRange->StartAnchor());
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+xpcTextRange::GetStartOffset(int32_t* aOffset)
>+{
>+  NS_ENSURE_ARG_POINTER(aOffset);
>+  *aOffset = mRange->StartOffset();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMPA>+class TextRange;
>+
>+class xpcTextRange MOZ_FINAL : public nsITextRange
>+{
>+public:
>+  virtual ~xpcTextRange();

what's wrong with the autogenerated one? and why is it virtual?

>+
>+private:
>+  xpcTextRange(TextRange* aRange);
>+  friend class xpcAccessibleHyperText;

why not just make the ctor public?

>+  nsRefPtr<TextRange> mRange;

why not just copy the members?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >+  Accessible* StartAnchor() const { return mStartAnchor; }
> >+  int32_t StartOffset() const { return mStartOffset; }
> >+  Accessible* EndAnchor() const { return mEndAnchor; }
> >+  int32_t EndOffset() const { return mEndOffset; }
> 
> I'm tempted to just make the members public and const and drop the accessors.

later impl will allow TextRange to move

> >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> 
> why do you need to ref count it?

I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko

> 
> >+already_AddRefed<a11y::TextRange>
> 
> why do you need a11y:: all over?

there's TextRange object under mozilla namespace

> >+  /**
> >+   * Return a range that encloses the text control or the document this
> >+   * accessible belongs to.
> 
> so its as if you get the doc or the root accessible forthe control and then
> do TextRange((root, 0), (root, end)) ?

do you mean to add extra consturctor or about adding basic implementation for some methods?

> >+   * Return a range containing the given accessible.
> 
> is this ((aChild, 0), (aChild, end)) ?

yes

> >+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
> 
> it seems like this dir is starting to have way too much glue :(

file a bug on it?

> >+// nsISupports
> >+NS_IMPL_ISUPPORTS1(xpcTextRange, xpcTextRange)
> 
> shouldn't that be xptTextRange, nsITextRange? (and btw wouldn't
> nsIAccessibleTextRange be better incase someone else has a text range?)

sounds good

> >+
> >+private:
> >+  xpcTextRange(TextRange* aRange);
> >+  friend class xpcAccessibleHyperText;
> 
> why not just make the ctor public?

we could but xpcAccessibleHyperText is unique place where it can be created

> >+  nsRefPtr<TextRange> mRange;
> 
> why not just copy the members?

TextRange will bunch of methods which the implementation we need to share of

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > >+  Accessible* StartAnchor() const { return mStartAnchor; }
> > >+  int32_t StartOffset() const { return mStartOffset; }
> > >+  Accessible* EndAnchor() const { return mEndAnchor; }
> > >+  int32_t EndOffset() const { return mEndOffset; }
> > 
> > I'm tempted to just make the members public and const and drop the accessors.
> 
> later impl will allow TextRange to move

when does that happen? and how do you track all of them?

> > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > 
> > why do you need to ref count it?
> 
> I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko

I don't see why you need any tricks just return TextRange* and say caller needs to delete it.

I'd really rather we didn't refcount things unless we absolutely have to.

> > 
> > >+already_AddRefed<a11y::TextRange>
> > 
> > why do you need a11y:: all over?
> 
> there's TextRange object under mozilla namespace

what happens if you do
using mozilla::a11y::TextRange at the top of the file?

> > >+  /**
> > >+   * Return a range that encloses the text control or the document this
> > >+   * accessible belongs to.
> > 
> > so its as if you get the doc or the root accessible forthe control and then
> > do TextRange((root, 0), (root, end)) ?
> 
> do you mean to add extra consturctor or about adding basic implementation
> for some methods?

asking what it returns

> > >+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
> > 
> > it seems like this dir is starting to have way too much glue :(
> 
> file a bug on it?

I'd rather people work on adding less when they write patches

> > >+
> > >+private:
> > >+  xpcTextRange(TextRange* aRange);
> > >+  friend class xpcAccessibleHyperText;
> > 
> > why not just make the ctor public?
> 
> we could but xpcAccessibleHyperText is unique place where it can be created

yeah, but making xpcHyperText friend means it can poke at everything else too though I guess there isn't much else.

> > >+  nsRefPtr<TextRange> mRange;
> > 
> > why not just copy the members?
> 
> TextRange will bunch of methods which the implementation we need to share of

fine

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> > > I'm tempted to just make the members public and const and drop the accessors.
> > 
> > later impl will allow TextRange to move
> 
> when does that happen? and how do you track all of them?

soon, we follow ITextRangeProvider interface, see http://msdn.microsoft.com/en-us/library/windows/desktop/ee671377%28v=vs.85%29.aspx

> > > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > > 
> > > why do you need to ref count it?
> > 
> > I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko
> 
> I don't see why you need any tricks just return TextRange* and say caller
> needs to delete it.

is it generally accepted practice in Gecko? I feel unsafe about it

> I'd really rather we didn't refcount things unless we absolutely have to.
> 
> > > 
> > > >+already_AddRefed<a11y::TextRange>
> > > 
> > > why do you need a11y:: all over?
> > 
> > there's TextRange object under mozilla namespace
> 
> what happens if you do
> using mozilla::a11y::TextRange at the top of the file?

as long as we use mozilla namespace I think it doesn't work (error: reference to 'TextRange' is ambiguous)

> > > >+  /**
> > > >+   * Return a range that encloses the text control or the document this
> > > >+   * accessible belongs to.
> > > 
> > > so its as if you get the doc or the root accessible forthe control and then
> > > do TextRange((root, 0), (root, end)) ?
> > 
> > do you mean to add extra consturctor or about adding basic implementation
> > for some methods?
> 
> asking what it returns

ok, the answer is yes

> > > >+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
> > > 
> > > it seems like this dir is starting to have way too much glue :(
> > 
> > file a bug on it?
> 
> I'd rather people work on adding less when they write patches

so am I, but that all depends, I don't feel like I want to jump into it here

> > > why not just make the ctor public?
> > 
> > we could but xpcAccessibleHyperText is unique place where it can be created
> 
> yeah, but making xpcHyperText friend means it can poke at everything else
> too though I guess there isn't much else.

I don't mind, which way do you prefer?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > > > I'm tempted to just make the members public and const and drop the accessors.
> > > 
> > > later impl will allow TextRange to move
> > 
> > when does that happen? and how do you track all of them?
> 
> soon, we follow ITextRangeProvider interface, see
> http://msdn.microsoft.com/en-us/library/windows/desktop/ee671377%28v=vs.
> 85%29.aspx

ok, so it only changes because someone calls method on object it doesn't track changes in underlying hypertext?

> > > > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > > > 
> > > > why do you need to ref count it?
> > > 
> > > I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko
> > 
> > I don't see why you need any tricks just return TextRange* and say caller
> > needs to delete it.
> 
> is it generally accepted practice in Gecko? I feel unsafe about it

I think so, why?

> > I'd really rather we didn't refcount things unless we absolutely have to.
> > 
> > > > 
> > > > >+already_AddRefed<a11y::TextRange>
> > > > 
> > > > why do you need a11y:: all over?
> > > 
> > > there's TextRange object under mozilla namespace
> > 
> > what happens if you do
> > using mozilla::a11y::TextRange at the top of the file?
> 
> as long as we use mozilla namespace I think it doesn't work (error:
> reference to 'TextRange' is ambiguous)

ok :/

> > > > >+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
> > > > 
> > > > it seems like this dir is starting to have way too much glue :(
> > > 
> > > file a bug on it?
> > 
> > I'd rather people work on adding less when they write patches
> 
> so am I, but that all depends, I don't feel like I want to jump into it here

I guess if you considered adjusting code generator or using templates but didn't see anything easy that's ok.

> > > > why not just make the ctor public?
> > > 
> > > we could but xpcAccessibleHyperText is unique place where it can be created
> > 
> > yeah, but making xpcHyperText friend means it can poke at everything else
> > too though I guess there isn't much else.
> 
> I don't mind, which way do you prefer?

I don't really care, but making the ctor public is probably what I'd do.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> (In reply to alexander :surkov from comment #7)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > 
> > > > > I'm tempted to just make the members public and const and drop the accessors.
> > > > 
> > > > later impl will allow TextRange to move
> > > 
> > > when does that happen? and how do you track all of them?
> > 
> > soon, we follow ITextRangeProvider interface, see
> > http://msdn.microsoft.com/en-us/library/windows/desktop/ee671377%28v=vs.
> > 85%29.aspx
> 
> ok, so it only changes because someone calls method on object it doesn't
> track changes in underlying hypertext?

not sure I get it. Example is Move() method that is supposed to change member values, so they cannot be const. Also these are strong references and AddRef/Release are not const either.

> > > > > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > > > > 
> > > > > why do you need to ref count it?
> > > > 
> > > > I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko
> > > 
> > > I don't see why you need any tricks just return TextRange* and say caller
> > > needs to delete it.
> > 
> > is it generally accepted practice in Gecko? I feel unsafe about it
> 
> I think so, why?

because of memory leaks since nothing requires the caller to free the memory. Can you give me couple good examples of the use?

> > > > > why not just make the ctor public?
> > > > 
> > > > we could but xpcAccessibleHyperText is unique place where it can be created
> > > 
> > > yeah, but making xpcHyperText friend means it can poke at everything else
> > > too though I guess there isn't much else.
> > 
> > I don't mind, which way do you prefer?
> 
> I don't really care, but making the ctor public is probably what I'd do.

ok

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > (In reply to alexander :surkov from comment #7)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > 
> > > > > > I'm tempted to just make the members public and const and drop the accessors.
> > > > > 
> > > > > later impl will allow TextRange to move
> > > > 
> > > > when does that happen? and how do you track all of them?
> > > 
> > > soon, we follow ITextRangeProvider interface, see
> > > http://msdn.microsoft.com/en-us/library/windows/desktop/ee671377%28v=vs.
> > > 85%29.aspx
> > 
> > ok, so it only changes because someone calls method on object it doesn't
> > track changes in underlying hypertext?
> 
> not sure I get it. Example is Move() method that is supposed to change
> member values, so they cannot be const. Also these are strong references and

I just meant hyperTextAccessible doesn't need to track related text ranges.

> AddRef/Release are not const either.

that means you can't have nsRefPtr<const T> but I don't think it means you can't have const nsRefPtr<T>

> > > > > > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > > > > > 
> > > > > > why do you need to ref count it?
> > > > > 
> > > > > I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko
> > > > 
> > > > I don't see why you need any tricks just return TextRange* and say caller
> > > > needs to delete it.
> > > 
> > > is it generally accepted practice in Gecko? I feel unsafe about it
> > 
> > I think so, why?
> 
> because of memory leaks since nothing requires the caller to free the
> memory. Can you give me couple good examples of the use?

if I looked I expect so, but I don't really care  what you do, so long as it doesn't involve pointless refcounting.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > (In reply to alexander :surkov from comment #7)
> > > > (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > > 
> > > > > > > I'm tempted to just make the members public and const and drop the accessors.
> > > > > > 
> > > > > > later impl will allow TextRange to move
> > > > > 
> > > > > when does that happen? and how do you track all of them?
> > > > 
> > > > soon, we follow ITextRangeProvider interface, see
> > > > http://msdn.microsoft.com/en-us/library/windows/desktop/ee671377%28v=vs.
> > > > 85%29.aspx
> > > 
> > > ok, so it only changes because someone calls method on object it doesn't
> > > track changes in underlying hypertext?
> > 
> > not sure I get it. Example is Move() method that is supposed to change
> > member values, so they cannot be const. Also these are strong references and
> 
> I just meant hyperTextAccessible doesn't need to track related text ranges.

I'm not sure I follow it

> > AddRef/Release are not const either.
> 
> that means you can't have nsRefPtr<const T> but I don't think it means you
> can't have const nsRefPtr<T>

right, I think I could have const nsRefPtr for root accessible only, I can't do that other members

> > > > > > > >+  NS_INLINE_DECL_REFCOUNTING(TextRange)
> > > > > > > 
> > > > > > > why do you need to ref count it?
> > > > > > 
> > > > > > I don't like nsAutoPtr tricks in methods, it's not really often used in Gecko
> > > > > 
> > > > > I don't see why you need any tricks just return TextRange* and say caller
> > > > > needs to delete it.
> > > > 
> > > > is it generally accepted practice in Gecko? I feel unsafe about it
> > > 
> > > I think so, why?
> > 
> > because of memory leaks since nothing requires the caller to free the
> > memory. Can you give me couple good examples of the use?
> 
> if I looked I expect so, but I don't really care  what you do, so long as it
> doesn't involve pointless refcounting.

ok, I hope so

Comment 12

[alexander :surkov (:asurkov)]

Trev, if refptr thing is only one thing left then we need to reach an agreement asap and move forward.

As I understood from yesterday chat on irc there are two alternatives
1) T* GetTextRange() to return a raw pointer of dynamically created object
2) void GetTextRange(T&) making the caller to create an object
 
As I mentioned 1st is dangerous because we can leak if a caller forgets to free the memory (it's quite easy if the caller doesn't know that returned object is not reference counted)

I don't really exited about 2nd approach because of name conflicts: GetObject name rather means to return an object but not to change the given object. 

Also neither approach 1) nor 2) doesn't answer clearly how to move ownership of these objects nicely from nsTArray to XPCOM array.

On the other hand I don't see any significant disadvantage of refptr approach. I would agree it doesn't look necessary but that's rather cleaning up issue we can deal later with. That cannot be our top priority.

Comment 13

[Trevor Saunders (:tbsaunde)]

> As I understood from yesterday chat on irc there are two alternatives
> 1) T* GetTextRange() to return a raw pointer of dynamically created object

you could call it Create / Construct or something if that makes you happier or return a nsAutoPtr<T>

> 2) void GetTextRange(T&) making the caller to create an object
>  
> As I mentioned 1st is dangerous because we can leak if a caller forgets to
> free the memory (it's quite easy if the caller doesn't know that returned
> object is not reference counted)

how many callers do you seriously think we're going to have? besides use the MOZ_COUNT_CTOR / DTOR macros and leak checking will catch this.  Or just return an nsAutoPtr<T> and not have the problem.

> I don't really exited about 2nd approach because of name conflicts:
> GetObject name rather means to return an object but not to change the given
> object. 

we already have things like that though (the stuff on Table<,Cell}Accessible to get arrays is the first to come to mind.

Also this has the advantage you can embed the TextRange with the XPCTextRange or put it on the stack and save a dynamic allocation.

> Also neither approach 1) nor 2) doesn't answer clearly how to move ownership
> of these objects nicely from nsTArray to XPCOM array.

embed TextRange as member of XPCTextRange then have XPCTextrRange(TextRange&&); and loop over array elements which is more or less the same thing you have now.  Given that took me all of five seconds to come up with I'm sure you can think of something else if you want to.

> On the other hand I don't see any significant disadvantage of refptr
> approach. I would agree it doesn't look necessary but that's rather cleaning
> up issue we can deal later with. That cannot be our top priority.

No, its an issue of whose allowed to do what and what the life times of things are. I alsmost gaurentee you we'll never be able to stop refcounting things once we start because someone will hold onto them past when they should.

Comment 14

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8379044 [details] [diff] [review]
patch3

perhaps I should have been more explicit sooner, but refcounting things that don't have to be is not ok.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 8379044 [details] [diff] [review]
patch3

We have to have an insight of one more reviewer. As I understand there's only one thing that prevents Trev to r+ is reference counting of TextRange object. As I said I'm agree with Trev ref counting is not necessary in this case but I'm not exited to jump into investigations of the issue 'cause I still don't see a nice approach I would be ok with (see comment #12). I'm pretty fine to have a follow up on it.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8379044 [details] [diff] [review]
patch3

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

Removing review since as per IRC Trevor will tweak this patch.

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: bug 969532 - part 1 - make nsRefPtr movable

with a tiny little style cleanup to another nsRefPtr ctor

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8383097 [details] [diff] [review]
bug 969532 - part 1 - make nsRefPtr movable

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

r=me with the changes below.

::: xpcom/base/nsAutoPtr.h
@@ +919,5 @@
> +      template<typename U>
> +      nsRefPtr(nsRefPtr<U>&& aRefPtr)
> +            : mRawPtr(aRefPtr.mRawPtr)
> +        {
> +          aRefPtr.mRawPtr = nullptr;

Please add a static_assert here about mozilla::IsBaseOf<T, U>::value being true.

@@ +987,5 @@
> +      template<typename U>
> +      nsRefPtr<T>&
> +      operator=(nsRefPtr<U>&& aRefPtr)
> +      {
> +        assign_assuming_AddRef(aRefPtr.mRawPtr);

Here too.

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: bug 969532 - add TextRange stub

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8383273 [details] [diff] [review]
bug 969532 - add TextRange stub

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

r=me with questions. Only made it through one full pass but trusting you guys have both gone over this as well.

::: accessible/public/nsIAccessibleText.idl
@@ +201,5 @@
> +  /**
> +   * Return an array of disjoint ranges for selected text within the text control
> +   * or otherwise the document this accessible belongs to.
> +   */
> +  readonly attribute nsIArray selectionRanges;

Really? This can return the document?

::: accessible/src/xpcom/xpcAccessibleHyperText.cpp
@@ +370,5 @@
> +xpcAccessibleHyperText::GetEnclosingRange(nsIAccessibleTextRange** aRange)
> +{
> +  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
> +  if (text->IsDefunct())
> +    return NS_ERROR_FAILURE;

Sorry I'm rusty. Is it safer to NS_ENSURE_ARG_POINTER/nullptr *aRange before returning here or do we expect callers to be careful? (ditto for other methods that take "nsIAccessibleTextRange**" below)

@@ +377,5 @@
> +  text->EnclosingRange(range->mRange);
> +  NS_ASSERTION(range->mRange.IsValid(),
> +               "Should always have an enclosing range!");
> +
> +    range.forget(aRange);

nit: remove the 2 extra indent spaces.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #20)
> > +   * Return an array of disjoint ranges for selected text within the text control
> > +   * or otherwise the document this accessible belongs to.
> > +   */
> > +  readonly attribute nsIArray selectionRanges;
> 
> Really? This can return the document?

that or otherwise belongs to the text control not to array

Comment 22

[Trevor Saunders (:tbsaunde)]

Attachment: bug 969532 - part 1 - make nsRefPtr movable r=froydnj

turns out gcc 4.4 wants me to know it can still cause grief so it decides that for code like
nsRefPtr<T> x;
nsRefPtr<U> y;
where U inherits T
x = y;
it should call the move constructor not the copy one, so lets work around that by templating the copy ctor / operator = too.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8385730 [details] [diff] [review]
bug 969532 - part 1 - make nsRefPtr movable r=froydnj

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

That is bizarre behavior and seems very footgun-ish.  I'm sure there's some C++ "guideline" about this, though.

Comment 24

[Trevor Saunders (:tbsaunde)]

> That is bizarre behavior and seems very footgun-ish.  I'm sure there's some
> C++ "guideline" about this, though.

actually, here's the discussion from #developers yesterday on this.

16:49 < botond> tbsaunde: so gcc 4.4's implementation of rvalue references is based on an early working draft of C++11 which allowed binding an rvalue reference to an lvalue
16:50 < botond> tbsaunde: keep in mind gcc 4.4 was released in early 2009, while C++11 was only finalized in early 2011
16:50 < botond> tbsaunde: that explains why the operator=(nsRefPtr<U>&&) is being called
16:50 < botond> tbsaunde: but i still don't understand why it's giving the access error
16:51 < tbsaunde> botond: well, it'd be kind of bad if mSpdySession lost its ref probably ...
16:51 < botond> tbsaunde: yeah. that's a problem...
16:51 < tbsaunde> botond: I'm not suprised gcc 4.4 is doing something screwy, but confirmation is nice ;)
16:52 <@khuey> fwiw, I believe that MSVC does that too
16:52 <@khuey> at least the version we're using
16:53 < tbsaunde> botond: so is there some way I can prevent this happening?
16:54 < tbsaunde> or do we just have to wait on making refptrs and nsAutoPtr movable  until we kill gcc 4.4 and msvc2010 ?
16:54  * botond thinks
16:56 < botond> tbsaunde: i notice there is no operator=(const nsRefPtr<U>&)
16:56 < botond> tbsaunde: if there were one, gcc 4.4 would prefer it over the rvalue ref version when the argument is an lvalue
16:56 < botond> tbsaunde: so, we can probably solve the problem by adding that

so yeah probably worth a dev platform post and more fist shaking at gcc 4.4 and msvc :\

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97e52c3f95ae

Comment 26

[Kip]

Hey folks. I seem to be having a compile time issue that's possibly related to this, but I can't be certain. I've posted it on [e-mail] here:

https://groups.google.com/forum/#!topic/mozilla.dev.builds/zb_pcPmB_AA

I admit I don't fully understand the error, but I do see the same "error: reference to ‘TextRange’ is ambiguous" message mentioned above. Here is my post:

Hey list,

I'm having difficulty building Thunderbird from source under Ubuntu
Trusty 14.04 (amd64). I believe I am using Mercurial head based off of
the latest I pulled out of the repository today.

        $ python client.py checkout

The above command completes without issue and I believe I should have
all (I hope) of the build dependencies.

        $ sudo apt-get build-deps thunderbird

However, when I go to build the source according to the wiki...

        $ ./mozilla/mach build

...my compilation proceeds fine well into it, but eventually bails with
what looks like a source level error. However, I've been seeing this
same error for the last few days and seeing commits since, so I'm
wondering if there is something up with my system, such as a missing
dependency, environment variable, an incomplete mozconfig, etc..
Compile log here:

        <http://pastebin.com/WLfkA4QB>

Any help much appreciated!