Last Comment Bug 764203 - Don't notify pivot change if it has not actually changed
: Don't notify pivot change if it has not actually changed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 16:02 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-06-19 01:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Only notify pivot change if it acutally changed. (967 bytes, patch)
2012-06-12 16:03 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Only notify pivot change if it acutally changed. (7.62 KB, patch)
2012-06-15 12:14 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-06-12 16:02:21 PDT

    
Comment 1 Eitan Isaacson [:eeejay] 2012-06-12 16:03:58 PDT
Created attachment 632454 [details] [diff] [review]
Only notify pivot change if it acutally changed.
Comment 2 Eitan Isaacson [:eeejay] 2012-06-15 09:51:08 PDT
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) 2012-06-15 10:07:07 PDT
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
Comment 4 David Bolter [:davidb] 2012-06-15 10:22:05 PDT
How about NotifyIfPivotChanged?
Comment 5 Eitan Isaacson [:eeejay] 2012-06-15 10:24:46 PDT
(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] 2012-06-15 10:32:10 PDT
When you land it be sure to call it GoAheadMakeMyDay and have it return void* pointing to random memory.
Comment 7 Trevor Saunders (:tbsaunde) 2012-06-15 10:34:22 PDT
(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] 2012-06-15 10:35:20 PDT
(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) 2012-06-15 11:55:30 PDT
(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] 2012-06-15 12:14:50 PDT
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.
Comment 12 Ed Morley [:emorley] 2012-06-19 01:18:54 PDT
https://hg.mozilla.org/mozilla-central/rev/a08eddc2c91e

Note You need to log in before you can comment on or make changes to this bug.