The default bug view has changed. See this FAQ.

Fix build warnings in accessible

RESOLVED FIXED in mozilla20

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Usurelu Catalin, Assigned: Usurelu Catalin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 690627 [details]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.95 Safari/537.11

Steps to reproduce:

Fixed build warnings in /accessible/src/base/NotificationController.cpp
(Assignee)

Comment 1

4 years ago
Created attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

I'm a beginner developer and this is my first contribution to the Mozilla codebase.
Attachment #690627 - Attachment is obsolete: true
Attachment #690629 - Flags: review?(catalin.usurelu5)

Updated

4 years ago
Attachment #690629 - Flags: review?(catalin.usurelu5) → review?(surkov.alexander)
(Assignee)

Comment 2

4 years ago
Created attachment 690633 [details]
warnings

Warnings
Blocks: 389800
Component: General → Disability Access APIs
Comment on attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

> NotificationController::QueueEvent(AccEvent* aEvent)
> {
>-  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
>+  NS_ASSERTION((aEvent->mAccessible && aEvent->mAccessible->IsApplication()) ||
>                aEvent->GetDocAccessible() == mDocument,

I half way feel like telling gcc to just shutup about this one, what do you think Alex?

>-      for (uint32_t jdx = aThisIndex - 1; jdx < aThisIndex; jdx--) {
>+      for (uint32_t jdx = aThisIndex - 1; jdx < (uint32_t)aThisIndex; jdx--) {

I'd prefer that you made the argument unsigned, and fixed up the loop where its called, rather than adding a cast.

Comment 4

4 years ago
Comment on attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

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

canceling, I'd like to take a look at next round

::: accessible/src/base/NotificationController.cpp
@@ +317,5 @@
>  void
>  NotificationController::CoalesceEvents()
>  {
>    uint32_t numQueuedEvents = mEvents.Length();
> +  uint32_t tail = numQueuedEvents - 1;

I would add an assertion here that numQueuedEvents is not 0

@@ +521,5 @@
>  
>      // Do not emit any preceding selection events for same widget if they
>      // weren't coalesced yet.
>      if (aThisEvent->mEventType != nsIAccessibleEvent::EVENT_SELECTION_WITHIN) {
> +      for (uint32_t jdx = aThisIndex - 1; jdx < (uint32_t)aThisIndex; jdx--) {

please follow Trev's suggestion
Attachment #690629 - Flags: review?(surkov.alexander)

Updated

4 years ago
Assignee: nobody → catalin.usurelu5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 5

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >-  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
> >+  NS_ASSERTION((aEvent->mAccessible && aEvent->mAccessible->IsApplication()) ||
> >                aEvent->GetDocAccessible() == mDocument,
> 
> I half way feel like telling gcc to just shutup about this one, what do you
> think Alex?

extra parentheses are not necessary but it should be ok. At least we were warned that assignee and reviewer could miss a wrong logic.
(Assignee)

Comment 6

4 years ago
Created attachment 690778 [details] [diff] [review]
Added assertion, removed cast and changed argument(because i removed the cast)
Attachment #690778 - Flags: review?

Comment 7

4 years ago
Comment on attachment 690778 [details] [diff] [review]
Added assertion, removed cast and changed argument(because i removed the cast)

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

::: accessible/src/base/NotificationController.cpp
@@ +319,5 @@
>  {
>    uint32_t numQueuedEvents = mEvents.Length();
> +  NS_ASSERTION(numQueuedEvents != 0,
> +               "There should be at least one pending event");
> +  uint32_t tail = numQueuedEvents - 1;

sorry for saying this late but you don't need numQueuedEvents variable at all

@@ +509,5 @@
>  
>  void
>  NotificationController::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
>                                                  AccSelChangeEvent* aThisEvent,
> +                                                uint32_t aThisIndex)

pls fix a caller as well (int32_t 'for' loop)
(Assignee)

Comment 8

4 years ago
Created attachment 690878 [details] [diff] [review]
Added requested changes
Attachment #690878 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #690878 - Flags: review? → review?(surkov.alexander)

Comment 9

4 years ago
Comment on attachment 690878 [details] [diff] [review]
Added requested changes

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

r=me with comment fixed

::: accessible/src/base/NotificationController.cpp
@@ +318,5 @@
>  NotificationController::CoalesceEvents()
>  {
> +  NS_ASSERTION(mEvents.Length() != 0,
> +               "There should be at least one pending event");
> +  uint32_t tail = mEvents.Length()  - 1;

extra whitespace before '-'

@@ +394,5 @@
>  
>      case AccEvent::eCoalesceSelectionChange:
>      {
>        AccSelChangeEvent* tailSelChangeEvent = downcast_accEvent(tailEvent);
> +      for (uint32_t index = tail - 1; index >= 0; index--) {

index < tail, the idea here, when index is 0 then (index--) is 0xfff... which is always greater than mEvents.Length() - 1.
Attachment #690878 - Flags: review?(surkov.alexander) → review+
> @@ +394,5 @@
> >  
> >      case AccEvent::eCoalesceSelectionChange:
> >      {
> >        AccSelChangeEvent* tailSelChangeEvent = downcast_accEvent(tailEvent);
> > +      for (uint32_t index = tail - 1; index >= 0; index--) {
> 
> index < tail, the idea here, when index is 0 then (index--) is 0xfff...
> which is always greater than mEvents.Length() - 1.



but more importantly there is no such thing as a unsigned value < 0.
Comment on attachment 690878 [details] [diff] [review]
Added requested changes

>+  NS_ASSERTION(mEvents.Length() != 0,

nit, you don't need the != 0

Updated

4 years ago
Attachment #690778 - Attachment is obsolete: true
Attachment #690778 - Flags: review?

Updated

4 years ago
Attachment #690629 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 691288 [details] [diff] [review]
Fixed patch

I don't know how I missed the unsigned > 0 part. The code was already written that way and I didn't think something so easy could be bugged.
Attachment #690633 - Attachment is obsolete: true
Attachment #690878 - Attachment is obsolete: true
Attachment #691288 - Flags: review?(surkov.alexander)

Comment 13

4 years ago
Comment on attachment 691288 [details] [diff] [review]
Fixed patch

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

r=me, thank you
Attachment #691288 - Flags: review?(surkov.alexander) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9637f34d0df0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9637f34d0df0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 819664
You need to log in before you can comment on or make changes to this bug.