1448490 - Make layers id strongly-typed

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

We have a bajillion different uint64_t ids being passed around all over the codebase, and it's very easy to accidentally screw up which one you're passing where. I'd like to introduce a LayersId struct type that allows us to get better compile-time safety.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=222ad7c9579eb2f35b6d1baff0d451a186c8c771

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1448490 - Make the layers id a struct instead of a uint64_t.

The new struct is in LayersTypes.h, all the rest of the changes are just
replacing existing uint64_t instances with the new LayersId struct.

Note that there is one functional change, in
CompositorBridgeParent::DeallocPWebRenderBridgeParent, where we now
correctly convert the PipelineId to a LayersId before using it to index
into sIndirectLayerTrees, whereas before we were incorrectly just using
the mHandle part of the PipelineId.

Review commit: https://reviewboard.mozilla.org/r/230952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230952/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8962105 [details]
Bug 1448490 - Make the layers id a struct instead of a uint64_t.

https://reviewboard.mozilla.org/r/230952/#review236398


Code analysis found 8 defects in this patch:
 - 8 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/APZCTreeManager.cpp:224
(Diff revision 1)
>    FocusState& mFocusState;
>    InputData& mEvent;
>    bool mMayChangeFocus;
>  };
>  
> -APZCTreeManager::APZCTreeManager(uint64_t aRootLayersId)
> +APZCTreeManager::APZCTreeManager(LayersId aRootLayersId)

Error: Bad implicit conversion constructor for 'apzctreemanager' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:238
(Diff revision 1)
>  {
>    MOZ_ASSERT(aSize > 0);
>    return AllocShmem(aSize, ipc::SharedMemory::TYPE_BASIC, aMem);
>  }
>  
> -UiCompositorControllerParent::UiCompositorControllerParent(const uint64_t& aRootLayerTreeId)
> +UiCompositorControllerParent::UiCompositorControllerParent(const LayersId& aRootLayerTreeId)

Error: Bad implicit conversion constructor for 'uicompositorcontrollerparent' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:238
(Diff revision 1)
>  {
>    MOZ_ASSERT(aSize > 0);
>    return AllocShmem(aSize, ipc::SharedMemory::TYPE_BASIC, aMem);
>  }
>  
> -UiCompositorControllerParent::UiCompositorControllerParent(const uint64_t& aRootLayerTreeId)
> +UiCompositorControllerParent::UiCompositorControllerParent(const LayersId& aRootLayerTreeId)

Error: Bad implicit conversion constructor for 'uicompositorcontrollerparent' [clang-tidy: mozilla-implicit-constructor]

Comment 4

[Kartikaya Gupta (email:[e-mail])]

reviewbot doesn't know what it's talking about. I filed https://github.com/mozilla-releng/services/issues/999 and https://github.com/mozilla-releng/services/issues/1000

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8962105 [details]
Bug 1448490 - Make the layers id a struct instead of a uint64_t.

https://reviewboard.mozilla.org/r/230952/#review236442

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d9f32557d77
Make the layers id a struct instead of a uint64_t. r=mattwoodrow

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5d9f32557d77

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0e344e9bce
Follow-up to fix compilation of disabled-by-default logging. r=me and DONTBUILD

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/fd0e344e9bce