Don't notify pivot change if it has not actually changed

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 632454 [details] [diff] [review]
Only notify pivot change if it acutally changed.
Attachment #632454 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

5 years ago
Comment on attachment 632454 [details] [diff] [review]
Only notify pivot change if it acutally changed.

Asking David for review since Alex is on vaycay.
Attachment #632454 - Flags: review?(surkov.alexander) → review?(dbolter)
Attachment #632454 - Flags: review?(dbolter) → review+
Comment on attachment 632454 [details] [diff] [review]
Only notify pivot change if it acutally changed.

># HG changeset patch
># User Eitan Isaacson <eitan@monotonous.org>
>
>Only notify pivot change if it acutally changed.
>
>diff --git a/accessible/src/base/nsAccessiblePivot.cpp b/accessible/src/base/nsAccessiblePivot.cpp
>index 6342503..18d3912 100644
>--- a/accessible/src/base/nsAccessiblePivot.cpp
>+++ b/accessible/src/base/nsAccessiblePivot.cpp
>@@ -516,16 +516,19 @@ nsAccessiblePivot::SearchForward(Accessible* aAccessible,
> 
>   return nsnull;
> }
> 
> void
> nsAccessiblePivot::NotifyPivotChanged(Accessible* aOldPosition,
>                                       PRInt32 aOldStart, PRInt32 aOldEnd)
> {

if it doesn't always notify then it should be renamed to MaybeNotifyOfPivotChange()

>+  if (aOldPosition == mPosition &&
>+      aOldStart == mStartOffset && aOldEnd == mEndOffset)
>+    return;
>   nsTObserverArray<nsCOMPtr<nsIAccessiblePivotObserver> 

nit, blank line after if
How about NotifyIfPivotChanged?
(Assignee)

Comment 5

5 years ago
(In reply to David Bolter [:davidb] from comment #4)
> How about NotifyIfPivotChanged?

Ok, you guys provoked me. I'm going to make that method return boolean, and have other methods that return true on move return it directly.
When you land it be sure to call it GoAheadMakeMyDay and have it return void* pointing to random memory.
(In reply to David Bolter [:davidb] from comment #4)
> How about NotifyIfPivotChanged?

I could live with that, but fwiw I think we've used MaybeFoo() for similar things in the past
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to David Bolter [:davidb] from comment #4)
> > How about NotifyIfPivotChanged?
> 
> I could live with that, but fwiw I think we've used MaybeFoo() for similar
> things in the past

True.
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> (In reply to David Bolter [:davidb] from comment #4)
> > How about NotifyIfPivotChanged?
> 
> Ok, you guys provoked me. I'm going to make that method return boolean, and
> have other methods that return true on move return it directly.

Then I think you can get rid of the checks before calling MoveInternal() too I think.
(Assignee)

Comment 10

5 years ago
Created attachment 633623 [details] [diff] [review]
Only notify pivot change if it acutally changed.

Here.

NotifyOfPivotChange(), it is obviously superior(*) to ifs maybes and buts. The subtle grammar implicitly implies that we notify of a pivot change. If it hasn't changed we don't notify.

Making it return boolean unifies the notification with the behavior of methods that return true upon a move.

* It may not be superior.
Attachment #632454 - Attachment is obsolete: true
Attachment #633623 - Flags: review?(dbolter)
Attachment #633623 - Flags: review?(dbolter) → review+
(Assignee)

Comment 11

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a08eddc2c91e
Assignee: nobody → eitan
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/a08eddc2c91e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.