GNOME Bugzilla – Bug 633553
magnifier: Redundant calls to global.get_pointer
Last modified: 2013-12-04 18:03:38 UTC
scrollToMousePos: function(prevCoord) { let [xMouse, yMouse, mask] = global.get_pointer(); if (!prevCoord || prevCoord.x != xMouse || prevCoord.y != yMouse) { let sysMouseOverAny = false; this._zoomRegions.forEach(function(zoomRegion, index, array) { if (zoomRegion.scrollToMousePos()) sysMouseOverAny = true; [...] scrollToMousePos: function() { let [xMouse, yMouse, mask] = global.get_pointer(); The position should be passed into ZoomRegion.scrollToMousePos() rather than requeried - global.get_pointer() is moderately expensive since it requires a round-trip to the X server.
Oh, and then that function it calls _updateMousePosition and isMouseOverRegion which both call global.get_pointer() again... Maybe the best technique here (to avoid passing the current position all over the place) is to cache the current mouse position as magnifier.mouseX, magnifier.mouseY, and then the code in zoomRegion can simply access this._magnifier.mouseX
Created attachment 173574 [details] [review] Fix redundant calls to global.get_pointer() Since we are keeping a current pointer position anyways, we don't have to continually call global.get_pointer() which is a round trip to the magnifier; make ZoomRegion simply fetch a current position stored in the Magnifier object.
Assigning for review
Review of attachment 173574 [details] [review]: As part of the review, I tried to apply the patch and test that everything functioned as before, including interacting with Orca. However, I can't apply against master. Is there another branch that that I should be using? The patch fails near ZoomRegion.isMouseOverRegion(), near line 1028. It looks like the original file has already been changed to _isMouseOverRegion() -- note the underscore. After that, the source file is very different from master. Is there a version of the patch that will work with what is in master? That just deals with global.get_pointer() changes? ::: js/ui/magnifier.js @@ -1026,3 @@ }, - _isMouseOverRegion: function(xMouse, yMouse) { There is no private _isMouseOverRegion() in master. @@ -1038,2 @@ mouseIsOver = ( xMouse >= this._viewPortX && xMouse < (this._viewPortX + this._viewPortWidth) && Also, no _viewPortX, etc. It looks like the source file for the diff is coming from somewhere else that I don't have access to.
(In reply to comment #4) > Review of attachment 173574 [details] [review]: > > As part of the review, I tried to apply the patch and test that everything > functioned as before, including interacting with Orca. However, I can't apply > against master. Is there another branch that that I should be using? This patch is on top of the refactoring in bug 633582 (note the Depends On: field in the bug)
Review of attachment 173574 [details] [review]: Testing this patch revealed no problems nor bugs. There is a small correction to the parameter documentation for a method, and a question about changing another method from public to private. Otherwise, this looks good. ::: js/ui/magnifier.js @@ +175,1 @@ * the new position is the same as the last one (optional). Nit: since the prevCoord parameter is no longer used in the function, remove the documentation for it (from Magnifier.scrollToMousePos()). @@ +1025,3 @@ }, + _isMouseOverRegion: function() { I don't see why _isMouseOverRegion() is marked private now. Calling it does not change the state of the ZoomRegion instance, and it's conceivable that some client code would want to know. Why hide this functionality?
(In reply to comment #6) > + _isMouseOverRegion: function() { > > I don't see why _isMouseOverRegion() is marked private now. Calling it does > not change the state of the ZoomRegion instance, and it's conceivable that some > client code would want to know. Why hide this functionality? As part of my big refactoring patch I made every method that wasn't used from outside ZoomRegion private. My general philosophy would be that if I see a function that's public I have to wonder about where it's being used outside the class, outside the source file, but if a function is private, then I know I don't have to worry about, and if I want to refactor it - change a parameter, etc, that I can do it locally. This is especially important for Javascript where we don't have a compiler or strict type checking to catch places where a refactoring misses a caller. So local methods are easier to understand and maintain, and I don't think the fact that a method is potentially useful in the future from another code portion justifies making it public right now. If we need it public in the future, we can easily make it public in the future.
> ... if I see a > function that's public I have to wonder about where it's being used outside the > class, outside the source file, but if a function is private, then I know I > don't have to worry about, and if I want to refactor it - change a parameter, > etc, that I can do it locally. This is especially important for Javascript > where we don't have a compiler or strict type checking to catch places where a > refactoring misses a caller. I see. Thanks for the explanation. I'll just say that in this specific case, there are no parameters to the function, and it internally doesn't have any side effects -- it's 'harmless' to call it from the outside. However, it's not worth holding up committing this patch. I can live with it as is, switching it to public in the future as needed. If you just fix/remove the documentation about the no longer used 'prevCoord' parameter in Magnifier.scrollToMousePos(), then it will be ready to push.
Pushed with left-over docs removed Attachment 173574 [details] pushed as 978cf9d - Fix redundant calls to global.get_pointer()