Last Comment Bug 890938 - Convert AsyncPanZoomController::ZoomToRect to take a CSSRect instead of a gfxRect
: Convert AsyncPanZoomController::ZoomToRect to take a CSSRect instead of a gfx...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: 23 Branch
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla25
Assigned To: Botond Ballo [:botond]
:
Mentors:
Depends on: 865735
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-08 07:55 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2013-07-12 14:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.69 KB, patch)
2013-07-09 10:17 PDT, Botond Ballo [:botond]
bugmail.mozilla: review+
Details | Diff | Review
updated patch (11.63 KB, patch)
2013-07-09 14:53 PDT, Botond Ballo [:botond]
bugmail.mozilla: review+
Details | Diff | Review
updated patch to fix Werror bustage (11.77 KB, patch)
2013-07-10 10:40 PDT, Botond Ballo [:botond]
bugmail.mozilla: review+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2013-07-08 07:55:24 PDT
This should be a decent first bug for Botond. Pretty straightforward - convert APZC::ZoomToRect to take a CSSRect and update callers and so on. See http://mxr.mozilla.org/mozilla-central/ident?i=ZoomToRect&filter= to quickly find places this is called/used so that the appropriate places can be patched. Note that MXR includes false positives (unrelated ZoomToRect identifiers) so you'll have to be careful. Try to propagate the CSSRect as far as it'll go, which should be to TabChild.cpp.
Comment 1 Botond Ballo [:botond] 2013-07-09 10:17:31 PDT
Created attachment 772714 [details] [diff] [review]
patch
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-09 12:08:53 PDT
While waiting for Botond to get his L1 commit access, I pushed this to try since it exercises mostly B2G code paths:

https://tbpl.mozilla.org/?tree=Try&rev=d828c691a073
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-09 13:55:58 PDT
Comment on attachment 772714 [details] [diff] [review]
patch

Review of attachment 772714 [details] [diff] [review]:
-----------------------------------------------------------------

Noticed a style issue (see below) but that should be easily fixable. Please upload a new patch with the comment below addressed and "r=kats" appended to the commit message. The try run is looking good so far and I don't anticipate any problems so we should be able to flag this bug for checkin once the new patch is up.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1272,5 @@
>  void AsyncPanZoomController::DetectScrollableSubframe() {
>    mDelayPanning = true;
>  }
>  
> +void AsyncPanZoomController::ZoomToRect(CSSRect zoomToRect) {

In our coding style, arguments to functions should start with "a", so this should be "aZoomToRect" (or just aRect if you don't want to change the name from before).

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +136,2 @@
>     */
> +  void ZoomToRect(CSSRect zoomToRect);

Ditto here.
Comment 4 Botond Ballo [:botond] 2013-07-09 14:53:13 PDT
Created attachment 772895 [details] [diff] [review]
updated patch
Comment 5 Botond Ballo [:botond] 2013-07-09 14:53:58 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 772714 [details] [diff] [review]
> patch
> 
> Review of attachment 772714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Noticed a style issue (see below) but that should be easily fixable. Please
> upload a new patch with the comment below addressed and "r=kats" appended to
> the commit message. The try run is looking good so far and I don't
> anticipate any problems so we should be able to flag this bug for checkin
> once the new patch is up.
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1272,5 @@
> >  void AsyncPanZoomController::DetectScrollableSubframe() {
> >    mDelayPanning = true;
> >  }
> >  
> > +void AsyncPanZoomController::ZoomToRect(CSSRect zoomToRect) {
> 
> In our coding style, arguments to functions should start with "a", so this
> should be "aZoomToRect" (or just aRect if you don't want to change the name
> from before).
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.h
> @@ +136,2 @@
> >     */
> > +  void ZoomToRect(CSSRect zoomToRect);
> 
> Ditto here.

Done.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-09 15:02:27 PDT
Comment on attachment 772895 [details] [diff] [review]
updated patch

Review of attachment 772895 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-07-10 07:11:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3265d1e6ed10
Comment 9 Botond Ballo [:botond] 2013-07-10 10:40:11 PDT
Created attachment 773394 [details] [diff] [review]
updated patch to fix Werror bustage

Fixed in updated patch.
Comment 10 Botond Ballo [:botond] 2013-07-11 07:04:40 PDT
Try results: https://tbpl.mozilla.org/?tree=Try&rev=fe91a2c7773e
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-11 07:46:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99aac5ea46
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-07-11 19:07:53 PDT
https://hg.mozilla.org/mozilla-central/rev/2b99aac5ea46

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