flypig.co.uk

As thigg rightly pointed out on the forum, today is Day 40! (that's Day 40, not Day 40 factorial; even if it might sometimes feel that long!). I didn't expect to still be trying to get the build to pass at this stage, but I can confidently say that we're closer to that point than we were at Day 1!.

My thanks go to everyone who's taken the time to read even part of these diaries. But also and especially to all the people who've provided feedback and helped out. The encouragement and generosity is genuinely life-affirming.

Let's take stock. We've applied 16 out of the 98 patches that were applied to ESR 78 (patches 0001, 0002, 0007, 0009, 0011, 0016, 0018, 0020, 0021, 0032, 0036, 0037, 0048, 0061, 0088 and 0089).

In total we've made 50 commits to the EmbedLite code and 61 commits to the Gecko code (including the 16 patches), which will turn into around 58 patches in the final version (it should be less because some will be combined).

In total this amounts to 213 files changes, 6794 insertions and 699 deletions.

I'm going to go out on a limb and say that we're getting close to a buildable library for the aarch64 target. Only time will tell whether this is actually the case or not. But it feels like it to me.

So that's where we're at; now where are we going? Well yesterday we ended up with a slew of new errors in nsNativeThemeQt.cpp. This shouldn't come as a surprise: this file contains specific Qt-related code that was added by our patch and so wouldn't be built as part of the main gecko development pipeline. As things change upstream, so they will break these bits of code that haven't been tested against (or updated to match) the changes.

But before getting on to them I have some more untechnical-debt to deal with. As tends to be my way, I get into a flow with fixing changes. Committing the changes to git pulls me out of this flow, so I will bunch up changes that ought to go into multiple commits.

That's where I am now. I've made lots of changes and now I have to partition them into separate self-contained and subject-constrained commits.

When doing this I find the -p flag of git add to be essential:

-p, --patch
   Interactively choose hunks of patch between the index and the work tree and
   add them to the index. This gives the user a chance to review the difference
   before adding modified contents to the index.

   This effectively runs add --interactive, but bypasses the initial command
   menu and directly jumps to the patch subcommand. See “Interactive mode” for
   details.

When using this command, it allows me to choose individual lines to either add to a particular commit, or skip to be left for a later commit. Very convenient when unweaving changes.

Working through the changes has resulted in six new commits being added to the gecko-dev mirror, which will eventually turn into six new patches to be applied to the upstream code. Maybe not exactly six (it may turn out to be convenient or sensible to consolidate some of the patches), but it gives a rough idea.

Now that I've paid my untechnical-debt (seems to be a recurring payment), I can go back to trying to fix the code. But it's the start of my work day now, so time to shift into a different mindset until this evening!

[...]

My work day is over so it's back to gecko error squashing. The current error situation is the same as I left it yesterday:

In file included from ${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:5:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.h:16:14: error: ‘virtual nsresult nsNativeThemeQt::DrawWidgetBackground(gfxContext*, nsIFrame*, 
  nsITheme::StyleAppearance, const nsRect&, const nsRect&)’ marked ‘override’,
  but does not override
   NS_IMETHOD DrawWidgetBackground(gfxContext* aContext, nsIFrame* aFrame,
              ^~~~~~~~~~~~~~~~~~~~
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp: In function ‘void
  PaintCheckboxControl(nsIFrame*, mozilla::gfx::DrawTarget*, const nsRect&,
  const mozilla::EventStates&)’:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:49:24: error: ‘NSRectToRect’
  was not declared in this scope
   Rect shadowGfxRect = NSRectToRect(paddingRect, twipsPerPixel);
                        ^~~~~~~~~~~~
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp: In function ‘void
  PaintCheckMark(nsIFrame*, mozilla::gfx::DrawTarget*, const nsRect&)’:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:91:19: error:
  ‘NSPointToPoint’ was not declared in this scope
   builder->MoveTo(NSPointToPoint(p, appUnitsPerDevPixel));
                   ^~~~~~~~~~~~~~

For the first of these it seems there's a new method parameter added to the DrawWidgetBackground() method. The signature has changed from this:

  /**
   * Draw the actual theme background.
   * @param aContext the context to draw into
   * @param aFrame the frame for the widget that we're drawing
   * @param aWidgetType the -moz-appearance value to draw
   * @param aRect the rectangle defining the area occupied by the widget
   * @param aDirtyRect the rectangle that needs to be drawn
   */
  NS_IMETHOD DrawWidgetBackground(gfxContext* aContext, nsIFrame* aFrame,
                                  StyleAppearance aWidgetType,
                                  const nsRect& aRect,
                                  const nsRect& aDirtyRect) = 0;

To this:

  /**
   * Draw the actual theme background.
   * @param aContext the context to draw into
   * @param aFrame the frame for the widget that we're drawing
   * @param aWidgetType the -moz-appearance value to draw
   * @param aRect the rectangle defining the area occupied by the widget
   * @param aDirtyRect the rectangle that needs to be drawn
   * @param DrawOverflow whether outlines, shadows and other such overflowing
   *        things should be drawn. Honoring this creates better results for
   *        box-shadow, though it's not a hard requirement.
   */
  enum class DrawOverflow { No, Yes };
  NS_IMETHOD DrawWidgetBackground(gfxContext* aContext, nsIFrame* aFrame,
                                  StyleAppearance aWidgetType,
                                  const nsRect& aRect, const nsRect& aDirtyRect,
                                  DrawOverflow = DrawOverflow::Yes) = 0;

As the comment says "not a hard requirement", so I'm not going to change the functionality in the EmbedLite version of the method; just accept the parameter and ignore it.

Next up is the missing NSRectToRect() declaration. In ESR 78 this comes from layout/base/nsLayoutUtils.h. It appears in the same file with the same signature in ESR 91. The header isn't included directly, so maybe it's just due to a shifting around of include statements?.. I've added it in directly now to nsNativeThemeQt.cpp and that's done the trick.

There were also a bunch of cases where the StyleAppearance aAppearance parameter had changed its name to StyleAppearance aWidgetType. C++ will happily accept different parameter names in the signature and implementation (only the types have to be the same) so I didn't need to change these but did so anyway. It helps keep the naming consistent across the class hierarchy.

Now we have this:

\${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp: In function
  ‘already_AddRefed do_GetNativeThemeDoNotUseDirectly()’:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:302:22: error:
  ‘widget_disable_native_theme_for_content’ is not a member of ‘mozilla::StaticPrefs’
     if (StaticPrefs::widget_disable_native_theme_for_content()) {
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:302:22: note: suggested
  alternative: ‘widget_non_native_theme_webrender’
     if (StaticPrefs::widget_disable_native_theme_for_content()) {
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                      widget_non_native_theme_webrender

Follow the changes through, it seems this static preference was changed in D105991 to widget_non_native_theme_enabled(). Switching to use that instead seems to do the trick.

But now the same method still causes trouble:

${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp: In function ‘already_AddRefed do_GetNativeThemeDoNotUseDirectly()’:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:303:37: error: ‘nsNativeBasicTheme::nsNativeBasicTheme()’ is protected within this context
       inst = new nsNativeBasicTheme();
                                     ^

Sure enough the nsNativeBasicTheme() constructor is now protected. It didn't used to be. It changed in this commit:

$ git log -1 09651af1bd5e7
commit 09651af1bd5e707fdc85ef2e92a4f4da4af3a4d9
Author: Stephen A Pohl 
Date:   Thu Jul 30 17:02:02 2020 +0000

    Bug 1640195: Address UX feedback for non-native widget styling. r=geckoview-reviewers,emilio,agi
    
    Differential Revision: https://phabricator.services.mozilla.com/D76509

Digging in to this it looks very much like the split between Native themes and NativeBasic themes has been made more pronounced and pushed slightly further up the stack. For example, the Gtk implementation now has separate files for both.

Creating a whole new Qt Basic theme doesn't sound like the route of least resistance. Instead I'm just going to remove the choice from inside this factory method and always return the Qt theme instead.

This isn't quite enough though. There's a new method that we're going to have to define to avoid the nsNativeThemeQt class being considered abstract:

${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp: In function
  ‘already_AddRefed do_GetNativeThemeDoNotUseDirectly()’:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:302:32: error: invalid
  new-expression of abstract class type ‘nsNativeThemeQt’
     inst = new nsNativeThemeQt();
                                ^
In file included from ${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:5:
${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.h:11:7: note:   because the
  following virtual functions are pure within ‘nsNativeThemeQt’:
 class nsNativeThemeQt final : private nsNativeTheme, public nsITheme {
       ^~~~~~~~~~~~~~~
In file included from ${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.h:8,
                 from ${PROJECT}/gecko-dev/widget/qt/nsNativeThemeQt.cpp:5:
${PROJECT}/obj-build-mer-qt-xr/dist/include/nsITheme.h:114:26: note:
  ‘virtual nsITheme::ScrollbarSizes nsITheme::GetScrollbarSizes
  (nsPresContext*, nsITheme::StyleScrollbarWidth, nsITheme::Overlay)’
   virtual ScrollbarSizes GetScrollbarSizes(nsPresContext*, StyleScrollbarWidth,
                          ^~~~~~~~~~~~~~~~~

The implementation of GetScrollbatSizes() actually needs some logic, so after looking through all the different implementations (Gtk, Android, Windows, Cocoa) I put together this version:

ScrollbarSizes nsNativeThemeQt::GetScrollbarSizes(nsPresContext* aPresContext, StyleScrollbarWidth aWidth,
                                                  Overlay) {
  int32_t size = aPresContext->CSSPixelsToDevPixels(SCROLL_BAR_SIZE);
  return {size, size}; 
}

This combines the simple Android approach (which I'm hoping will be most appropriate for a mobile device) with the Sailfish Browser CSS to pixel scaling function. This will probably need tweaking later.

But that should certainly be enough to get it through at least.

I also had to fix up some errors with the changes I made yesterday related to nsPrintSettingsQt. But all straightforward stuff (the error I made was to remove some class attributes without removing their default values in the constructor). With those done the widget/qt folder now compiles. It's time to do a full build.

And so also time for me to call it a day. Forty days: this has been a long journey so far, but now might be a good time to mention that this first stage — getting the build to pass — is also likely to be the shortest. So no stopping any time soon!

If you want to read my other posts related to this, you can find them in my Gecko Dev Diary.