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

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Use gfxPoint, etc instead of gfx::Point

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Add ZoomScale and gfxConversion

Comment 3

[Mozilla RelEng Bot]

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/[e-mail]-f02d9469d382

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Add ZoomScale and gfxConversion

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Move to gfx:: types for APZC

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-9e071648d631

Comment 7

[Bas Schouten (:bas.schouten)]

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 :)

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add ZoomScale and gfxConversion;

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Use double precision in FrameMetrics

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add ZoomScale;

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Use double precision in FrameMetrics

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Change to using gfx:: types in AsyncPanZoomController;

Comment 13

[Mozilla RelEng Bot]

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/[e-mail]-d3ee2270443a

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f4eff03e82
https://hg.mozilla.org/integration/mozilla-inbound/rev/cacb9f3ee44d

Comment 15

[Geoff Brown [:gbrown]]

This caused frequent test failures: Android robocop testLoad -- bug 838563. Can we back out?

Comment 16

[Ed Morley [:emorley]]

Backed out for causing bug 838563:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ae8d4c2dda
https://hg.mozilla.org/integration/mozilla-inbound/rev/743755ad6e48

Comment 17

[Chris Lord [:cwiiis]]

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?

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

[Mozilla RelEng Bot]

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/[e-mail]-5f8927f5a205

Comment 20

[Mozilla RelEng Bot]

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/[e-mail]-24e0f68f18a9

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Has bitrotted and is probably not worth doing at this stage.