flypig.co.uk

List items

Items from the current list are shown below.

Blog

18 Sep 2023 : Day 33 #
Yesterday we ended the evening pondering on the following error. 
193:51.55 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
                                EmbedLiteViewParent.cpp:16,
193:51.55                  from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.55 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedContentController.h:32:16: error: ‘virtual void mozilla::
          embedlite::EmbedContentController::NotifyLayerTransforms(const
          nsTArray&)’ marked ‘override’, but
          does not override
193:51.55    virtual void NotifyLayerTransforms(
193:51.55                 ^~~~~~~~~~~~~~~~~~~~~
193:51.55 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedContentController.h:54:16: error: ‘virtual void mozilla::
          embedlite::EmbedContentController::NotifyPinchGesture(mozilla::
          PinchGestureInput::PinchGestureType, const ScrollableLayerGuid&,
          mozilla::LayoutDeviceCoord, mozilla::Modifiers)’ marked ‘override’,
          but does not override
193:51.55    virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
193:51.55                 ^~~~~~~~~~~~~~~~~~
I'm on the train this morning heading from Cambridge to London, so time to refocus on how to fix it.

The line causing the error for us in the ESR 91 code is the following: 
  virtual void NotifyLayerTransforms(
      const nsTArray &aTransforms) override;
This is an override for a method of the same name in the class hierarchy. Here's the original version of the method being overridden from ESR 78: 
  virtual void NotifyLayerTransforms(
      const nsTArray& aTransforms) = 0;
And for comparison, here's the updated version of the method that we need to override in ESR 91: 
  virtual void NotifyLayerTransforms(nsTArray&& aTransforms) = 0;
So it looks like it's just a slight change to the method signature: now there's no const, and the parameter is using rvalue references in the array (shout out to vlagged for explaining the notation to me before!). As it happens the EmbedLite version of this method doesn't actually do anything: 
void EmbedContentController::NotifyLayerTransforms(const nsTArray &aTransforms)
{
  LOGT("NOT YET IMPLEMENTED");
}
So it should be safe just to update the signature to match. So that's what I've done for this one. The error directly following this one is slightly different. Here the NotifyPinchGesture() method has gained an entirely new parameter. It's changed from this: 
  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
                                  const ScrollableLayerGuid& aGuid,
                                  LayoutDeviceCoord aSpanChange,
                                  Modifiers aModifiers) = 0;
To this: 
  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
                                  const ScrollableLayerGuid& aGuid,
                                  const LayoutDevicePoint& aFocusPoint,
                                  LayoutDeviceCoord aSpanChange,
                                  Modifiers aModifiers) = 0;
Notice the extra line for the aFocusPoint parameter. According to the doc strings, the new aFocusPoint is "The focus point of the pinch event.". That sounds like the sort of parameter that might be useful for the Sailfish Browser in general. However, it turns out that we don't have an implementation for this method either, so again, it should be safe just to add in the new parameter but ignore the underlying functionality.

Next up we have these errors: 
193:51.55 In file included from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.55 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewParent.cpp: In constructor ‘mozilla::embedlite::
          EmbedLiteViewParent::EmbedLiteViewParent(const uint32_t&, const
          uint32_t&, const uint32_t&, const uintptr_t&, const bool&, const bo
          invalid new-expression of abstract class type
          ‘mozilla::embedlite::EmbedContentController’
193:51.55    , mContentController(new EmbedContentController(this, mThread))
193:51.55                                                                 ^
193:51.56 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/
                                embedshared/EmbedLiteViewParent.cpp:16,
193:51.56                  from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.56 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedContentController.h:19:7: note:   because the following virtual
          functions are pure within ‘mozilla::embedlite::EmbedContentController’:
193:51.56  class EmbedContentController : public mozilla::layers::GeckoContentController
193:51.56        ^~~~~~~~~~~~~~~~~~~~~~
193:51.56 In file included from ${PROJECT}/obj-build-mer-qt-xr/ipc/ipdl/
                                _ipdlheaders/mozilla/embedlite/PEmbedLiteViewParent.h:24,
193:51.56                  from ${PROJECT}/gecko-dev/mobile/sailfishos/
                                embedshared/EmbedLiteViewParent.h:9,
193:51.56                  from ${PROJECT}/gecko-dev/mobile/sailfishos/
                                embedshared/EmbedLiteViewParent.cpp:9,
193:51.56                  from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.56 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/layers/
          GeckoContentController.h:43:16: note:         ‘virtual void mozilla::
          layers::GeckoContentController::NotifyLayerTransforms
          (nsTArray&&)’
193:51.56    virtual void NotifyLayerTransforms(nsTArray&& aTransforms) = 0;
193:51.56                 ^~~~~~~~~~~~~~~~~~~~~
193:51.56 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/layers/
          GeckoContentController.h:83:16: note:         ‘virtual void mozilla::
          layers::GeckoContentController::NotifyPinchGesture(mozilla::
          PinchGestureInput::PinchGestureType, const mozilla::layers::
          ScrollableLayerGuid&, const LayoutDevicePoint&,
          mozilla::LayoutDeviceCoord, mozilla::Modifiers)’
193:51.56    virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
193:51.56                 ^~~~~~~~~~~~~~~~~~
It looks like this "invalid new-expression of abstract class" error was caused because the two earlier functions had the wrong parameters, so the compiler took the inherited pure virtual functions from GeckoContentController instead. That means that the earlier fixes should fix these errors as well.

Next we have this, which is basically saying that the APZEventResult::mStatus attribute is now private and so can no longer be accessed the way we're trying to. 
193:51.57 In file included from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.57 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewParent.cpp: In member function ‘virtual nsresult
          mozilla::embedlite::EmbedLiteViewParent::ReceiveInputEvent
          (const mozilla::InputData&)’:
193:51.57 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewParent.cpp:464:17: error: ‘nsEventStatus mozilla::
          layers::APZEventResult::mStatus’ is private within this context
193:51.58    if (apzResult.mStatus == nsEventStatus_eConsumeNoDefault) {
193:51.58                  ^~~~~~~
193:51.58 In file included from ${PROJECT}/obj-build-mer-qt-xr/dist/include/
                                mozilla/layers/LayersMessageUtils.h:22,
193:51.58                  from ${PROJECT}/obj-build-mer-qt-xr/ipc/ipdl/
                                _ipdlheaders/mozilla/layers/PCompositorManager.h:27,
193:51.58                  from ${PROJECT}/obj-build-mer-qt-xr/ipc/ipdl/
                                _ipdlheaders/mozilla/layers/PCompositorManagerParent.h:9,
193:51.58                  from ${PROJECT}/obj-build-mer-qt-xr/dist/include/
                                mozilla/layers/CompositorManagerParent.h:15,
193:51.58                  from ${PROJECT}/gecko-dev/mobile/sailfishos/
                                embedthread/EmbedLiteCompositorBridgeParent.h:15,
193:51.58                  from ${PROJECT}/gecko-dev/mobile/sailfishos/
                                embedshared/EmbedLiteViewParent.cpp:14,
193:51.58                  from Unified_cpp_mobile_sailfishos1.cpp:2:
193:51.58 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/layers/
          APZInputBridge.h:150:17: note: declared private here
193:51.58    nsEventStatus mStatus;
193:51.58                  ^~~~~~~
          field ‘nsEventStatus mozilla::layers::APZEventResult::mStatus’ can be
          accessed via ‘nsEventStatus mozilla::layers::APZEventResult::GetStatus() const’
193:51.58    if (apzResult.mStatus == nsEventStatus_eConsumeNoDefault) {
193:51.58                  ^~~~~~~
193:51.58                  GetStatus()
The compiler gives us a pretty neat hint here: "mStatus can be accessed via APZEventResult::GetStatus()". But of course I don't trust the compiler (with good reason I think), so I'm going to have to check this myself.

First, checking the git blame and associated log gives us a clear idea about what's gone on here: 
$ git blame gfx/layers/apz/public/APZInputBridge.h -L 129,129 -L 150,150
2b2d8e7195154 (Hiroyuki Ikezoe 2021-03-02 08:06:27 +0000 129)  private:
a61744ca91510 (Botond Ballo    2019-09-19 02:45:21 +0000 150)   nsEventStatus mStatus;
$ git log -1 2b2d8e7195154
commit 2b2d8e7195154fe76b221678835f13cb541a18bb
Author: Hiroyuki Ikezoe 
Date:   Tue Mar 2 08:06:27 2021 +0000

    Bug 1678505 - Make APZEventResult::mStatus and mHandledResult private. r=botond
    
    We do want APZEventResult to have a valid mHandledResult in the case of
    nsEventStatus_eConsumeDoDefault.
    
    Note that when we call SetStatusAsConsumeDoDefault() with a InputBlockState,
    in ReceiveScrollWheelInput() for example, we need to keep the block alive there,
    so each block is now RefPtr-ed instead of a raw pointer in such functions (the
    raw pointer is sometimes the active one (mActiveWheelBlock etc.) which will be
    discarded in ProcessQueue()).
    
    Differential Revision: https://phabricator.services.mozilla.com/D103417
This certainly tallies with the compiler, and the diff also points in that direction. Here's the method that the compiler recommended to us: 
  nsEventStatus GetStatus() const { return mStatus; };
I hate to admit it, but that looks pretty good. I guess my faith in compiler recommendations went up a level. I found three instances of mResult usage in the EmbedLiteViewParent class, all of which I was able to easily changed to use GetStatus() instead (they were all reads, no writes).

Next up we have this: 
193:52.00 In file included from Unified_cpp_mobile_sailfishos1.cpp:38:
193:52.00 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/nsWindow.cpp:
          In member function ‘mozilla::gl::GLContext* mozilla::embedlite::
          nsWindow::GetGLContext() const’:
193:52.00 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/nsWindow.cpp:420:57:
          error: ‘CreateWrappingExisting’ is not a member of ‘mozilla::gl::
          GLContextProvider’ {aka ‘mozilla::gl::GLContextProviderEGL’}
193:52.00        RefPtr mozContext = GLContextProvider::CreateWrappingExisting(context, surface, display);
193:52.00                                                          ^~~~~~~~~~~~~~~~~~~~~~
From the error it looks like CreateWrappingExisting() has been removed, and checking the source code in more details confirms it. The CreateWrappingExisting() has been completely removed from the GLContextProvider class interface. Obliterated. In fact, GLContextProvider has been stripped to the bone. That's not going to be ideal. So let's find out why upstream decided to do this. 
$ git log -1 -S "CreateWrappingExisting" gfx/gl/GLContextProviderImpl.h
commit a824ab4d81046accae4dbd3e7971b9694f8d45a0
Author: Jeff Gilbert 
Date:   Fri Aug 7 07:14:46 2020 +0000

    Bug 1656034 - Support multiple EglDisplays per GLLibraryEGL. r=lsalzman,sotaro,stransky
    
    Have webrender use its own EGLDisplay, letting WebGL use a different
    one.
    
    Differential Revision: https://phabricator.services.mozilla.com/D85496
So it looks like this is a real functionality change, not just a refactoring.

Given this, I'm toying with the idea of just adding the method back in. The question is whether this will affect the issue that this commit was designed to resolve. Looking carefully through all the changes, it seems that even in ESR 78, EmbedLite was the only consumer of this method. So it's possible it just got hoovered up by these changes, rather than it being an essential change in order to add the new functionality.

Unfortunately, when I try it, reverting the commit doesn't work. So I'm going to restore it manually. This also isn't ideal; it may explode later; but it should at least help get the build through. As you may have guessed, I don't find this in any way ideal, but right now it is necessary.

With these changes made (and best forgotten about) let's move on. This is up next. 
193:52.01 In file included from Unified_cpp_mobile_sailfishos1.cpp:47:
193:52.01 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedContentController.cpp: In member function ‘void mozilla::
          embedlite::EmbedContentController::DoSendScrollEvent
          (mozilla::layers::RepaintRequest)’:
193:52.01 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedContentController.cpp:169:31: error: ‘const struct mozilla::
          layers::RepaintRequest’ has no member named ‘GetScrollOffset’; did you
          mean ‘mScrollOffset’?
193:52.01    contentRect.MoveTo(aRequest.GetScrollOffset());
193:52.01                                ^~~~~~~~~~~~~~~
193:52.01                                mScrollOffset
In this case it's looking like a case of the method name changing. Here's the method in ESR 78: 
  const CSSPoint& GetScrollOffset() const { return mScrollOffset; }
And here's something that looks remarkably similar in ESR 91, but with a slightly different name: 
  const CSSPoint& GetVisualScrollOffset() const { return mScrollOffset; }
The difference is certainly close enough to make the 'rename' hypothesis seem plausible.

Digging through the logs to check, it seems the change was made in two stages. First the original method with the original name was removed in commit 540e25bdd48b3. Around 2 hours and 53 minutes later, a following change reintroduced the method, but with this slightly different name, in commit 539d62590ca3f.

So I've gone ahead and addressed the error by renaming the use of the method in our code. So far we're doing well on the error-squashing front, so let's not lose momentum. Next up. 
193:53.07 In file included from Unified_cpp_mobile_sailfishos1.cpp:101:
193:53.07 ${PROJECT}/gecko-dev/mobile/sailfishos/modules/EmbedLiteAppService.cpp:
          In member function ‘virtual nsresult EmbedLiteAppService::
          GetAnyEmbedWindow(bool, mozIDOMWindowProxy**)’:
193:53.07 ${PROJECT}/gecko-dev/mobile/sailfishos/modules/EmbedLiteAppService.cpp:331:19:
          error: ‘class nsIDocShell’ has no member named ‘GetIsActive’; did you
          mean ‘GetIsAppTab’?
193:53.07          docShell->GetIsActive(&isActive);
193:53.08                    ^~~~~~~~~~~
Reviewing the change log for the code is helpful in understanding what's going on here: 
$ git log -1 -S "isActive" docshell/base/nsIDocShell.idl
commit 3987c781d028e4edc599659f0776d26b747bfbd6
Author: Emilio Cobos Álvarez 
Date:   Fri Dec 11 15:43:19 2020 +0000

    Bug 1635914 - Move active flag handling explicitly to BrowsingContext. r=nika
    
    And have it mirror in the parent process more automatically.
    
    The docShellIsActive setter in the browser-custom-element side needs to
    be there rather than in the usual DidSet() calls because the
    AsyncTabSwitcher code relies on getting an exact amount of notifications
    as response to that specific setter. Not pretty, but...
    
    BrowserChild no longer sets IsActive() on the docshell itself for OOP
    iframes. This fixes bug 1679521. PresShell activeness is used to
    throttle rAF as well, which handles OOP iframes nicely as well.
    
    Differential Revision: https://phabricator.services.mozilla.com/D96072
From this it looks like we can switch this: 
bool isActive;
docShell->GetIsActive(&isActive);
With this: 
bool isActive = docShell->GetBrowsingContext()->IsActive();
Worth giving a go.

There are still many, many errors in the list to work through, but I've reached my effort limit for the day, so I'm going to set the build going and take another look at what comes out tomorrow. If all of these go through, this will have been a pretty productive day of coding.

As always, if you want to read my other posts don't forget they're available in my full Gecko Dev Diary.

Comments

Uncover Disqus comments

Actions

Login Email Share

Archives

View