759305 - De-ns-ify nsHyperTextAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

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]]

Attachment: Patch (v1)

Asking Surkov for initial feedback... you've been stealing them lately :)

Comment 2

[alexander :surkov (:asurkov)]

what about to put it under mozilla::a11y namespace?

Comment 3

[Mark Capella [:capella]]

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 (:asurkov)]

(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 (:asurkov)]

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]]

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 (:asurkov)]

(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]]

TRY push
https://tbpl.mozilla.org/?tree=Try&rev=64467b586550

Comment 9

[Mark Capella [:capella]]

Attachment: Patch for inbound

Final version, includes late fixup for warning in bug 740766.

Comment 10

[Mark Capella [:capella]]

Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.

Comment 11

[Mark Capella [:capella]]

Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58e26b8bd5ba

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58e26b8bd5ba