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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Only notify pivot change if it acutally changed.

Comment 2

[Eitan Isaacson [:eeejay]]

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

Asking David for review since Alex is on vaycay.

Comment 3

[Trevor Saunders (:tbsaunde)]

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

># HG changeset patch
># User Eitan Isaacson <[e-mail]>
>
>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

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

How about NotifyIfPivotChanged?

Comment 5

[Eitan Isaacson [:eeejay]]

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

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

When you land it be sure to call it GoAheadMakeMyDay and have it return void* pointing to random memory.

Comment 7

[Trevor Saunders (:tbsaunde)]

(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

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 9

[Trevor Saunders (:tbsaunde)]

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

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: 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.

Comment 11

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a08eddc2c91e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a08eddc2c91e