Convert AsyncPanZoomController::ZoomToRect to take a CSSRect instead of a gfxRect

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: botond)

Tracking

23 Branch
mozilla25
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → botond
(Assignee)

Comment 1

4 years ago
Created attachment 772714 [details] [diff] [review]
patch
Depends on: 865735
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
(Assignee)

Updated

4 years ago
Attachment #772714 - Flags: review?(bugmail.mozilla)
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.
Attachment #772714 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 772895 [details] [diff] [review]
updated patch
Attachment #772714 - Attachment is obsolete: true
Attachment #772895 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #772895 - Flags: review? → review?(bugmail.mozilla)
(Assignee)

Comment 5

4 years ago
(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 on attachment 772895 [details] [diff] [review]
updated patch

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

Perfect, thanks!
Attachment #772895 - Flags: review?(bugmail.mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3265d1e6ed10
Keywords: checkin-needed
Backed out for Werror bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e71d868f45

https://tbpl.mozilla.org/php/getParsedLog.php?id=25122609&tree=Mozilla-Inbound
(Assignee)

Comment 9

4 years ago
Created attachment 773394 [details] [diff] [review]
updated patch to fix Werror bustage

Fixed in updated patch.
Attachment #772895 - Attachment is obsolete: true
Attachment #773394 - Flags: review?(bugmail.mozilla)
Attachment #773394 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 10

4 years ago
Try results: https://tbpl.mozilla.org/?tree=Try&rev=fe91a2c7773e
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99aac5ea46
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2b99aac5ea46
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.