Last Comment Bug 762038 - get rid HyperTextAccessible CID
: get rid HyperTextAccessible CID
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
Depends on: 762394 762396
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-06-06 07:03 PDT by alexander :surkov
Modified: 2012-06-20 02:19 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.71 KB, patch)
2012-06-16 15:55 PDT, Andrew Quartey [:drexler]
tbsaunde+mozbugs: review+
surkov.alexander: feedback+
Details | Diff | Review
updated_patch (8.42 KB, patch)
2012-06-18 23:14 PDT, Andrew Quartey [:drexler]
tbsaunde+mozbugs: review+
Details | Diff | Review
patch_for_checkin (8.73 KB, patch)
2012-06-19 08:42 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review

Description alexander :surkov 2012-06-06 07:03:51 PDT
All instances like
nsRefPtr<HyperTextAccessible> text(do_QueryObject(obj));
replace on
nsRefPtr<Accessible> acc(do_QueryObject(obj));
HyperTextAccessible* text = acc->AsHyperText();

remove NS_HYPERTEXTACCESSIBLE_IMPL_CID and fix HyperTextAccessible::QueryInterface
Comment 1 Andrew Quartey [:drexler] 2012-06-16 15:55:10 PDT
Created attachment 633848 [details] [diff] [review]
patch

Locally tested and sent to try: https://tbpl.mozilla.org/?tree=Try&rev=1c1fbb2ad915
Comment 2 alexander :surkov 2012-06-16 21:54:49 PDT
Comment on attachment 633848 [details] [diff] [review]
patch

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

looks ok

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +154,5 @@
>    NS_ENSURE_TRUE(aStartOffset <= aEndOffset &&
>                   (aStartOffset >= 0 || (aStartOffset != -1 && aEndOffset != -1)),
>                   NS_ERROR_INVALID_ARG);
> +  
> +  nsRefPtr<Accessible> acc(do_QueryObject(aTextAccessible));

technically it may be null so add a check please, if null return invalid arg

@@ +170,5 @@
>    mStartOffset = aStartOffset;
>    mEndOffset = aEndOffset;
>  
>    nsRefPtr<Accessible> oldPosition = mPosition.forget();
> +  mPosition = do_QueryObject(newPosition);

mPosition = newPosition should be enough
Comment 3 Trevor Saunders (:tbsaunde) 2012-06-18 16:07:13 PDT
Comment on attachment 633848 [details] [diff] [review]
patch

>-
>-  nsRefPtr<HyperTextAccessible> newPosition = do_QueryObject(aTextAccessible);
>+  

you introduce trailing whitespace here too.
Comment 4 Andrew Quartey [:drexler] 2012-06-18 23:14:11 PDT
Created attachment 634302 [details] [diff] [review]
updated_patch

fixed the nits and the Mac OS QI call failure for a HyperTextAccessible initialization i talked about in IRC. File in question is mozTextAccessible.mm
Comment 5 Trevor Saunders (:tbsaunde) 2012-06-19 06:00:00 PDT
Comment on attachment 634302 [details] [diff] [review]
updated_patch

>+  nsRefPtr<Accessible> acc(do_QueryObject(aTextAccessible));
>+  if (!acc)
>+    return NS_ERROR_INVALID_ARG;
>+  HyperTextAccessible* newPosition = acc->AsHyperText();

nit, blank line after return

>   if ((self = [super initWithAccessible:accessible])) {
>-    CallQueryInterface(accessible, &mGeckoTextAccessible);
>+    mGeckoTextAccessible = accessible->AsHyperText();

so, this code is crazy, but I hope there is now an unmatched Release() somewhere.  The easiest fix is probably to just NS_IF_ADDREF() on mGeckoTextAccessible, but that's really broken and would add to the pile ofthings that need to be fixed here.  I suspect the correct fix is to change NS_IF_RELEASE() in expire() in this file to mGeckoTextAccessible = nsnull, but I'm not sure that's correct though it seems more sane than what we have now, and is atleast a step in the right direction.
Comment 6 Andrew Quartey [:drexler] 2012-06-19 08:42:30 PDT
Created attachment 634444 [details] [diff] [review]
patch_for_checkin

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
 
Fixed nits and set mGeckoTextAccessible = nsnull in expire().
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-06-19 16:04:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17e673803b4
Comment 8 Ed Morley [:emorley] 2012-06-20 02:19:27 PDT
https://hg.mozilla.org/mozilla-central/rev/b17e673803b4

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