Closed Bug 443212 Opened 16 years ago Closed 16 years ago

Kinetic scrolling

Categories

(Firefox for Android Graveyard :: General, enhancement, P2)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
Other
Maemo
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
fennec1.0m7

People

(Reporter: christian.bugzilla, Assigned: blassey)

References

Details

Attachments

(1 file, 4 obsolete files)

updated based on mfinkle's comments
11.18 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
Flags: blocking-fennec1.0+
Priority: -- → P2
Assignee: nobody → blassey
Attached patch WIP patch (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
I realized the interval wasn't necessary, so got rid of it.  This is a big smoother

        Attachment #330685 -
        Attachment is obsolete: true

    
  
Target Milestone: Fennec M6 → Fennec M7

    
    

    
  

      
      
      
      

        Attached patch
          
          
        using canvas for checkerboard (obsolete)
        — 
          — Splinter Review
      

    


  
for some reason there is a big performance hit when a background image is set on the stack.  This uses a second canvas to show the scrolling checkerboard, which seems a like overkill to me.

Even still, the performance isn't as good as I think it needs to be.  The refresh rate is too slow when the "browser" canvas isn't visible because the regular pattern can appear to be moving in the wrong direction ( or way two slow or not at all).

Also, at a minimum its set to need at least two mouse moves.  Often 
even this low bar ignores flicks.

        Attachment #330689 -
        Attachment is obsolete: true

    
    

    
  
why do we need a checkerboard for kinetic scrolling?  can we split that out and file a bug on "moving stuff sucks when there is a background on a stack" (or something more descriptive with a testcase?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        sans checkerboard stuff (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #333379 -
        Attachment is obsolete: true
Depends on: 450297

    
    

    
  
Comment on attachment 333443 [details] [diff] [review]
sans checkerboard stuff

Drive by review

>     <implementation implements="nsIObserver">
>       <constructor>
>+	
>+          <![CDATA[

<constructor><![CDATA[

>         this._dragStartTimeout = -1;
>+
>+        this.PAN_EVENTS_TO_TRACK = 2;
>+        this._panEventTracker = new Array(this.PAN_EVENTS_TO_TRACK);
>+        this._panEventTrackerPtr = 0;

Can we rename this to _panEventTrackerIndex? the "Ptr" reminds me of pointers.

>+
>+       ]]>

line this up with the </constructor> and make it look like other bodies: ]]></constructor>

>-          oldPageY: 0
>+          oldPageY: 0,
>+	  velocityX: 0,
>+	  velocityY: 0,
>+	  originalX: 0,
>+	  originalY: 0,
>+	  destinationX: 0,
>+	  destinationY: 0

TABs in here, throwing the spacing off

>+
>+      <field name="_bgcanvas">
>+        document.getAnonymousElementByAttribute(this, "anonid", "bgcanvas");
>       </field>

This doesn't appear to be used in this patch. Needed for checkerboard patch?

> 
>+      <method name="_startKinetic">
>+        <body><![CDATA[
>+           //JProfStartProfiling();
>+           var p2 = this._panEventTracker[this._panEventTrackerPtr];
>+           var p1 = this._panEventTracker[(this._panEventTrackerPtr+1)%this.PAN_EVENTS_TO_TRACK];

Spaces around the "%"

>+               let destX = Math.min( this.dragData.pageX - this.dragData.dragX + this.dragData.velocityX *2000,  contentAreaWidth-canvasWidth);
>+               let destY = Math.min( this.dragData.pageY - this.dragData.dragY + this.dragData.velocityY *2000,  contentAreaHeight-canvasHeight);

Drop the extra space after "Math.min( "
Add spaces around last "-" operator

>+               this.dragData.destinationX =  -destX + this.dragData.pageX;
>+               this.dragData.destinationY =  -destY + this.dragData.pageY  ;

Remove extra spaces after "=" and " ;"

>+               this.dragData.kineticId = window.setInterval(this._doKinetic, 50, this, dt/(this.PAN_EVENTS_TO_TRACK-1));

Add spaces around "/" and "-" operators

50ms is almost as fast as we can refresh the canvas. Might want to play with that value.

>+             this._endPan()
>+           }
>+      
>+        ]]></body>

Remove blank line

>+
>+      <method name="_doKinetic">
>+        <parameter name="self"/>
>+        <parameter name="dt"/>
>+        <body><![CDATA[
>+        let startX = self.dragData.dragX;
>+        let startY = self.dragData.dragY;

Indent the code in this body 2 spaces

>+        if (Math.abs(self.dragData.destinationX - self.dragData.dragX) < 3) {
>+           self.dragData.velocityX = 0;
>+        }
>+        if (Math.abs(self.dragData.destinationY - self.dragData.dragY) < 3) {
>+           self.dragData.velocityY = 0;
>+        }

What's the "3"? Maybe use a constant and comment what effect it has.

>+        let dx = self.dragData.originalX == self.dragData.destinationX?0:
>+                 self.dragData.velocityX*dt*(Math.sqrt(Math.abs(self.dragData.destinationX - self.dragData.dragX))/
>+                                             Math.sqrt(Math.abs(self.dragData.destinationX - self.dragData.originalX)));
>+        let dy = self.dragData.originalY == self.dragData.destinationY?0:
>+                 self.dragData.velocityY*dt*(Math.sqrt(Math.abs(self.dragData.destinationY - self.dragData.dragY))/
>+                                             Math.sqrt(Math.abs(self.dragData.destinationY - self.dragData.originalY)));

Fix spacing around operators

>+           
>+        if((self.dragData.originalX < self.dragData.destinationX && 
>+            self.dragData.originalX < self.dragData.dragX -dx&&
>+            self.dragData.dragX-dx < self.dragData.destinationX ) || 
>+            (self.dragData.originalX > self.dragData.destinationX && 
>+            self.dragData.originalX > self.dragData.dragX -dx&&
>+            self.dragData.dragX-dx > self.dragData.destinationX ))
>+           self.dragData.dragX-=dx;
>+        else
>+           self.dragData.dragX = self.dragData.destinationX;
>+        if((self.dragData.originalY < self.dragData.destinationY && 
>+            self.dragData.originalY < self.dragData.dragY-dy &&
>+            self.dragData.dragY-dy < self.dragData.destinationY ) || 
>+            (self.dragData.originalY > self.dragData.destinationY && 
>+            self.dragData.originalY > self.dragData.dragY -dy&&
>+            self.dragData.dragY-dy > self.dragData.destinationY ))
>+           self.dragData.dragY-=dy;
>+        else
>+           self.dragData.dragY = self.dragData.destinationY;

Holy if-statement, Batman. Thems some amazing if conditions. Operator spacing needs some work. Comments about what the tests are doing might be helpful. Also, would it be more performant to order the conditions so the most likely conditions are checked first, so the logic can short circuit and we don't need to check all conditions? I don't know what the "right" order is, just an FYI.

>+
>+        self._updateCanvasPosition();

I wonder if we are trying to update the canvas too often. This method is called every 50ms, which might be too fast for every frame. Perhaps we could try to throttle this method a bit.

>+
>+        if ( ((startX - self.dragData.dragX)/(self.dragData.destinationX - self.dragData.originalX)  < 0 &&  (startY - self.dragData.dragY)/ (self.dragData.destinationY - self.dragData.originalY) < 0) || ( Math.abs(startX - self.dragData.dragX) < 4 && Math.abs(startY - self.dragData.dragY) < 4 )) {
>+        

Remove blank line and space after first paren of the "if". A comment would be helpful here too.

Perhaps putting "startX - self.dragData.dragX" and "startY - self.dragData.dragY" in local variables would allow you to reuse them and reduce clutter too.

>+           self._endKinetic();
>+        }
>+        ]]></body>
>+      </method>
>+
>+      <method name="_endKinetic">
>+        <body><![CDATA[

Spacing is off in this body. Should be 2 spaces.

>+          
>+           window.clearInterval(this.dragData.kineticId);
>+           this.dragData.kineticId = 0;
>+
>+            // dragX/dragY are garanteed to be within the correct bounds, so just
>+            // update pageX/pageY directly.
>+            this.dragData.pageX -= this.dragData.dragX / this._zoomLevel;
>+            this.dragData.pageY -= this.dragData.dragY / this._zoomLevel;
>+
>+            // relocate the canvas to 0x0 in the window
>+            this.dragData.dragX = 0;
>+            this.dragData.dragY = 0;
>+
>+            // update canvas position and draw the canvas at the new location
>+            this._browserToCanvas();
>+            this._updateCanvasPosition();
>+
>+            this.dragData.dragging = false;
>+            //JProfStopProfiling()
>+        ]]></body>
>+      </method>
>+
>       <!-- Pans directly to a given X/Y (in page coordinates) -->
>       <method name="_panTo">
>         <parameter name="aX"/>
>         <parameter name="aY"/>
>         <body><![CDATA[
>           var [deltaX, deltaY] = this._constrainPanCoords(aX - this.dragData.pageX,
>                                                           aY - this.dragData.pageY);
>           this.dragData.pageX += deltaX;
>@@ -817,21 +939,27 @@
>             //this.deckbrowser._updateCanvasPosition();
> 
>             var self = this.deckbrowser;
>             this.deckbrowser._dragStartTimeout = setTimeout(function () {
>               self._dragStartTimer();
>             }, 200);
> 
>             this._lastMouseDown = aEvent;
>+            for (var i =0; i < self.PAN_EVENTS_TO_TRACK ; i++) {
>+              self._panEventTracker[i] = null;
>+            }

Should you reset the _panEventTrackerPtr here too?

>+            if(self.dragData.kineticId)
Spacing on "if"

>               this._lastMouseUp = aEvent;
>             }
>+            
>+             this.deckbrowser._startKinetic();

bad spacing on this line ^

>+            for (var i =0; i < this.deckbrowser.PAN_EVENTS_TO_TRACK ; i++) {
>+              this.deckbrowser._panEventTracker[i] = null;
>+            }
>+

Remove blank line

>+            let now = Date.now();
>+            if(this.deckbrowser.dragData.dragging) {
>+              this.deckbrowser._panEventTrackerPtr = (this.deckbrowser._panEventTrackerPtr+1)%this.deckbrowser.PAN_EVENTS_TO_TRACK;

Spacing around the "if" and the operators.

    
    

    
  
> >+        this._panEventTrackerPtr = 0;
> 
> Can we rename this to _panEventTrackerIndex? the "Ptr" reminds me of pointers.

_panEventTrackerIndex it is

> >+
> >+      <field name="_bgcanvas">
> >+        document.getAnonymousElementByAttribute(this, "anonid", "bgcanvas");
> >       </field>
> 
> This doesn't appear to be used in this patch. Needed for checkerboard patch?

yup, moved over to other patch



> 
> >+               this.dragData.kineticId = window.setInterval(this._doKinetic, 50, this, dt/(this.PAN_EVENTS_TO_TRACK-1));
> 
> Add spaces around "/" and "-" operators
> 
> 50ms is almost as fast as we can refresh the canvas. Might want to play with
> that value.
 
actually, it should be dt / (this.PAN_EVENTS_TO_TRACK - 1), the thinking being we'll update during the kinetic pan at the same (average) rate that we did during the manual pan.  It now reads:

this.dragData.kineticId = window.setInterval(this._doKinetic, dt / (this.PAN_EVENTS_TO_TRACK - 1), this, dt / (this.PAN_EVENTS_TO_TRACK - 1));

but, yea..... this is something we should play with.

> >+             this._endPan()
> >+           }
> >+      
> >+        ]]></body>
> 
> Remove blank line
> 
> >+
> >+      <method name="_doKinetic">
> >+        <parameter name="self"/>
> >+        <parameter name="dt"/>
> >+        <body><![CDATA[
> >+        let startX = self.dragData.dragX;
> >+        let startY = self.dragData.dragY;
> 
> Indent the code in this body 2 spaces
> 
> >+        if (Math.abs(self.dragData.destinationX - self.dragData.dragX) < 3) {
> >+           self.dragData.velocityX = 0;
> >+        }
> >+        if (Math.abs(self.dragData.destinationY - self.dragData.dragY) < 3) {
> >+           self.dragData.velocityY = 0;
> >+        }
> 
> What's the "3"? Maybe use a constant and comment what effect it has.

added:  
const stopTheshold = 3;  //Distance from the destination point that we'll consider to be "there"

the purpose of this is once we get close to the target it can still take a fairly long time to get *exactly* to it, since we're decelerating.  This is especially bad since we don't repaint the canvas until the pan is over.  After thinking about it though, the velocity should be set to 1/dt rather than 0, so we "snap" to the dest rather than stopping 3 pixels from it.
          
> 
> >+        let dx = self.dragData.originalX == self.dragData.destinationX?0:
> >+                 self.dragData.velocityX*dt*(Math.sqrt(Math.abs(self.dragData.destinationX - self.dragData.dragX))/
> >+                                             
> 
> >+           
> >+        if((self.dragData.originalX < self.dragData.destinationX && 
> >+            self.dragData.originalX < self.dragData.dragX -dx&&
> >+            self.dragData.dragX-dx < self.dragData.destinationX ) || 
> >+            (self.dragData.originalX > self.dragData.destinationX && 
> >+            self.dragData.originalX > self.dragData.dragX -dx&&
> >+            self.dragData.dragX-dx > self.dragData.destinationX ))
> >+           self.dragData.dragX-=dx;
> >+        else
> >+           self.dragData.dragX = self.dragData.destinationX;
> >+        if((self.dragData.originalY < self.dragData.destinationY && 
> >+            self.dragData.originalY < self.dragData.dragY-dy &&
> >+            self.dragData.dragY-dy < self.dragData.destinationY ) || 
> >+            (self.dragData.originalY > self.dragData.destinationY && 
> >+            self.dragData.originalY > self.dragData.dragY -dy&&
> >+            self.dragData.dragY-dy > self.dragData.destinationY ))
> >+           self.dragData.dragY-=dy;
> >+        else
> >+           self.dragData.dragY = self.dragData.destinationY;
> 
> Holy if-statement, Batman. Thems some amazing if conditions. Operator spacing
> needs some work. Comments about what the tests are doing might be helpful.
> Also, would it be more performant to order the conditions so the most likely
> conditions are checked first, so the logic can short circuit and we don't need
> to check all conditions? I don't know what the "right" order is, just an FYI

I *think* the first condition of each of the sub conditions can be dropped because they're implied.  (if x < y && y < z => x < z), so I've dropped them, which combined with a local variable makes it more readable.

In terms of order, the question is if we're panning up/left or down/right more often. I've rearranged them to be biased towards down/right 
.
> 
> >+
> >+        self._updateCanvasPosition();
> 
> I wonder if we are trying to update the canvas too often. This method is called
> every 50ms, which might be too fast for every frame. Perhaps we could try to
> throttle this method a bit.

now its being called at the same rate at which we were processing mouse moves.  This is just another guess, but we'll see how it goes

> 
> >+
> >+        if ( ((startX - self.dragData.dragX)/(self.dragData.destinationX - self.dragData.originalX)  < 0 &&  (startY - self.dragData.dragY)/ (self.dragData.destinationY - self.dragData.originalY) < 0) || ( Math.abs(startX - self.dragData.dragX) < 4 && Math.abs(startY - self.dragData.dragY) < 4 )) {
> >+        
> 
> Remove blank line and space after first paren of the "if". A comment would be
> helpful here too.
> 
> Perhaps putting "startX - self.dragData.dragX" and "startY -
> self.dragData.dragY" in local variables would allow you to reuse them and
> reduce clutter too.

yup, looks better

> >             this._lastMouseDown = aEvent;
> >+            for (var i =0; i < self.PAN_EVENTS_TO_TRACK ; i++) {
> >+              self._panEventTracker[i] = null;
> >+            }
> 
> Should you reset the _panEventTrackerPtr here too?

its a circle buffer, so it shouldn't matter







    
    

    
  


  

        Attachment #333443 -
        Attachment is obsolete: true

        Attachment #333823 -
        Flags: review?(mark.finkle)

    
    

    
  
Comment on attachment 333823 [details] [diff] [review]
updated based on mfinkle's comments

Looks good. Only have some small nits. You could remove any commented lines as well before checkin.

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml
>     <implementation implements="nsIObserver">
>-      <constructor>
>+      <constructor><![CDATA[
>+
>         this._zoomLevel = 1;

Drop blank line

>+        
>+
>+       ]]></constructor>

Drop blank lines

>       <field name="dragData">
>+          destinationX: 0,
>+	  destinationY: 0

Still have 1 TAB in there

>         })
>       </field>
> 
>       <field name="_stack">
>         document.getAnonymousElementByAttribute(this, "anonid", "cstack");
>       </field>
> 
>       <field name="_canvas">
>@@ -728,16 +741,117 @@
>           // Canvas needs to move up for content to scroll down
>           this.dragData.dragX = -x;
>           this.dragData.dragY = -y;
> 
>           this._updateCanvasPosition();
>         ]]></body>
>       </method>
> 
>+      <method name="_startKinetic">
>+        <body><![CDATA[
>+           //JProfStartProfiling();
>+           var p2 = this._panEventTracker[this._panEventTrackerIndex];
>+           var p1 = this._panEventTracker[(this._panEventTrackerIndex+1) % this.PAN_EVENTS_TO_TRACK];

Spaces around "+"

>+               let destX = Math.min(this.dragData.pageX - this.dragData.dragX + this.dragData.velocityX *2000,  contentAreaWidth - canvasWidth);
>+               let destY = Math.min(this.dragData.pageY - this.dragData.dragY + this.dragData.velocityY *2000,  contentAreaHeight - canvasHeight);

Space between "*2000"

>+          let dy = self.dragData.originalY == self.dragData.destinationY?0:

Spaces around "?"

>+          // dragX/dragY are garanteed to be within the correct bounds, so just

"guaranteed"

>             this._lastMouseDown = aEvent;
>+            for (var i =0; i < self.PAN_EVENTS_TO_TRACK ; i++) {

Remove space between "=0" and remove space between "_TRACK ;"

>+            for (var i =0; i < this.deckbrowser.PAN_EVENTS_TO_TRACK ; i++) {

Remove space between "=0" and remove space between "_TRACK ;"

        Attachment #333823 -
        Flags: review?(mark.finkle) → review+

    
    

    
  
pushed changeset 7bc7f121adb0
http://hg.mozilla.org/mobile-browser/index.cgi/rev/7bc7f121adb0
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
verified with beta3
Status: RESOLVED → VERIFIED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: