764367 - Fix build warnings in accessible/

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Trevor Saunders (:tbsaunde)]

I have concerns about this, but I think you should work something out with jdm for bug 755048 first since your patch seems to have some of the same changes as parts of that patch that I'm absolutely willing to take.

note surkov is around for reviews, but I think he's trying to vacation (as am I sort of)

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 632674 [details] [diff] [review]
Patch v1

sorry I noticed request so late (I'm away from work these days) so redirecting request to Trevor

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 632674 [details] [diff] [review]
Patch v1

>+FAIL_ON_WARNINGS = 1

if it were just me I'd say just no.  I don't believe its worth the time it'll cost people working on windows.  I think its a lot sainer to just make sure the warnings are somewhat under control when we don't have well known issues hurting real people.

  That said I guess if Alex feels strongly I'll let him try and convince me :)

ia2Role) \
>-  MOZ_STATIC_ASSERT(roles::geckoRole == nsIAccessibleRole::ROLE_ ## geckoRole, "internal and xpcom roles differ!");
>+  MOZ_STATIC_ASSERT(roles::geckoRole == roles::Role(nsIAccessibleRole::ROLE_ ## geckoRole), "internal and xpcom roles differ!");

ugh, that doesn't really feel right although it seems like it should probably do the right thing.  I'd be tempted to cast one side to int, but that might be weird if the compiler decides enums are unsigned.  Mostly I just don't want to do enough lawyering about the spec to decide if this is correct.  If these warnings actually bother someone I might be convinced to take a #pragma here to punt on what is correct and go on with life.

> nsEventShell::FireEvent(AccEvent* aEvent)
> {
>   if (!aEvent)
>     return;
> 
>   Accessible* accessible = aEvent->GetAccessible();
>-  NS_ENSURE_TRUE(accessible,);
>+  if (!accessible) {
>+    return;
>+  }

nit, no { }

note I don't think that should ever happen so I'd prefer filing a bug to just remove the check altogether.

> nsEventShell::FireEvent(PRUint32 aEventType, Accessible* aAccessible,
>                         EIsFromUserInput aIsFromUserInput)
> {
>-  NS_ENSURE_TRUE(aAccessible,);
>+  if (!aAccessible) {
>+    return;
>+  }

nit, no braces

>+    DebugOnly<nsresult> rv = SetARIASelected(row, false);

I'm fairly tempted to think this is just papering over the fact that function should return void.

>   for (PRUint32 idx = aIndex + 1; idx < mChildren.Length(); idx++) {
>-    NS_ASSERTION(mChildren[idx]->mIndexInParent == idx - 1, "Accessible child index doesn't match");
>+    NS_ASSERTION(mChildren[idx]->mIndexInParent == PRInt32(idx) - 1, "Accessible child index doesn't match");

an acessible shouldn't have an index of -1 when its in the array of kids, so you could just assert idx > 0 and then cast the result of IndexInParent() to unsigned.

>   for (PRUint32 idx = index + 1; idx < mChildren.Length(); idx++) {
>-    NS_ASSERTION(mChildren[idx]->mIndexInParent == idx, "Accessible child index doesn't match");
>+    NS_ASSERTION(mChildren[idx]->mIndexInParent == PRInt32(idx), "Accessible child index doesn't match");

same here

>   Accessible* child = nsnull;
>-  while ((child = walker.NextChild()) && AppendChild(child));
>+  while ((child = walker.NextChild()) && AppendChild(child)) {
>+    continue;
>+  }

that's really silly looking.  I'd probably just ignore the compiler here, but if you insist I guess I might take continue without the { }

> class ApplicationAccessible : public AccessibleWrap,
>                              public nsIAccessibleApplication
> {
> public:
>+  using Accessible::GroupPosition;

so, both versions of GroupPosition() are defined on Accessible, so I have no clue which this pulls in.  I think the right solution is to [binaryname] the xpcom to something else.

>   nsCOMPtr<nsISelectionPrivate> privSel(do_QueryInterface(domSel));
>   nsresult rv = privSel->
>     GetRangesForIntervalArray(startNode, 0, startNode, childCount, true, aRanges);
>-  NS_ENSURE_SUCCESS(rv,);
>+  if (NS_FAILED(rv)) {
>+    return;
>+  }

I believe this code is obsolete anyway, and if it isn't I should get around to making it so, but nit, no { }

>--- a/accessible/src/html/HTMLFormControlAccessible.cpp
>+++ b/accessible/src/html/HTMLFormControlAccessible.cpp
>@@ -169,17 +169,19 @@ HTMLRadioButtonAccessible::GetPositionAn
>   nsRefPtr<nsContentList> inputElms;
> 
>   nsCOMPtr<nsIFormControl> formControlNode(do_QueryInterface(mContent));
>   dom::Element* formElm = formControlNode->GetFormElement();
>   if (formElm)>     inputElms = NS_GetContentList(formElm, namespaceId, tagName);
>   else
>     inputElms = NS_GetContentList(mContent->OwnerDoc(), namespaceId, tagName);
>-  NS_ENSURE_TRUE(inputElms, );
>+  if (!inputElms) {
>+    return;
>+  }

it seems NS_GetContentList() will never return null, so I'd propose just removing the check unless someone sees a good reason to allow that flexibility.

>   bool isSelected;
>   PRInt32 columnExtent = 0;
> 
>-  nsresult rv = tableLayout->
>+  tableLayout->

I guess that works, but killing nsITableLayout or atleast this method seems safer than hoping nobody makes it failable.

>     { return xpcAccessibleTable::UnselectRow(aRowIdx); } \
>   NS_IMETHOD IsProbablyForLayout(bool* aResult) \
>   { return xpcAccessibleTable::IsProbablyForLayout(aResult); } \
>+  using TableAccessible::IsRowSelected; \
>+  using TableAccessible::IsCellSelected; \
>+  using TableAccessible::SelectRow;

this might be ok for now, but making this a seperate object is the right fix.

> void
> XULColorPickerAccessible::CacheChildren()
> {
>-  NS_ENSURE_TRUE(mDoc,);
>+  if (!mDoc) {
>+    return;
>+  }

no idea why its there it shouldn't ever happen just remove it.

>-  NS_ENSURE_TRUE(mDoc,);
>-  if (!isMenu && !isMenuButton)
>+  if (!mDoc || (!isMenu && !isMenuButton))
>     return;
here too I believe.

>--- a/dom/interfaces/xul/nsIDOMXULMultSelectCntrlEl.idl
>+++ b/dom/interfaces/xul/nsIDOMXULMultSelectCntrlEl.idl
>@@ -3,16 +3,20 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsIDOMXULSelectCntrlEl.idl"
> 
> [scriptable, uuid(E3CD8269-9502-4E70-910C-0D6D71B22253)]
> interface nsIDOMXULMultiSelectControlElement : nsIDOMXULSelectControlElement
> {
>+%{C++
>+  using nsIDOMXULSelectControlElement::GetSelectedItem;
>+%}

make someone else review that, I have no idea if its correct.

Comment 4

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 632674 [details] [diff] [review]
> Patch v1
> 
> >+FAIL_ON_WARNINGS = 1
> 
> if it were just me I'd say just no.  I don't believe its worth the time
> it'll cost people working on windows.  I think its a lot sainer to just make
> sure the warnings are somewhat under control when we don't have well known
> issues hurting real people.
> 

For as much as I hate warnings, I have to second Trevor's argument. Also each time I upgrade gcc or gtk I get new warnings.
No -Werror.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > Comment on attachment 632674 [details] [diff] [review]
> > Patch v1
> > 
> > >+FAIL_ON_WARNINGS = 1
> > 
> > if it were just me I'd say just no.  I don't believe its worth the time
> > it'll cost people working on windows.  I think its a lot sainer to just make
> > sure the warnings are somewhat under control when we don't have well known
> > issues hurting real people.
> > 
> 
> For as much as I hate warnings, I have to second Trevor's argument. Also
> each time I upgrade gcc or gtk I get new warnings.

afaik it tries to be selective about which warnings and compilers.

However you do raise a concern with the atk headers which we import, should upstream decidie to add something that gives a warning we'll have some grief.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> >   bool isSelected;
> >   PRInt32 columnExtent = 0;
> > 
> >-  nsresult rv = tableLayout->
> >+  tableLayout->
> 
> I guess that works, but killing nsITableLayout or atleast this method seems
> safer than hoping nobody makes it failable.

FWIW, ehsan made this change without bothering with reviews:

https://hg.mozilla.org/mozilla-central/rev/9f1844790612

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 644954 [details] [diff] [review]
Patch v3

 
I'd r+ this without the fail on warnings bit.

> topsrcdir = @top_srcdir@
> srcdir    = @srcdir@
> VPATH   = @srcdir@
>+FAIL_ON_WARNINGS := 1

I thought I made it pretty clear I wouldn't r+ that last time?

> 
>-////////////////////////////////////////////////////////////////////////////////
>-// AutoGeneratedTextAttr
>-////////////////////////////////////////////////////////////////////////////////
>+////////////////////////////////////////////////////////////////////////////////
>+// AutoGeneratedTextAttr
>+////////////////////////////////////////////////////////////////////////////////

seems you change nothing here, but whatever/

> TextAttrsMgr::AutoGeneratedTextAttr::
>   AutoGeneratedTextAttr(HyperTextAccessible* aHyperTextAcc,
>                         Accessible* aAccessible) :
>-  TTextAttr<bool>(!aAccessible)
>+  TTextAttr<bool>(!aAccessible)

same here

> {
>   mRootNativeValue = false;
>   mIsRootDefined = false;
> 
>   if (aAccessible)
>     mIsDefined = mNativeValue = (aAccessible->NativeRole() == roles::STATICTEXT);
> }
> 
>@@ -747,20 +747,20 @@ TextAttrsMgr::TextPosTextAttr::
>       float percentValue = styleCoord.GetPercentValue();
>       return percentValue > 0 ?
>         eTextPosSuper :
>         (percentValue < 0 ? eTextPosSub : eTextPosBaseline);
>     }
> 
>     case eStyleUnit_Coord:
>     {
>-       nscoord coordValue = styleCoord.GetCoordValue();
>-       return coordValue > 0 ?
>-         eTextPosSuper :
>-         (coordValue < 0 ? eTextPosSub : eTextPosBaseline);
>+      nscoord coordValue = styleCoord.GetCoordValue();
>+      return coordValue > 0
>+             ? eTextPosSuper
>+             : (coordValue < 0 ? eTextPosSub : eTextPosBaseline);

nit, iirc Alex prefers operator at end of line.

>   Accessible* accessible = aEvent->GetAccessible();
>-  NS_ENSURE_TRUE(accessible,);
>+  if (!accessible)
>+    return;

nit, that should really never happen please add an assert.

> void
> nsEventShell::FireEvent(PRUint32 aEventType, Accessible* aAccessible,
>                         EIsFromUserInput aIsFromUserInput)
> {
>-  NS_ENSURE_TRUE(aAccessible,);
>+  if (!aAccessible)
>+    return;

same

>diff --git a/accessible/src/html/HTMLFormControlAccessible.cpp b/accessible/src/html/HTMLFormControlAccessible.cpp
>--- a/accessible/src/html/HTMLFormControlAccessible.cpp
>+++ b/accessible/src/html/HTMLFormControlAccessible.cpp
>@@ -168,17 +168,17 @@ HTMLRadioButtonAccessible::GetPositionAn
>   nsRefPtr<nsContentList> inputElms;
> 
>   nsCOMPtr<nsIFormControl> formControlNode(do_QueryInterface(mContent));
>   dom::Element* formElm = formControlNode->GetFormElement();
>   if (formElm)
>     inputElms = NS_GetContentList(formElm, namespaceId, tagName);
>   else
>     inputElms = NS_GetContentList(mContent->OwnerDoc(), namespaceId, tagName);
>-  NS_ENSURE_TRUE(inputElms, );
>+  MOZ_ASSERT(inputElms);

nit, please don't use fatal asserts for that, though I guess it'll crash shortley any way.

> void
> XULColorPickerAccessible::CacheChildren()
> {
>-  NS_ENSURE_TRUE(mDoc,);
>+  if (!mDoc)
>+    return;

that shouldn't ever happen, please either remove the check or file a bug.

>+++ b/accessible/src/xul/XULFormControlAccessible.cpp
>@@ -177,18 +177,17 @@ XULButtonAccessible::CacheChildren()
>                                        nsGkAtoms::menu,
>                                        eCaseMatters);
> 
>   bool isMenuButton = isMenu ?
>     false :
>     mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
>                           nsGkAtoms::menuButton, eCaseMatters);
> 
>-  NS_ENSURE_TRUE(mDoc,);
>-  if (!isMenu && !isMenuButton)
>+  if (!mDoc || (!isMenu && !isMenuButton))
>     return;

same here

> XULTextFieldAccessible::CacheChildren()
> {
>-  NS_ENSURE_TRUE(mDoc,);
>+  if (!mDoc)
>+    return;

same

>-  return selectedrowCount == RowCount();
>+  return selectedrowCount == PRInt32(RowCount());

nit, casting the other way may more sense.
>-  return selectedRowCount > 0 && selectedRowCount == RowCount() ? ColCount() : 0;
>+  return selectedRowCount > 0 && selectedRowCount == PRInt32(RowCount()) ? ColCount() : 0;

and here

> 
> class XULTreeItemAccessibleBase : public AccessibleWrap
> {
> public:
>   using Accessible::GetParent;
>+  using Accessible::GroupPosition;

why is this needed when you do the binaryname stuff?

>diff --git a/accessible/src/xul/XULTreeGridAccessible.cpp b/accessible/src/xul/XULTreeGridAccessible.cpp
>--- a/accessible/src/xul/XULTreeGridAccessible.cpp
>+++ b/accessible/src/xul/XULTreeGridAccessible.cpp
>@@ -67,17 +67,17 @@ XULTreeGridAccessible::SelectedColCount(
> {
>   // If all the row has been selected, then all the columns are selected,
>   // because we can't select a column alone.
> 
>   PRInt32 selectedRowCount = 0;
>   nsresult rv = GetSelectionCount(&selectedRowCount);
>   NS_ENSURE_SUCCESS(rv, 0);
> 
>-  return selectedRowCount > 0 && selectedRowCount == RowCount() ? ColCount() : 0;
>+  return selectedRowCount > 0 && selectedRowCount == PRInt32(RowCount()) ? ColCount() : 0;

nit, cast to unsigned, since the first half half the check should mean it will never be negative.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: part 1

I just pulled this out of patch 3.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Created attachment 686689 [details] [diff] [review]
> part 1

I'm confused by this patch (as contrasted to the patch in bug 814671)

The problem that both patches address is: we have two C++ methods functions named "GroupPosition()", and their names clash.

The patch here and the patch on bug 814671 are basically equivalent, in that they each rename one of those functions -- they just picked opposite functions to rename.
  * Bug 814671's patch renames the internal-only function (following a well-established practice, per bug 814671 comment 5)
  * The patch on this bug here renames the public one, and adds a "binaryname()" annotation to the IDL so that we can still find it.  (This leaves us in a more-confusing situation, IMHO -- someone looking for the C++ impl of the javascript-exposed method has to read the IDL file carefully to know what to look for, or else they'll end up finding the helper-method and perhaps being confused about why its method-signature differs from the JS one)

I've never seen "binaryname()" used in IDL before.  I think it complicates/confuses things, and it shouldn't be used gratuitously -- we should only use it when there's a very good reason.  And I don't think "we have an internal helper-method, used as a helper for the scriptable method, and that internal method that happened to pick the same name" is a good enough reason.

Comment 13

[Trevor Saunders (:tbsaunde)]

> The problem that both patches address is: we have two C++ methods functions
> named "GroupPosition()", and their names clash.
> 
> The patch here and the patch on bug 814671 are basically equivalent, in that
> they each rename one of those functions -- they just picked opposite
> functions to rename.
>   * Bug 814671's patch renames the internal-only function (following a
> well-established practice, per bug 814671 comment 5)
>   * The patch on this bug here renames the public one, and adds a
> "binaryname()" annotation to the IDL so that we can still find it.  (This
> leaves us in a more-confusing situation, IMHO -- someone looking for the C++
> impl of the javascript-exposed method has to read the IDL file carefully to
> know what to look for, or else they'll end up finding the helper-method and
> perhaps being confused about why its method-signature differs from the JS
> one)

I wouldn't really describe the one that stays GroupPosition() to be internal only infact I'd probably r- a patch adding C++ uses of the xpcom one  that don't come with a very good reason.

> I've never seen "binaryname()" used in IDL before.  I think it

we have some uses in the tree but I forget where

> complicates/confuses things, and it shouldn't be used gratuitously -- we
> should only use it when there's a very good reason.  And I don't think "we
> have an internal helper-method, used as a helper for the scriptable method,
> and that internal method that happened to pick the same name" is a good
> enough reon.

again its not really accurate to describe it as internal imho it's actually the prefered interface for C++.  So I think it is pretty confusing to rename the thing we want C++ to use to FooInternal() when the internal means C++ or maybe libxul only.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> I wouldn't really describe the one that stays GroupPosition() to be internal
> only
[...]
> again its not really accurate to describe it as internal imho 

I meant that it was "internal" in the sense that it's currently only invoked as a helper-method for other methods on the same class (& on one subclass "AccessibleWrap").

So we're using it as an internal class method, at least right now. (One could mark it 'protected' and we'd still compile just fine.)

> So I think it is pretty confusing to rename
> the thing we want C++ to use to FooInternal()

If this were a method that other C++ code was calling & relying on, then I'd agree.  But as noted above, it's really behaving as an internal helper method right now. (Though if we were to do the Internal-renaming, I'd also think we'd want to mark it as protected to make that more explicit.  I agree that a public class named "Internal" would be confusing.)

Anyway, it doesn't matter much, and I'm probably just bikeshedding at this point, for which I apologize. :)  The decision doesn't really affect me, & you/surkov get the final say of course, so I'll defer to you guys.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Created attachment 686689 [details] [diff] [review]
> part 1
> 
> I just pulled this out of patch 3.

If this is preferred solution of the Accessibility owner/peers, can someone land it?

Comment 16

[alexander :surkov (:asurkov)]

We want to get rid a warning only, right? Why 'using' approach doesn't seem right in this case?