Change FrameMetrics to use gfx:: types instead of gfxSize and nsIntRect, etc.

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: kentuckyfriedtakahe)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

FrameMetrics and AsyncPanZoomController use a mixture of double and float based types. As floating point precision is not considered to be a bottleneck in scrolling it seems more useful to have extra precision.
Component: General → Graphics: Layers
Product: Boot2Gecko → Core
Created attachment 713124 [details] [diff] [review]
Use gfxPoint, etc instead of gfx::Point
Summary: Change FrameMetrics to use gfxPoint, gfxSize, gfxRect instead of gfx:: types → Change FrameMetrics to use gfx:: types instead of gfxSize and nsIntRect, etc.
Created attachment 715014 [details] [diff] [review]
Part 1: Add ZoomScale and gfxConversion
Attachment #713124 - Attachment is obsolete: true
Attachment #715014 - Flags: review?(bas)

Comment 3

6 years ago
Try run for f02d9469d382 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f02d9469d382
Results (out of 274 total builds):
    success: 259
    warnings: 13
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f02d9469d382
Created attachment 715281 [details] [diff] [review]
Part 1: Add ZoomScale and gfxConversion
Attachment #715014 - Attachment is obsolete: true
Attachment #715014 - Flags: review?(bas)
Attachment #715281 - Flags: review?
Created attachment 715286 [details] [diff] [review]
Part 2: Move to gfx:: types for APZC

Comment 6

6 years ago
Try run for 9e071648d631 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9e071648d631
Results (out of 98 total builds):
    success: 96
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-9e071648d631
Comment on attachment 715281 [details] [diff] [review]
Part 1: Add ZoomScale and gfxConversion

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

::: gfx/2d/ZoomScale.h
@@ +16,5 @@
> +  public BaseSize<Float, ZoomScale> {
> +  typedef BaseSize<Float, ZoomScale> Super;
> +
> +  ZoomScale() : Super(1, 1) {}
> +  ZoomScale(Float aWidth, Float aHeight) : Super(aWidth, aHeight) {}

aX and aY seem better to me here, probably making BasePoint a better baseclass :s. All in all I'm not sure how I feel about this class but it can't hurt I guess.

::: gfx/thebes/gfxConversion.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

gfx2DGlue.h :)
Attachment #715281 - Flags: review? → review+
Created attachment 715823 [details] [diff] [review]
Add ZoomScale and gfxConversion;
Attachment #715286 - Attachment is obsolete: true
Created attachment 715826 [details] [diff] [review]
Use double precision in FrameMetrics
Attachment #715281 - Attachment is obsolete: true
Created attachment 717689 [details] [diff] [review]
Use double precision in FrameMetrics
Created attachment 717710 [details] [diff] [review]
Change to using gfx:: types in AsyncPanZoomController;

Comment 13

6 years ago
Try run for d3ee2270443a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d3ee2270443a
Results (out of 97 total builds):
    success: 96
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-d3ee2270443a
Attachment #715826 - Attachment is obsolete: true
Attachment #715823 - Attachment is obsolete: true
Attachment #717689 - Attachment is obsolete: true
Attachment #717710 - Flags: review?(jones.chris.g)
Attachment #717710 - Flags: review?(jones.chris.g) → review+
This caused frequent test failures: Android robocop testLoad -- bug 838563. Can we back out?
Depends on: 838563
Was looking through this patch, this looked suspicious:

    3.44 -  gfxPoint GetScrollOffsetInLayerPixels() const
    3.45 +  gfx::Point GetScrollOffsetInLayerPixels() const
    3.46    {
    3.47 -    return gfxPoint(
    3.48 -      static_cast<gfx::Float>(
    3.49 -        mScrollOffset.x * LayersPixelsPerCSSPixel().width),
    3.50 -      static_cast<gfx::Float>(
    3.51 -        mScrollOffset.y * LayersPixelsPerCSSPixel().height));
    3.52 +    return mScrollOffset * LayersPixelsPerCSSPixel();
    3.53    }

I notice there used to be a float cast after the multiplication, and looking it up, gfxPoint uses gfxFloat, which is actually a double (I didn't know this...)

So is the performance increase because of the precision drop? And would that explain the intermittent failure too?
The purpose of the patch is to improve consistency more than to improve performance. Any related performance changes could affect fragile tests. I haven't looked into it yet.

Comment 19

6 years ago
Try run for 5f8927f5a205 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5f8927f5a205
Results (out of 353 total builds):
    success: 319
    warnings: 34
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-5f8927f5a205

Comment 20

6 years ago
Try run for 24e0f68f18a9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=24e0f68f18a9
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-24e0f68f18a9
Has bitrotted and is probably not worth doing at this stage.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.