Last Comment Bug 759305 - De-ns-ify nsHyperTextAccessible
: De-ns-ify nsHyperTextAccessible
Status: RESOLVED FIXED
[lang=c++]
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-05-29 05:44 PDT by Mark Capella [:capella]
Modified: 2012-06-01 08:44 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (171.02 KB, patch)
2012-05-30 08:40 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch for inbound (170.22 KB, patch)
2012-05-30 17:38 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Mark Capella [:capella] 2012-05-29 05:44:33 PDT
similar to bug 748724, bug 758870, etc.

move from /html to /generic and rename all wraps etc, like we've been doing
Comment 1 Mark Capella [:capella] 2012-05-30 08:40:48 PDT
Created attachment 628350 [details] [diff] [review]
Patch (v1)

Asking Surkov for initial feedback... you've been stealing them lately :)
Comment 2 alexander :surkov 2012-05-30 08:42:58 PDT
what about to put it under mozilla::a11y namespace?
Comment 3 Mark Capella [:capella] 2012-05-30 08:45:17 PDT
I gave it a go like the last one and ran into build problems. I tried to hack my way through for a bit, but didn't resolve it. I can try a little longer to figure it out if you'd like.

I wonder if there's a better way to do this as a comprehensive / all in one approach rather than piecing it in?
Comment 4 alexander :surkov 2012-05-30 09:00:33 PDT
(In reply to Mark Capella [:capella] from comment #3)
> I gave it a go like the last one and ran into build problems. I tried to
> hack my way through for a bit, but didn't resolve it. I can try a little
> longer to figure it out if you'd like.
> 
> I wonder if there's a better way to do this as a comprehensive / all in one
> approach rather than piecing it in?

ok, let's keep them separately if it's not easy.
Comment 5 alexander :surkov 2012-05-30 09:48:34 PDT
Comment on attachment 628350 [details] [diff] [review]
Patch (v1)

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

r=me, fix nits and go land it

::: accessible/src/generic/DocAccessible.cpp
@@ +171,1 @@
>                                              (void**)&foundInterface) :

wrong indentation, please check if you can keep it on the same line

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1021,5 @@
>    * nsIAccessibleText impl.
>    */
>  NS_IMETHODIMP
> +HyperTextAccessible::GetTextBeforeOffset(PRInt32 aOffset, AccessibleTextBoundary aBoundaryType,
> +                                         PRInt32* aStartOffset, PRInt32* aEndOffset, nsAString& aText)

don't you break 80 char per line restriction? here and below

@@ +1387,5 @@
>  }
>  
>  NS_IMETHODIMP
> +HyperTextAccessible::GetLinkIndexAtOffset(PRInt32 aOffset,
> +                                          PRInt32* aLinkIndex)

keep on the same line pls

::: accessible/src/generic/Makefile.in
@@ +26,5 @@
>    TextLeafAccessible.cpp \
>    $(NULL)
> +
> +EXPORTS = \
> +  HyperTextAccessible.h \

it seems you don't need to export it

::: accessible/src/html/nsHTMLCanvasAccessible.h
@@ +10,5 @@
>  
>  /**
>   * HTML canvas accessible (html:canvas).
>   */
> +class nsHTMLCanvasAccessible : public HyperTextAccessible

please file a bug on that, it should be inherited from HyperTextAccessibleWrap

::: accessible/src/mac/mozTextAccessible.h
@@ +9,5 @@
>  @interface mozTextAccessible : mozAccessible
>  {
>    // both of these are the same old mGeckoAccessible, but already
>    // QI'd for us, to the right type, for convenience.
> +  HyperTextAccessible       *mGeckoTextAccessible;         // strong

nit: please remove those whitespaces between

::: accessible/src/msaa/nsHyperTextAccessibleWrap.cpp
@@ +47,5 @@
>  nsresult
> +HyperTextAccessibleWrap::GetModifiedText(bool aGetInsertedText,
> +                                         nsAString& aText,
> +                                         PRUint32 *aStartOffset,
> +                                         PRUint32 *aEndOffset)

type* name

::: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
@@ +118,5 @@
>    nsXFormsEditableAccessible(aContent, aDoc)
>  {
>  }
>  
> +NS_IMPL_ISUPPORTS_INHERITED3(nsXFormsInputAccessible, Accessible, HyperTextAccessible, nsIAccessibleText, nsIAccessibleEditableText)

please put each item on own line

::: accessible/tests/mochitest/hypertext/test_update.html
@@ +156,5 @@
>       href="https://bugzilla.mozilla.org/show_bug.cgi?id=625009">
>      Mozilla Bug 625009
>    </a>
>    <a target="_blank"
> +     title="Crash in HyperTextAccessible::GetText()"

ok :) you might want to rename a bug perhaps? :)

::: accessible/tests/mochitest/role/test_general.html
@@ +75,5 @@
>       title="html:input of type "file" no longer rendered to screen readers">
>      Mozilla Bug 472326
>    </a><br>
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=474261"
> +      title="Test remaining implementations in HyperTextAccessible::GetRole">

same here :)
Comment 6 Mark Capella [:capella] 2012-05-30 09:56:46 PDT
I wasn't sure about renaming the two test "title"s ... this way they are accurate as a tooltip in the test, and still point to the original bug with the correct name as-of that point in time, but maybe I should just leave it as it was?
Comment 7 alexander :surkov 2012-05-30 10:07:59 PDT
(In reply to Mark Capella [:capella] from comment #6)
> I wasn't sure about renaming the two test "title"s ... this way they are
> accurate as a tooltip in the test, and still point to the original bug with
> the correct name as-of that point in time, but maybe I should just leave it
> as it was?

yes, just leave them
Comment 8 Mark Capella [:capella] 2012-05-30 14:57:20 PDT
TRY push
https://tbpl.mozilla.org/?tree=Try&rev=64467b586550
Comment 9 Mark Capella [:capella] 2012-05-30 17:38:32 PDT
Created attachment 628549 [details] [diff] [review]
Patch for inbound

Final version, includes late fixup for warning in bug 740766.
Comment 10 Mark Capella [:capella] 2012-05-30 17:40:26 PDT
Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.
Comment 11 Mark Capella [:capella] 2012-05-31 01:55:16 PDT
Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58e26b8bd5ba
Comment 12 Ed Morley [:emorley] 2012-06-01 08:44:49 PDT
https://hg.mozilla.org/mozilla-central/rev/58e26b8bd5ba

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