flypig.co.uk

Yesterday we were working through our epic list of build errors. Epic != Good of course! I got through maybe two or three of them and today need to plough onward. As I left things the plan was to replace RecvGetFrameUniformity() in EmbedLiteCompositorProcessParent with GetFrameUniformity(), so that's where we'll start today.

The code change is pretty straightforward, I just followed the same pattern as in ContentCompositorBridgeParent. But this new version actually adds some functionality in, so there's a fair chance it will fail at the next build. Let's see.

Next up we have this:

229:05.36 ${PROJECT}/gecko-dev/mobile/sailfishos/embedprocess/
          EmbedLiteCompositorProcessParent.h:61:16: error: ‘virtual void
          mozilla::embedlite::EmbedLiteCompositorProcessParent::
          SetConfirmedTargetAPZC(const LayersId&, const uint64_t&, const 
          nsTArray&)’ marked ‘override’,
          but does not override
229:05.36    virtual void SetConfirmedTargetAPZC(const LayersId& aLayersId,
229:05.36                 ^~~~~~~~~~~~~~~~~~~~~~

I'm having real trouble figuring this error out just by looking through the code. The method is found in CompositorBridgeParentBase and CompositorBridgeParent. I don't see any difference in the way it's been approached between the ESR 78 and ESR 91 versions of the code.

I've already fixed a few of the earlier errors, but I don't see how they might affect this one. Still, maybe running the build again will throw up something interesting.

Reluctantly I set the build running again. I'm reluctant because it's going to take a few hours to get back to this point again (even with an incremental build). But otherwise I'm at a loss, so it's worth a try.

The build runs... and produces the same error. After some more staring at the code the reason becomes clear. The third parameter has lost its const-ness and also become an rvalue reference (thanks to vlagged for putting me straight on this!). I must have read over that line multiple times and not spotted it.

void SetConfirmedTargetAPZC(
    const LayersId& aLayersId, const uint64_t& aInputBlockId,
    nsTArray&& aTargets) override;

Checking using git blame throws up this:

$ git log -1 f92b30262c8dc
commit f92b30262c8dca96dd456931fb53d3581e15e599
Author: Botond Ballo 
Date:   Tue Aug 25 02:17:06 2020 +0000

    Bug 1635256 - Avoid unnecessary array copies in NotifyLayerTransforms and SetConfirmedTargetAPZC. r=kats
    
    Differential Revision: https://phabricator.services.mozilla.com/D88083

The code associated with this method doesn't actually do anything, so thankfully fixing this should just be a case of changing the method signature to match. I drop the const and add the extra reference and we can move on.

Next up is this, which looks similar but actually turns out to be unrelated.

229:05.36 ${PROJECT}/gecko-dev/mobile/sailfishos/embedprocess/
          EmbedLiteCompositorProcessParent.h:67:35: error:
          ‘virtual mozilla::ipc::IPCResult mozilla::embedlite::
          EmbedLiteCompositorProcessParent::RecvRemotePluginsReady()’ marked
          ‘override’, but does not override
229:05.36    virtual mozilla::ipc::IPCResult RecvRemotePluginsReady() override { return IPC_OK(); }
229:05.36                                    ^~~~~~~~~~~~~~~~~~~~~~

Here's the relevant change:

$ git log -1 -S RecvRemotePluginsReady gfx/layers/ipc/CompositorBridgeParent.h 
commit b41a2b9d21cd513afd5c913ecd58e3b4a08e8a6c
Author: Mats Palmgren 
Date:   Mon Jan 25 11:53:49 2021 +0000

    Bug 1687239 part 2 - Remove plugin support from layout/.  r=emilio
    
    Note that there's still a little plugin related code in
    widget/ and gfx/ etc after this.  That can be removed
    once we remove plugin support from dom/ etc.
    The removal from layout/ should be pretty complete though.
    
    Differential Revision: https://phabricator.services.mozilla.com/D102140

The RecvRemotePluginsReady() method isn't actually doing anything in our code:

virtual mozilla::ipc::IPCResult RecvRemotePluginsReady() override { return IPC_OK(); }

So the easiest thing to do is probably just to remove it.

Next up is this:

229:05.48 ${PROJECT}/gecko-dev/mobile/sailfishos/embedprocess/
          EmbedLiteCompositorProcessParent.cpp:70:57: error: no matching
          function for call to ‘mozilla::layers::CompositorOptions::
          CompositorOptions(bool, bool)’
229:05.48                             CompositorOptions(true, false),
229:05.48                                                          ^

The CompositorOptions.h file has grown significantly since the ESR 78 release. The CompositorOptions() method is still there, but it seems to have gained an extra Boolean parameter. From this:

CompositorOptions(bool aUseAPZ, bool aUseWebRender)

To this:

CompositorOptions(bool aUseAPZ, bool aUseWebRender,
                  bool aUseSoftwareWebRender)

With the difference having been introduced with this commit:

$ git log -1 08a43977900e5
commit 08a43977900e505d91d2a6935f3905d62983f40c
Author: Andrew Osmond 
Date:   Wed Feb 24 19:40:00 2021 +0000

    Bug 1688096 - Part 2. Add flag to CompositorOptions to allow SW-WR on a per widget basis. r=mattwoodrow
    
    The pref gfx.webrender.software.unaccelerated-widget.allow may be used
    to allow software WebRender to be used with new windows/popups that have
    transparency on Windows. Otherwise they would fallback to basic layers.
    
    Similarly, the pref gfx.webrender.software.unaccelerated-widget.force
    may be used to force software WebRender for all windows that would
    fallback to basic layers.
    
    Differential Revision: https://phabricator.services.mozilla.com/D104855

The new parameter is UseSoftwareWebRender which, according to this text, seems to be useful for rendering transparent widgets. That doesn't seem to match with anything the Sailfish implementation might need so I'm just going to set the parameter to false. Later on we may need to flip this to true.

We're gradually picking these errors off, but there are many more to do. At this rate they'll keep me busy until at least the end of this week.

Next up we have the following:

229:05.49 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/layers/
          CompositorOptions.h:29:7: note:   candidate expects 1 argument,
          2 provided. In member function ‘virtual mozilla::layers::PLayerTransactionParent* 
          mozilla::embedlite::EmbedLiteCompositorProcessParent::
          AllocPLayerTransactionParent(const nsTArray<
          mozilla::layers::LayersBackend>&, const LayersId&)’:
229:05.50 ${PROJECT}/gecko-dev/mobile/sailfishos/embedprocess/
          EmbedLiteCompositorProcessParent.cpp:168:25: error: ‘void
          mozilla::layers::LayerTransactionParent::AddIPDLReference()’
          is protected within this context
229:05.50      p->AddIPDLReference();
229:05.50                          ^

It looks like this and a few other related errors might be fixed by patch 0021 "Hackish fix for preferences usage in Parent process (part 1)". Although this patch seems to be fixing something else, it does make EmbedLiteCompositorProcessParent a friend of LayerTransactionParent, which should give it access to those protected methods.

The patch applies without incident:

$ patch -d gecko-dev -p1 < rpm/0021-sailfishos-gecko-Hackish-fix-for-preferences-usage-i.patch 
patching file dom/ipc/DOMTypes.ipdlh
Hunk #1 succeeded at 27 with fuzz 2 (offset 7 lines).
patching file gfx/layers/composite/LayerManagerComposite.cpp
Hunk #1 succeeded at 181 (offset 2 lines).
patching file gfx/layers/ipc/LayerTransactionParent.h
patching file gfx/thebes/gfxPlatform.cpp
Hunk #1 succeeded at 1022 (offset -22 lines).
patching file modules/libpref/Preferences.cpp
Hunk #1 succeeded at 90 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 3519 with fuzz 1 (offset -46 lines).

So let's proceed with that. Time for another rebuild. And time for some sleep. It was slow, but steady, progress today. It looks like we've addressed a few errors, so we're moving forwards one error at a time.

As always, for other posts, check out my full Gecko Dev Diary.