flypig.co.uk

Following on from the changes we looked at yesterday, I was hoping for the build to go further this morning. Unfortunately it's not quite there yet!

188:26.34 gfx/gl
188:43.54 In file included from Unified_cpp_gfx_gl0.cpp:47:
188:43.54 ${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp: In static member
          function ‘static already_AddRefed mozilla::
          gl::GLContextProviderEGL::CreateWrappingExisting(void*, void*, void*)’:
188:43.54 ${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp:1008:71: error:
          no matching function for call to ‘mozilla::gl::EglDisplay::Create
          (const RefPtr&, EGLContext, bool)’
188:43.54    const auto egl = EglDisplay::Create(lib, (EGLContext)aDisplay, false);
188:43.54                                                                        ^

The first parameter appears to be the issue:

no known conversion for argument 1 from ‘const RefPtr’ to ‘mozilla::gl::GLLibraryEGL&’

The DefaultEglLibrary() method we're calling returns a RefPtr which we need to turn into a GLLibraryEGL& so we can pass it in to this method here:

  static std::shared_ptr Create(GLLibraryEGL&, EGLDisplay,
                                            bool isWarp);

This differs from the code we borrowed from, which was creating the display using a non-static method of the library:

  const auto egl = lib->CreateDisplay(true, &failureId);

So we need to adjust the approach slightly. As far as I can tell there's no reason why EglDisplay::Create() couldn't be using the RefPtr but it's used mostly internally inside GLLibrary so is usually accepting *this instead. It's only calling methods from the instance passed to it, so it should be safe just to remove the RefPtr and use it directly. So that's what I've done; the new line looks like this:

  const auto egl = EglDisplay::Create(*lib.operator->(), (EGLContext)aDisplay, false);

I decided to do a partial build for this bit. Here are the commands I used.

$ sfdk engine exec
$ sb2 -t SailfishOS-devel-aarch64.default
$ source `pwd`/obj-build-mer-qt-xr/rpm-shared.env
$ make -j1 -C obj-build-mer-qt-xr/gfx/gl/

This gives a much faster turnaround on small changes compared to performing an incremental build. It allows me almost immediately identify that this set of changes builds fine like this.

But to find the next set of errors I have to run the incremental build again, so off it goes.

Now some more errors:

339:36.50 In file included from Unified_cpp_mobile_sailfishos1.cpp:119:
339:36.50 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:
          In member function  virtual nsresult EmbedUnloadScriptEvent::Run()’:
339:36.50 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:152:48:
          error: no matching function for call to ‘nsTLiteralString::
          nsTLiteralString(const char [7])’
339:36.50        event->InitEvent(nsLiteralString("unload"), false, false);
339:36.50                                                 ^

For this first one it looks like it's just a case of adding the newly required string annotations. So from this:

      event->InitEvent(nsLiteralString("unload"), false, false);

To this:

      event->InitEvent(nsLiteralString(u"unload"_ns), false, false);

Now I'm using partial builds I can test this really quickly... and yes that's done the trick. So onward:

339:36.54 In file included from Unified_cpp_mobile_sailfishos1.cpp:119:
339:36.55 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:
          In member function  mozilla::WidgetTouchEvent mozilla::embedlite::
          BrowserChildHelper::ConvertMutiTouchInputToEvent
          (const mozilla::MultiTouchInput&, bool&)’:
339:36.55 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:561:16:
          error: ‘const class mozilla::MultiTouchInput’ has no member named
          ‘ToWidgetTouchEvent’; did you mean ‘ToWidgetEvent’?
339:36.55    return aData.ToWidgetTouchEvent(widget);
339:36.55                 ^~~~~~~~~~~~~~~~~~
339:36.55                 ToWidgetEvent

We can use git blame to help us out here.

$ git blame widget/InputData.h -L 680,680
007c999be8b0b (Edgar Chen 2020-10-01 08:52:10 +0000 680)   WidgetWheelEvent ToWidgetEvent(nsIWidget* aWidget) const;
$ git log -1 007c999be8b0b
commit 007c999be8b0b85b03e06ed678515219e2007055
Author: Edgar Chen 
Date:   Thu Oct 1 08:52:10 2020 +0000

    Bug 1666201 - Part 1: Rename ToWidget{Wheel|Mouse}Event to ToWidgetEvent; r=kats
    
    So they can be used in template.
    
    Differential Revision: https://phabricator.services.mozilla.com/D91490

Looking at the diff it really does look like a straight naming swap. Nice and easy. But also worth doing a quick check to ensure there aren't any other usages that need fixing up.

$ grep -rIn "ToWidgetTouchEvent" * --include="*.cpp" | wc -l
0

Great, all clean! Up next:

339:36.56 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:
          At global scope:
339:36.56 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:751:15:
          error: no declaration matches ‘nsresult mozilla::embedlite::
          BrowserChildHelper::BeginSendingWebProgressEventsToParent()’
339:36.56  NS_IMETHODIMP BrowserChildHelper::BeginSendingWebProgressEventsToParent() {
339:36.56                ^~~~~~~~~~~~~~~~~~
339:36.56 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/BrowserChildHelper.cpp:751:15:
          note: no functions named ‘nsresult mozilla::embedlite::
          BrowserChildHelper::BeginSendingWebProgressEventsToParent()’

A quick check in the ESR 78 code shows the signature for this was previously coming from dom/interfaces/base/nsIBrowserChild.idl. It's been completely removed in ESR 91, following diff D105558:

$ git log -S beginSendingWebProgressEventsToParent -1 dom/interfaces/base/nsIBrowserChild.idl
commit 1dfe5d5e5b276e337be6b9a224513f55287f0d71
Author: Nika Layzell 
Date:   Tue Mar 9 15:29:41 2021 +0000

    Bug 1663757 - Part 3: Start sending web progress events in oop subframes, r=annyG
    
    Previously, we would only send web progress events from the toplevel
    BrowserParent, as other frames would never have the browser-child.js framescript
    loaded in them, and so would never start sending events. This change moves the
    decision to begin sending events into BrowserChild itself around the same time
    as it would've happened previously with the framescript.
    
    This new callsite should still avoid sending events for the creation of the
    initial about:blank document in the BrowserChild, while not skipping any other
    events, as before.
    
    Differential Revision: https://phabricator.services.mozilla.com/D105558

The call to BeginSendingWebProgressEventsToParent() only does one thing, which is to set the mShouldSendWebProgressEventsToParent class attribute to true. The upstream change has moved this to be the first act on calling the BrowserChild::InitBrowserChildMessageManager() method instead. We should do the same to copy this behaviour, then we can remove the BeginSendingWebProgressEventsToParent() method completely.

I've added it inside BrowserChildHelper::InitBrowserChildHelperMessageManager() and removed the now redundant method.

Next up:

339:36.58 In file included from Unified_cpp_mobile_sailfishos1.cpp:128:
339:36.58 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:
          In member function ‘virtual nsresult DirProvider::
          GetFile(const char*, bool*, nsIFile**)’:
339:36.58 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:62:33:
          error: ‘NS_LITERAL_CSTRING’ was not declared in this scope
339:36.58          rv = file->AppendNative(NS_LITERAL_CSTRING("searchEngines"));
339:36.58                                  ^~~~~~~~~~~~~~~~~~
339:36.64 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:62:33:
          note: suggested alternative: ‘JSVAL_TAG_STRING’
339:36.64          rv = file->AppendNative(NS_LITERAL_CSTRING("searchEngines"));
339:36.64                                  ^~~~~~~~~~~~~~~~~~
339:36.64                                  JSVAL_TAG_STRING
339:36.65 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:67:45:
          error: ‘NS_LITERAL_CSTRING’ was not declared in this scope
339:36.65          rv = file->AppendRelativeNativePath(NS_LITERAL_CSTRING(".local/share/org.sailfishos/browser/searchEngines"));
339:36.65                                              ^~~~~~~~~~~~~~~~~~
339:36.71 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:67:45:
          note: suggested alternative: ‘JSVAL_TAG_STRING’
339:36.71          rv = file->AppendRelativeNativePath(NS_LITERAL_CSTRING(".local/share/org.sailfishos/browser/searchEngines"));
339:36.71                                              ^~~~~~~~~~~~~~~~~~
339:36.71                                              JSVAL_TAG_STRING
339:36.71 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:117:31:
          error: ‘NS_LITERAL_CSTRING’ was not declared in this scope
339:36.71        rv = file->AppendNative(NS_LITERAL_CSTRING("defaults"));
339:36.71                                ^~~~~~~~~~~~~~~~~~
339:36.77 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:117:31:
          note: suggested alternative: ‘JSVAL_TAG_STRING’
339:36.78        rv = file->AppendNative(NS_LITERAL_CSTRING("defaults"));
339:36.78                                ^~~~~~~~~~~~~~~~~~
339:36.78                                JSVAL_TAG_STRING

These aren't new, we previously had to change all the NS_LITERAL_STRING uses. We're doing the same here, replacing NS_LITERAL_CSTRING("string") with "string"_ns instead. Note that we don't prefix the "string" with a u because this is a C-string (ASCII rather than unicode). This works for literal string literals, but if it's a pre-processor-defined string literal then we need to wrap it in a constructor instead like this: nsLiteralCString(DEFINE) (as it happens, we don't have any of these, but if we did we'd need to follow this pattern). Easy changes, but there are a few of them:

$ grep -rIn "NS_LITERAL_CSTRING" * --include="*.cpp" | wc -l
10

Nevertheless after a few minutes of editing things are looking healthier:

$ grep -rIn "NS_LITERAL_CSTRING" * --include="*.cpp" | wc -l
0

Next up:

339:36.78 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp: In
          member function ‘virtual nsresult DirProvider::GetFiles
          (const char*, nsISimpleEnumerator**)’:
339:36.78 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:183:21:
          error: ‘NS_APP_DISTRIBUTION_SEARCH_DIR_LIST’ was not declared in this scope
339:36.78    if (!strcmp(aKey, NS_APP_DISTRIBUTION_SEARCH_DIR_LIST)) {
339:36.78                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
339:36.97 ${PROJECT}/gecko-dev/mobile/sailfishos/utils/DirProvider.cpp:183:21:
          note: suggested alternative: ‘XRE_APP_DISTRIBUTION_DIR’
339:36.97    if (!strcmp(aKey, NS_APP_DISTRIBUTION_SEARCH_DIR_LIST)) {
339:36.97                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
339:36.97                      XRE_APP_DISTRIBUTION_DIR

Let's find out where this was defined in the ESR 78 code.

$ grep -rIn "NS_APP_DISTRIBUTION_SEARCH_DIR_LIST" * --include="*.h"
xpcom/io/nsAppDirectoryServiceDefs.h:44:#define NS_APP_DISTRIBUTION_SEARCH_DIR_LIST "SrchPluginsDistDL"

Now let's find out what happened to it.

$ git log -1 -S "NS_APP_DISTRIBUTION_SEARCH_DIR_LIST" xpcom/io/nsAppDirectoryServiceDefs.h
commit 5a80757288f70d0278f3c3124d2abc0029baafab
Author: Mark Banner 
Date:   Tue Sep 1 18:08:22 2020 +0000

    Bug 1619926 - Remove distribution search directory provider definitions. r=daleharvey
    
    Also remove DirectoryProvider as it is now unused.
    
    Depends on D88018
    
    Differential Revision: https://phabricator.services.mozilla.com/D88019

Well, I guess it's gone then. Previously it looks like this was used to distinguish the "SrchPluginsDistDL" search path from others. It's now only used in this one place, so for the sake of simplicity I've decided just to define it at the top of the file with the same value. This may be unnecessary: it could be that we can just remove the bit of code it's guarding completely. But it also doesn't look like it's going to be doing a huge amount of harm if the code is retained either (there probably won't be a directly called "SrchPluginsDistDL" any more anyway), so I think it's okay to leave it in too.

That leaves just one more for this particular set of unified source files.

${PROJECT}/gecko-dev/mobile/sailfishos/modules/EmbedLiteAppService.cpp:172:53:
required from here
${PROJECT}/obj-build-mer-qt-xr/dist/include/nsBaseHashtable.h:741:14: error:
no match for ‘operator=’ (operand types are ‘mozilla::UniquePtr
<sTArray<nsCOMPtr<nsIEmbedMessageListener> >, mozilla::DefaultDelete
<nsTArray<nsCOMPtr<nsIEmbedMessageListener> > > >’ and ‘nsTArray
<nsCOMPtr<nsIEmbedMessageListener> >*’)
       Data() = std::forward<U>(aData);

I get a real sense of déjà vue with this one. Probably because I already switched mMessageListeners.Get() for mMessageListeners.InsertOrUpdate() at some point in the past. This one took me a while to figure out, but after carefully looking at the upstream changes it all make perfect sense: they're basically combining this complex seven-line dance into a single neat call.

  nsTArray<nsCOMPtr<nsIEmbedMessageListener> >* array;
  nsDependentCString cstrname(name);
  if (!mMessageListeners.Get(cstrname, &array)) {
    array = new nsTArray<nsCOMPtr<nsIEmbedMessageListener> >();
    mMessageListeners.Get(cstrname, array);
  }
  array->AppendElement(listener);

Here's what the new code becomes:

  nsDependentCString cstrname(name);
  mMessageListeners.GetOrInsertNew(cstrname)->AppendElement(listener);

Much nicer!

That dealt with all the errors we had until now from Unified_cpp_mobile_sailfishos2.cpp. But with those cleared there are now a whole collection more just in this one unified file. Here are the first few:

In file included from Unified_cpp_mobile_sailfishos2.cpp:2:
${PROJECT}/gecko-dev/mobile/sailfishos/utils/EmbedLiteXulAppInfo.cpp:229:1:
error: no declaration matches ‘nsresult mozilla::embedlite::EmbedLiteXulAppInfo::
GetRemoteType(nsAString&)’
 EmbedLiteXulAppInfo::GetRemoteType(nsAString& aRemoteType) {
 ^~~~~~~~~~~~~~~~~~~
In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/utils/
                      EmbedLiteXulAppInfo.h:11,
                 from ${PROJECT}/gecko-dev/mobile/sailfishos/utils/
                      EmbedLiteXulAppInfo.cpp:9,
                 from Unified_cpp_mobile_sailfishos2.cpp:2:
${PROJECT}/obj-build-mer-qt-xr/dist/include/nsIXULRuntime.h:223:14: note:
candidate is: ‘virtual nsresult mozilla::embedlite::EmbedLiteXulAppInfo::
GetRemoteType(nsACString&)’
   NS_IMETHOD GetRemoteType(nsACString& aRemoteType) override; \
              ^~~~~~~~~~~~~

In ESR 78 this GetRemoteType() method is coming from gecko-dev/xpcom/system/nsIXULRuntime.idl:

  /**
   * The type of remote content process we're running in.
   * null if we're in the parent/chrome process. This can contain
   * a URI if Fission is enabled, so don't use it for any kind of
   * telemetry.
   */
  readonly attribute AString remoteType;

In ESR 91 it's still there, but the signature has changed slightly:

  readonly attribute AUTF8String remoteType;

We just need to update the signature to match.

In file included from Unified_cpp_mobile_sailfishos2.cpp:29:
${PROJECT}/gecko-dev/mobile/sailfishos/utils/WebBrowserChrome.cpp:174:15:
error: no declaration matches ‘nsresult WebBrowserChrome::DestroyBrowserWindow()’
 NS_IMETHODIMP WebBrowserChrome::DestroyBrowserWindow()
               ^~~~~~~~~~~~~~~~

This one is a little more interesting. It seems to have been removed upstream before ESR 78 and reintroduce in patch 0048:

Subject: [PATCH] Revert "Bug 1494175 - Remove unimplemented
 nsIWebBrowserChrome methods. r=qdot"

This partially reverts commit 578ac09f67274b520071a3ef0052405cde0ef9f0.

Sailfish OS embedding requires destroyBrowserWindow to handle
window.close (OnWindowCloseRequested).

Hopefully I'll be able to still apply the patch easily to the current code. However, before I do that I need to commit all of the changes I've made up until now. And since it's late, that's going to be a task for tomorrow. In the morning I'll have a go at applying the patch and getting back to all the other errors that have now popped up!

So that's it for today. As always, if you want to read my other posts they're available in my full Gecko Dev Diary.