Closed
Bug 1146518
Opened 9 years ago
Closed 9 years ago
Make AtkHyperlink to deal with IPC proxies
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: smaug, Assigned: tbsaunde)
References
Details
Attachments
(4 files)
1.71 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
3.93 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
2.68 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
6.81 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8596238 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8596239 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8596240 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8596241 -
Flags: review?(surkov.alexander)
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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()
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
> > > 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?
Comment 10•9 years ago
|
||
(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 11•9 years ago
|
||
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 12•9 years ago
|
||
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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
> ::: 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
Assignee | ||
Comment 15•9 years ago
|
||
(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...
Comment 16•9 years ago
|
||
(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
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fee8f02aa5b https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6c8126c488 https://hg.mozilla.org/integration/mozilla-inbound/rev/73233637e522 https://hg.mozilla.org/integration/mozilla-inbound/rev/dff454ff41ac
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4fee8f02aa5b https://hg.mozilla.org/mozilla-central/rev/3a6c8126c488 https://hg.mozilla.org/mozilla-central/rev/73233637e522 https://hg.mozilla.org/mozilla-central/rev/dff454ff41ac
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•