Closed
Bug 820197
Opened 12 years ago
Closed 12 years ago
Fix build warnings in accessible
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: catalin.usurelu5, Assigned: catalin.usurelu5)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
3.97 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
Attachment #690629 -
Flags: review?(catalin.usurelu5) → review?(surkov.alexander)
Assignee | ||
Comment 2•12 years ago
|
||
Warnings
Comment 3•12 years ago
|
||
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•12 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•12 years ago
|
Assignee: nobody → catalin.usurelu5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 5•12 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•12 years ago
|
||
Attachment #690778 -
Flags: review?
Comment 7•12 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•12 years ago
|
||
Attachment #690878 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #690878 -
Flags: review? → review?(surkov.alexander)
Comment 9•12 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+
Comment 10•12 years ago
|
||
> @@ +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 11•12 years ago
|
||
Comment on attachment 690878 [details] [diff] [review]
Added requested changes
>+ NS_ASSERTION(mEvents.Length() != 0,
nit, you don't need the != 0
Updated•12 years ago
|
Attachment #690778 -
Attachment is obsolete: true
Attachment #690778 -
Flags: review?
Updated•12 years ago
|
Attachment #690629 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
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•12 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•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Keywords: checkin-needed
Comment 15•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•