Closed
Bug 890938
Opened 11 years ago
Closed 11 years ago
Convert AsyncPanZoomController::ZoomToRect to take a CSSRect instead of a gfxRect
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: kats, Assigned: botond)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
Attachment #772714 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
Attachment #772714 -
Attachment is obsolete: true
Attachment #772895 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #772895 -
Flags: review? → review?(bugmail.mozilla)
Assignee | ||
Comment 5•11 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.
Reporter | ||
Comment 6•11 years ago
|
||
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+
Reporter | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 7•11 years ago
|
||
Keywords: checkin-needed
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
Fixed in updated patch.
Attachment #772895 -
Attachment is obsolete: true
Attachment #773394 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Updated•11 years ago
|
Attachment #773394 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=fe91a2c7773e
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•