Closed Bug 1146518 Opened 5 years ago Closed 5 years ago

Make AtkHyperlink to deal with IPC proxies

Categories

(Core :: Disability Access APIs, defect)

36 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: smaug, Assigned: tbsaunde)

References

Details

Attachments

(4 files)

Depends on: 1146615
Attachment #8596238 - Flags: review?(surkov.alexander)
Attachment #8596239 - Flags: review?(surkov.alexander)
Attachment #8596240 - Flags: review?(surkov.alexander)
Attachment #8596241 - Flags: review?(surkov.alexander)
Comment on attachment 8596238 [details] [diff] [review]
Only pass hyper links to MaiHyperlink::MaiHyperlink

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

::: accessible/atk/nsMaiHyperlink.cpp
@@ -94,5 @@
>      mHyperlink(aHyperLink),
>      mMaiAtkHyperlink(nullptr)
>  {
> -  if (!mHyperlink->IsLink())
> -    return;

no assertion just in case?
Attachment #8596238 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8596239 [details] [diff] [review]
allow MaiHyperlink to store references to proxies

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

I don't have problems with the patch but it'd be good to have somebody who knows multiprocess architecture. Out of curiosity why do you need those proxies if ATK has own version of the tree? Couldn't we work same way as we plugins are implemented?

::: accessible/atk/nsMai.h
@@ +50,5 @@
>    return aMajor < atkMajorVersion ||
>           (aMajor == atkMajorVersion && aMinor <= atkMinorVersion);
>  }
>  
> +// This is or'd with the pointer in MaiAtkObject::accWrap if the wrap-ee is a

curious what or'd mean

I think we use /* */ for comments in global

::: accessible/atk/nsMaiHyperlink.h
@@ +33,5 @@
> +      if (!mHyperlink || mHyperlink & IS_PROXY)
> +        return nullptr;
> +
> +      Accessible* link = reinterpret_cast<Accessible*>(mHyperlink);
> +      return link->IsLink() ? link : nullptr;

from previous patch hyperlink has to be IsLink()
(In reply to alexander :surkov from comment #5)
> Comment on attachment 8596238 [details] [diff] [review]
> Only pass hyper links to MaiHyperlink::MaiHyperlink
> 
> Review of attachment 8596238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/atk/nsMaiHyperlink.cpp
> @@ -94,5 @@
> >      mHyperlink(aHyperLink),
> >      mMaiAtkHyperlink(nullptr)
> >  {
> > -  if (!mHyperlink->IsLink())
> > -    return;
> 
> no assertion just in case?

Well, part of the point of the patch is to make the later patches easier by making it easy for the class to store things other than a Accessible*


> I don't have problems with the patch but it'd be good to have somebody who
> knows multiprocess architecture. Out of curiosity why do you need those
> proxies if ATK has own version of the tree? Couldn't we work same way as we
> plugins are implemented?

I don't understand the question, what are you talking about? why MaiHyperlink exists and the  MaiAtkHyperlink doesn't just point at the related AtkObject or Accessible? I don't know, but its always been that way.

> ::: accessible/atk/nsMai.h
> @@ +50,5 @@
> >    return aMajor < atkMajorVersion ||
> >           (aMajor == atkMajorVersion && aMinor <= atkMinorVersion);
> >  }
> >  
> > +// This is or'd with the pointer in MaiAtkObject::accWrap if the wrap-ee is a
> 
> curious what or'd mean

bitwise or.

> I think we use /* */ for comments in global

what do you mean global?

> ::: accessible/atk/nsMaiHyperlink.h
> @@ +33,5 @@
> > +      if (!mHyperlink || mHyperlink & IS_PROXY)
> > +        return nullptr;
> > +
> > +      Accessible* link = reinterpret_cast<Accessible*>(mHyperlink);
> > +      return link->IsLink() ? link : nullptr;
> 
> from previous patch hyperlink has to be IsLink()

I guess it makes sense to change it to an assert.
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
 
> > no assertion just in case?
> 
> Well, part of the point of the patch is to make the later patches easier by
> making it easy for the class to store things other than a Accessible*

ok

> > I don't have problems with the patch but it'd be good to have somebody who
> > knows multiprocess architecture. Out of curiosity why do you need those
> > proxies if ATK has own version of the tree? Couldn't we work same way as we
> > plugins are implemented?
> 
> I don't understand the question, what are you talking about? why
> MaiHyperlink exists and the  MaiAtkHyperlink doesn't just point at the
> related AtkObject or Accessible? I don't know, but its always been that way.

yes, it's good question, I was curious about it but didn't asked :) but I meant the approach in general, it's out of scope of this bug though. Anyway, can't we use atk sockets mechanism to connect content processes trees similar way we do for plugins? (instead of proxies and all stuff you do for windows)


> > I think we use /* */ for comments in global
> 
> what do you mean global?

I meant global scope, i.e. we tend to use // for comments in functions, /* */ for everything else. anyway style issue, not big deal.
> > > I don't have problems with the patch but it'd be good to have somebody who
> > > knows multiprocess architecture. Out of curiosity why do you need those
> > > proxies if ATK has own version of the tree? Couldn't we work same way as we
> > > plugins are implemented?
> > 
> > I don't understand the question, what are you talking about? why
> > MaiHyperlink exists and the  MaiAtkHyperlink doesn't just point at the
> > related AtkObject or Accessible? I don't know, but its always been that way.
> 
> yes, it's good question, I was curious about it but didn't asked :) but I
> meant the approach in general, it's out of scope of this bug though. Anyway,
> can't we use atk sockets mechanism to connect content processes trees
> similar way we do for plugins? (instead of proxies and all stuff you do for
> windows)

please don't start that discussion yet again.

> 
> > > I think we use /* */ for comments in global
> > 
> > what do you mean global?
> 
> I meant global scope, i.e. we tend to use // for comments in functions, /*
> */ for everything else. anyway style issue, not big deal.

ok, I guess its more common, but not the first exception.  I guess I don't really care though leaving it is easier and may mkae blaime work slightly better?
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> > yes, it's good question, I was curious about it but didn't asked :) but I
> > meant the approach in general, it's out of scope of this bug though. Anyway,
> > can't we use atk sockets mechanism to connect content processes trees
> > similar way we do for plugins? (instead of proxies and all stuff you do for
> > windows)
> 
> please don't start that discussion yet again.

I don't recall we talked about linux part though, but it's not part of the bug so we don't have to discuss it here
Comment on attachment 8596239 [details] [diff] [review]
allow MaiHyperlink to store references to proxies

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

I don't have problems with the patch but it'd be good to have somebody who knows multiprocess architecture. Out of curiosity why do you need those proxies if ATK has own version of the tree? Couldn't we work same way as we plugins are implemented?

r=me, it'd be great if somebody who knows this code would look at it

::: accessible/atk/nsMai.h
@@ +50,5 @@
>    return aMajor < atkMajorVersion ||
>           (aMajor == atkMajorVersion && aMinor <= atkMinorVersion);
>  }
>  
> +// This is or'd with the pointer in MaiAtkObject::accWrap if the wrap-ee is a

curious what or'd mean

I think we use /* */ for comments in global

::: accessible/atk/nsMaiHyperlink.h
@@ +33,5 @@
> +      if (!mHyperlink || mHyperlink & IS_PROXY)
> +        return nullptr;
> +
> +      Accessible* link = reinterpret_cast<Accessible*>(mHyperlink);
> +      return link->IsLink() ? link : nullptr;

from previous patch hyperlink has to be IsLink()
Attachment #8596239 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8596240 [details] [diff] [review]
create MaiHyperlinks for proxies

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

rs=me

::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
@@ +22,1 @@
>      return nullptr;

should be nicer to have on if for all (I think you don't want warn if !IsLink())
Attachment #8596240 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8596241 [details] [diff] [review]
make atk hyper link impl support proxies

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

wrong indentation though the whole file (4 spaces vs 2), otherwise looks good, rs=me

::: accessible/atk/nsMaiHyperlink.cpp
@@ +181,2 @@
>  
>      return g_strdup(cautoStr.get());

might be nicer to not have else {} part

::: accessible/atk/nsMaiHyperlink.h
@@ +41,5 @@
> +  {
> +    if (!(mHyperlink & IS_PROXY))
> +      return nullptr;
> +
> +    return reinterpret_cast<ProxyAccessible*>(mHyperlink);

just style: return mHyperlink & IS_PROXY ? mHyperlink : nullptr;
Attachment #8596241 - Flags: review?(surkov.alexander) → review+
> ::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
> @@ +22,1 @@
> >      return nullptr;
> 
> should be nicer to have on if for all (I think you don't want warn if
> !IsLink())

huh?
 I don't understand that sentence
(In reply to alexander :surkov from comment #13)
> Comment on attachment 8596241 [details] [diff] [review]
> make atk hyper link impl support proxies
> 
> Review of attachment 8596241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> wrong indentation though the whole file (4 spaces vs 2), otherwise looks
> good, rs=me
> 
> ::: accessible/atk/nsMaiHyperlink.cpp
> @@ +181,2 @@
> >  
> >      return g_strdup(cautoStr.get());
> 
> might be nicer to not have else {} part

how?

> ::: accessible/atk/nsMaiHyperlink.h
> @@ +41,5 @@
> > +  {
> > +    if (!(mHyperlink & IS_PROXY))
> > +      return nullptr;
> > +
> > +    return reinterpret_cast<ProxyAccessible*>(mHyperlink);
> 
> just style: return mHyperlink & IS_PROXY ? mHyperlink : nullptr;

its a really long line already...
(In reply to alexander :surkov from comment #12)
> ::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
> @@ +22,1 @@
> >      return nullptr;
> 
> should be nicer to have on if for all (I think you don't want warn if
> !IsLink())

on -> one, all -> everything, like
if (accWrap && !accWrap->IsLink() || !GetProxy(ATK_OBJECT(aImpl))
  return nullptr;

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

> > >      return g_strdup(cautoStr.get());
> > 
> > might be nicer to not have else {} part
> 
> how?

if (something) {
  do_something();
  return g_strdup(cautoStr.get());
}

do_something();
return g_strdup(cautoStr.get());

> 
> > ::: accessible/atk/nsMaiHyperlink.h
> > @@ +41,5 @@
> > > +  {
> > > +    if (!(mHyperlink & IS_PROXY))
> > > +      return nullptr;
> > > +
> > > +    return reinterpret_cast<ProxyAccessible*>(mHyperlink);
> > 
> > just style: return mHyperlink & IS_PROXY ? mHyperlink : nullptr;
> 
> its a really long line already...

you can do it in two lines
(In reply to alexander :surkov from comment #16)
> (In reply to alexander :surkov from comment #12)
> > ::: accessible/atk/nsMaiInterfaceHyperlinkImpl.cpp
> > @@ +22,1 @@
> > >      return nullptr;
> > 
> > should be nicer to have on if for all (I think you don't want warn if
> > !IsLink())
> 
> on -> one, all -> everything, like
> if (accWrap && !accWrap->IsLink() || !GetProxy(ATK_OBJECT(aImpl))
>   return nullptr;

ok, I guess that's fine though really I guess it should be an assert for !IsLink() since that really shouldn't happen.

> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > > >      return g_strdup(cautoStr.get());
> > > 
> > > might be nicer to not have else {} part
> > 
> > how?
> 
> if (something) {
>   do_something();
>   return g_strdup(cautoStr.get());
> }
> 
> do_something();
> return g_strdup(cautoStr.get());
> 

I guess that's fine with me.

> > 
> > > ::: accessible/atk/nsMaiHyperlink.h
> > > @@ +41,5 @@
> > > > +  {
> > > > +    if (!(mHyperlink & IS_PROXY))
> > > > +      return nullptr;
> > > > +
> > > > +    return reinterpret_cast<ProxyAccessible*>(mHyperlink);
> > > 
> > > just style: return mHyperlink & IS_PROXY ? mHyperlink : nullptr;
> > 
> > its a really long line already...
> 
> you can do it in two lines

I think it'd actually end up being 3 (in part because we need to add a anding out of the IS_PROXY bit, and over 1 I tend to prefer if anyway.
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.