flypig.co.uk

Today I'm travelling back from the Isle of Arran to Cambridge. It's a ten hour journey by train, which is exciting because it gives me a good long stretch to do some coding. The middle leg from Glasgow to London is the highlight as it gives me a full five hours of uninterrupted opportunity to code. That means it's a bit of a longer write-up today.

When we reached Glasgow there was a point when it looked like we might not be making the journey after all given our train had been cancelled. But due to the quick-action and quick-thinking of my wife, we managed to get booked on the next train with seats and all. So my coding can continue!

During the trip on the way here a week ago I was able to apply patch 0002 to return the Qt layer. That was a lot of manual drudgery. This time I've not got a single long monotonous task, instead I'll be continuing to work through as many of the build errors that come up as I can and figuring out the tweaks needed to get past them.

So let's get to it. Yesterday recall that we finished the day with an error about a missing header file.

18:06.85 In file included from ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:44:
18:06.85 ${PROJECT}/gfx/thebes/gfxQtPlatform.h:11:10: fatal error:
         nsDataHashtable.h: No such file or directory
18:06.85  #include "nsDataHashtable.h"
18:06.85           ^~~~~~~~~~~~~~~~~~~
18:06.85 compilation terminated.

Thanks go to mal for looking in to this in the meantime and noting the the header file isn't needed any more. He messaged me on IRC:
 
So to tackle the error I just removed the include line. Even if there is something else needed, I figured the resulting error would indicate the structures and methods needed. It turns out that in this particular instance there is — just as mal suggested — no need to add anything. We'll discover later (spoiler alert!) this issue is going to return and then we'll need to do something slightly different.

Building the project now throws up the following:

 2:17.49 ${PROJECT}/gfx/cairo/cairo/src/cairo-qt-surface.cpp:790:14: error:
         ‘class QRegion’ has no member named ‘unite’; did you mean ‘united’?
 2:17.49       qr = qr.unite(r);
 2:17.49               ^~~~~
 2:17.50               united

This is confusing. This issue was introduced by the changes made to update Cairo to 1.17.4, Bugzilla bug 739096. We can see all of these changes in the associated pull request.

The use of QRegion::united() here was replaced with QRegion::unite(). From the code it looks like the intention is still to combine the regions, but both Qt 5 and Qt 6 seem to use the same interface with united() rather than the call it's being replaced with.

So the change appears to be breaking whichever version of Qt is intended to be used. I'll need to figure out what's really going on here, and for that I'll need to check why Cairo was updated.

I'm going to leave this investigation for a future time though. In the meantime changing the code back to using united() allows it to build. The build now throws up the following error, which seems to be unrelated.

 2:18.03 ${PROJECT}/image/decoders/icon/qt/nsIconChannel.cpp: In function
         ‘nsresult moz_qicon_to_channel(QImage*, nsIURI*, nsIChannel**)’:
 2:18.03 ${PROJECT}/image/decoders/icon/qt/nsIconChannel.cpp:101:35: error:
         ‘NS_LITERAL_CSTRING’ was not declared in this scope
 2:18.03                                    NS_LITERAL_CSTRING(IMAGE_ICON_MS));
 2:18.03                                    ^~~~~~~~~~~~~~~~~~

You may recall that we saw this error yesterday, but then it went away. That can happen: usually it's because of the multiple compilation jobs running in parallel. Fixing an error can change the timing and order of things so that we get some other error appearing on subsequent runs.

This one has come back, so we now have to tackle it. The file causing the error was pulled in as part of the changes from 0002 "Bring back Qt layer", so it's not so surprising if something has changed and broken the code. But first, I need to check that it wasn't some later patch that introduced this change. If it was, it may be that I've not pulled in all of the associated changes, which might also explain why this error is occurring.

A quick grep of the filename in the rpm folder shows it's not touched by any later patches.

$ grep -rIn "nsIconChannel.cpp" *
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:278: image/decoders/icon/qt/nsIconChannel.cpp      | 137 +++++
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:355: create mode 100644 image/decoders/icon/qt/nsIconChannel.cpp
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:2125:+    'nsIconChannel.cpp',
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:2157:diff --git a/image/decoders/icon/qt/nsIconChannel.cpp b/image/decoders/icon/qt/nsIconChannel.cpp
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:2161:+++ b/image/decoders/icon/qt/nsIconChannel.cpp

That's enough to convince me that it does need a new fix. What's more, looking through the ESR 78 code and comparing it to the ESR 91 code, I can see that all except one instance of NS_LITERAL_CSTRING usage has been removed, and that one remaining case is wrapped in a preprocessor conditional so may not be used in practice. Could it be that this macro has been removed in its entirety? There must be some way to define string literals, so hopefully it's just a case of finding out how.

The answer comes from Fabrice Desré who got in contact via Mastodon to share his experience. Fabrice is working on Capyloon, a mobile OS which is based on Boot to Gecko (the fork of Firefox OS which eventually became the basis for KaiOS). As such Fabrice is familiar with upstream Gecko code refactorings and "other fun changes" (as he puts it!). This is what Fabrice said:
 
Indeed they were. We can see this directly in the code by comparing the old ESR 78 code with our new ESR 91 code. Here's an example line from ESR 78:

browser/components/shell/nsGNOMEShellService.cpp:431:
    gsettings->GetCollectionForSchema(NS_LITERAL_CSTRING(kDesktopBGSchema),

And here's the same line in ESR 91:

    gsettings->GetCollectionForSchema(nsLiteralCString(kDesktopBGSchema),

As you can see, the NS_LITERAL_CSTRING macros has been replaced with nsLiteralCString. So I've made an identical change to the nsIconChannel.cpp file that triggered the error. Thanks Fabrice! Let's kick the build off again.

Next up are these errors:

 1:42.03 In file included from ${PROJECT}/gfx/thebes/gfxQtPlatform.cpp:12:
 1:42.03 ${PROJECT}/gfx/thebes/gfxQtPlatform.h:35:22: error:
         ‘virtual nsresult gfxQtPlatform::UpdateFontList()’ marked ‘override’, 
         but does not override
 1:42.04      virtual nsresult UpdateFontList() override;
 1:42.04                       ^~~~~~~~~~~~~~
 1:42.04 ${PROJECT}/gfx/thebes/gfxQtPlatform.h:37:34: error:
         conflicting return type specified for
         ‘virtual gfxPlatformFontList*gfxQtPlatform::CreatePlatformFontList()’
 1:42.04      virtual gfxPlatformFontList* CreatePlatformFontList() override;
 1:42.04                                   ^~~~~~~~~~~~~~~~~~~~~~
 1:42.04 In file included from ${PROJECT}/gfx/thebes/gfxQtPlatform.h:9,
 1:42.04                  from ${PROJECT}/gfx/thebes/gfxQtPlatform.cpp:12:
 1:42.05 ${PROJECT}/gfx/thebes/gfxPlatform.h:380:16: note: overridden function is
         ‘virtual bool gfxPlatform::CreatePlatformFontList()’
 1:42.05    virtual bool CreatePlatformFontList() = 0;
 1:42.05                 ^~~~~~~~~~~~~~~~~~~~~~

My immediate reaction to this is to just remove the override annotation from gfxQtPlatform::UpdateFontList(). But the gfx/thebes/gfxQtPlatform.h file this comes from is one of the new files we added ourselves, and something in the back of my mind makes me think this needs closer investigation.

Checking the code out, the class definition being inherited from has changed. Let's do a bit of software archaeology.

Here's the ESR 78 version:

  /**
   * Rebuilds the any cached system font lists
   */
  virtual nsresult UpdateFontList();

Here's the ESR 91 version:

  /**
   * Rebuilds the system font lists (if aFullRebuild is true), or just notifies
   * content that the list has changed but existing memory mappings are still
   * valid (aFullRebuild is false).
   */
  nsresult UpdateFontList(bool aFullRebuild = true);

Using git blame and git log I can see that the change happened in commit 809ac3660845f, the log entry for which references Bugzilla bug 1676966

The changes were made to optimise page rendering by removing unnecessary font re-initialisation. So there's some functional change here that we'll need to try to retain or replicate. The good news is that our overrides are actually just calling the underlying implementation, so it should just be a case of redirecting the methods and parameters appropriately.

The relevant commits to look at seem to be 809ac3660845f and 18310539bfe25.

There are some more related errors that follow.

 2:43.13 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp: In member function ‘virtual nsresult gfxFcPlatformFontList::InitFontListForPlatform()’:
 2:43.13 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1445:3: error:
         ‘ClearSystemFontOptions’ was not declared in this scope
 2:43.14    ClearSystemFontOptions();
 2:43.14    ^~~~~~~~~~~~~~~~~~~~~~
 2:43.25 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1445:3: note:
         suggested alternative: ‘PrepareFontOptions’
 2:43.25    ClearSystemFontOptions();
 2:43.25    ^~~~~~~~~~~~~~~~~~~~~~
 2:43.25    PrepareFontOptions
 2:43.26 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1514:3: error: 
         ‘UpdateSystemFontOptions’ was not declared in this scope
 2:43.26    UpdateSystemFontOptions();
 2:43.26    ^~~~~~~~~~~~~~~~~~~~~~~
 2:43.38 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1514:3: note: 
         suggested alternative: ‘PrepareFontOptions’
 2:43.38    UpdateSystemFontOptions();
 2:43.39    ^~~~~~~~~~~~~~~~~~~~~~~
 2:43.39    PrepareFontOptions
 2:43.46 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp: In lambda function:
 2:43.46 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1721:72: error: 
         could not convert ‘false’ from ‘bool’ to ‘mozilla::WeightRange’
 2:43.46                                               weight,     stretch, style};
 2:43.46                                                                         ^
 2:43.47 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1721:72: error:
         could not convert ‘weight’ from ‘gfxPlatformFontList::WeightRange’
         {aka ‘mozilla::WeightRange’} to ‘mozilla::StretchRange’
 2:43.47 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1721:72: error:
         could not convert ‘stretch’ from ‘gfxPlatformFontList::StretchRange’
         {aka ‘mozilla::StretchRange’} to ‘mozilla::SlantStyleRange’
 2:43.49 ${PROJECT}/gfx/thebes/gfxFcPlatformFontList.cpp:1721:72: error:
         could not convert ‘style’ from ‘gfxPlatformFontList::SlantStyleRange’
         {aka ‘mozilla::SlantStyleRange’} to ‘RefPtr’

From the blame of the file, the error here seems to be related to a different change; that in commit e2df872ab83b5 which references Bugzilla bug 1708285.

The fix for these turns out to be less formulaic than most of the issues we've seen so far. To address this I had to essentially update the Qt versions of the GetFontList() and CreatePlatformFontList() methods to match those made for other platforms. This meant passing in different parameters and doing slightly different things with them.

This highlights one of the difficulties in working without a working build. I've made the changes I think look appropriate, but I have no way to test them at this stage. I'll just have to come back to them later and perform the checks once things the build is working fully.

The final error in the above log output relates to the change in commit 2f1aa020c3d4c, Bugzilla bug 1714282.

While working on this I also noticed the following error. I might have missed it before because it doesn't generate a red error message. I'll need to follow this up too.

 2:13.63 make[4]: *** No rule to make target 'moc_message_pump_qt.cc', needed by
         'moc_message_pump_qt.o'.  Stop.

During the journey I'm able to fix the font method failures. The parameter conversion and error relating to the message pump will have to wait until tomorrow.

Reflecting back on my train journey, it wasn't quite as productive as it could have been. The earlier cancellation meant the train we ended up on was super-busy, which made it a trickier to concentrate. But I did make progress, which is the aim of the game here. Slowly but surely.

If you want to read my other posts on this topic, check out my Gecko Dev Diary.