flypig.co.uk

I'm disappointed I didn't manage to sort out the ambience switching colour mode yesterday. I got so far and then just crashed. It was an intense week for me at work and this evening my mind decided it'd had enough.

Now it's the morning, the sun is bright and I feel fresh again. Today I'm travelling, which means train-based coding, which I love. There's nothing better than some enforced focus time spent coding combined with the knowledge that the train is making progress on your behalf, even if you're not.

But it also means I have to be a little judicious with my work. I don't want to spend the entire journey building the packages. Minimising time spent waiting for the compiler is going to be key. That, and figuring out what the correct solution is to this ambience problem.

The reason the problem is a little tricky is because, first, the "Look And Feel" code — which is platform-specific — has changed upstream between ESR 78 and ESR 91. Second, I find the way the code is structured, with the Qt variant of the nsLookAndFeel class being a subclass of nsXPLokAndFeel, to be confusing.

The "XP" in "nsXPLookAndFeel" isn't a reference to Windows XP. Ironically it's almost the opposite. In Mozilla parlance, XP is an abbreviation for "Cross Platform". Not confusing at all.

As an aside: to be fair to Mozilla, XPFE (cross-platform front-end), XPCOM (cross-platform component object model) and use of XP to mean "Cross Platform" more generally, goes back at least as far as 1998, predating the release of Windows XP in August 2001 by several years.

Back to the investigation. After a bit of thought and reviewing the code, the reason for the ambience changes failing to affect the colour mode, and in particular the reason for the breakage between ESR 78 and ESR 91, becomes clear.

The first thing to point out is that the state of the colour mode is extracted through a call to LookAndFeel::GetInt() which returns a value that relates to the current state. In ESR 78 this call is immediately forwarded to the current instance that implements nsLookAndFeel. In our case that's an instance of the Qt version of nsLookAndFeel. Here's the code from ESR 78:

nsresult LookAndFeel::GetInt(IntID aID, int32_t* aResult) {
  return nsLookAndFeel::GetInstance()->GetIntImpl(aID, *aResult);
}

The Qt version then has some code that looks like this:

nsresult
nsLookAndFeel::GetIntImpl(IntID aID, int32_t &aResult)
{
    nsresult rv = nsXPLookAndFeel::GetIntImpl(aID, aResult);
    // Make an exception for eIntID_SystemUsesDarkTheme as this is handled below
    if (NS_SUCCEEDED(rv))
        return rv;

    rv = NS_OK;
    
    switch (aID) {
        case eIntID_CaretBlinkTime:
            aResult = 500;
            break;
[...]
        case eIntID_SystemUsesDarkTheme:
            // Choose theme based on ambience
            aResult = mObserver->GetDarkAmbience() ? 1 : 0;
            break;
[...]

The particular value we're interested in is when aID is set to eIntID_SystemUsesDarkTheme, since that's the identifier used for the colour mode.

As you can see the first thing the method does is defer the request to the parent class implementation nsXPLookAndFeel::GetIntImpl(). The value returned by this is used if the call succeeds. However, if the call to the parent class fails (which happens if it doesn't support a particular value) then the execution continues, allowing the Qt implementation to provide its own version of this value instead.

So far so good. What you don't see from this — because it's in a different part of the code — is that nsXPLookAndFeel::GetIntImpl() does support the eIntID_SystemUsesDarkTheme identifier and so will return with a successful result. That prevents us providing our own value via the Qt implementation.

To work around this, on ESR 78 I updated the condition at the head of the method so that it reads like this instead:

    if (NS_SUCCEEDED(rv) && ((aID != eIntID_SystemUsesDarkTheme) || (aResult != 
    2)))
        return rv;

This is a bit nasty. It says "defer to the parent class in all cases except when the identifier is eIntID_SystemUsesDarkTheme." Although it's ugly to have code that reroutes based on a hard-coded exception like this, the alternative would have been to make changes to the parent nsXPLookAndFeel class. When I wrote this I judged the latter to be worse because nsXPLookAndField is an upstream file, whereas the Qt version of nsLookAndFeel is already part of our own Sailfish-specific implementation.

Better to make changes to our own file than to have to patch an upstream file.

How does this all relate to ESR 91? Well, in ESR 91 the execution flow has changed. Rather than routing the request to the Qt-specific implementation first, only to have it call back to the underlying cross-platform implementation from upstream, in ESR 91 things start with the cross-platform implementation. Here's the code:

nsresult nsXPLookAndFeel::GetIntValue(IntID aID, int32_t& aResult) {
  if (!sInitialized) {
    Init();
  }

  if (const auto* cached = sIntCache.Get(aID)) {
    if (cached->isNothing()) {
      return NS_ERROR_FAILURE;
    }
    aResult = cached->value();
    return NS_OK;
  }

  if (NS_SUCCEEDED(Preferences::GetInt(sIntPrefs[size_t(aID)], &aResult))) {
    sIntCache.Insert(aID, Some(aResult));
    return NS_OK;
  }

  if (NS_FAILED(NativeGetInt(aID, aResult))) {
    sIntCache.Insert(aID, Nothing());
    return NS_ERROR_FAILURE;
  }

  sIntCache.Insert(aID, Some(aResult));
  return NS_OK;
}

As we can see in this code, first the cache is checked. Next the cross-platform implementation is checked. Only if both of these fail does the code move on to calling the child class's NativeGetInt() implementation.

As a consequence of all this, all requests for the value of eIntID_SystemUsesDarkTheme get handled by the upstream code and are never routed to our Qt implementation.

Rather than try to code around this, I'm thinking a better solution would be to directly set the preference when the ambience changes.

To this end I've added in some code to do exactly this. When the observer receives a notification about an ambience change, it now updates the preference directly:

    if (mDarkAmbience != darkAmbience) {
        mDarkAmbience = darkAmbience;
        MOZ_LOG(sLookAndFeel, LogLevel::Info, ("Ambience set to %s", 
    mDarkAmbience ? "dark" : "light"));

        if (NS_SUCCEEDED(Preferences::SetInt(
    "ui.systemUsesDarkTheme", darkAmbience))) {
            Refresh();
            if (nsCOMPtr<nsIObserverService> obs = services::GetObserverService(
    )) {
                NotifyChangedAllWindows(widget::ThemeChangeKind::
    StyleAndLayout);
            }
        }
        else {
          MOZ_LOG(sLookAndFeel, LogLevel::Warning, (&quot;Setting ambience 
    preference ui.systemUsesDarkTheme failed.&quot;));
        }
    }

There are a couple of things to note about the above snippet of code. First is that we have to call Refresh() directly after setting the preference. This call will trigger all of the settings caches to be cleared:

void nsXPLookAndFeel::RefreshImpl() {
  // Wipe out our caches.
  sLightColorCache.Clear();
  sDarkColorCache.Clear();
  sFontCache.Clear();
  sFloatCache.Clear();
  sIntCache.Clear();

  // Clear any cached FullLookAndFeel data, which is now invalid.
  if (XRE_IsParentProcess()) {
    widget::RemoteLookAndFeel::ClearCachedData();
  }
}

If we don't clear these caches, the value stored at the previous access will be used instead of the new value.

The second point is that we call NotifyChangedAllWindows() directly after making the change. This tells gecko to redraw all of the live pages, so that they switch from light to dark, or dark to light, depending on the change.

This is a change to the C++ code so it'll need to be rebuilt before I can test it out. Thankfully a partial build should be enough.

After rebuilding, installing and testing the change, it's not working quite as I had hoped. It's doing what I expected, in that it's now updating the setting as the ambience changes. Unfortunately this setting is persistent across reboots and is now set to one of only two values: "dark" or "light". That means that not only does the settings change affect the rendering, it also updates the setting that's shown in the user interface. This overwrites the value if it's set to "follow ambience" to one of "dark" or "light".

That's not what we want. If the value is set to "follow ambience" we do want the render to change when the ambience changes, but we don't want the setting to change, we want it to stay as "follow ambience". That is, we want the setting to stay the same but the value used internally to be overridden transparently at runtime. This will therefore need a little rethinking. It also makes me realise why I chose not to implement it this way previously.

So, I've gone back to the approach from before, but updated it for the new structure. This means adding the condition into the nsXPLookAndFeel.cpp file (exactly in the way I'd hoped to avoid!) like this:

  if (NS_SUCCEEDED(Preferences::GetInt(sIntPrefs[size_t(aID)], &aResult))) {
    if ((aID != IntID::SystemUsesDarkTheme) || (aResult != 2)) {
      sIntCache.Insert(aID, Some(aResult));
      return NS_OK;
    }
  }

This will extract the value requested from the preferences. If the value requested is for the system dark them, or if it comes back with the value 2 ("follow ambience") then rather than returning the value directly it will instead go on and request the value via a call to nsLookAndFeel::NativeGetInt().

When it does this it'll pick up the value dynamically selected based on the ambience, rather than the value set as the preference.

I'm still letting it cache the value, which means I'm also keeping the code that calls Refresh() when the ambience changes.

Testing these changes I find that now things are working a lot better: both the fixed light and dark values work. If it's set to follow the ambience then that's exactly what it does. The only caveat is that when the browser is started from cold it's not being initialised with the correct value. That's because the message being sent from the front end at start up is getting lost. I think that's due to the fact the engine ready to receive it yet, but I'm not totally certain. That's something for me to figure out tomorrow.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.