flypig.co.uk

It's another freezing cold day today. It certainly feels like winter now. All the better to be snug inside and doing some coding. As I write this it's already the evening; over the last few days I've been working on fixing address bar search. That's now working (up to a point, because not all of the available search providers' sites quite work correctly yet), but while investigating it I hit the issue of the VarCache having been removed. These need to be converted into statis preferences, as detailed in task #1027.

I've been psyching myself up for tackling these preferences. I'm not sure, but I suspect this task is going to require a bit of investigation and thought. Here's a bit of code that needs fixing. I've helpfully left some comments there for myself to pick up, which offers me some solid ground to start from.

static void ReadAZPCPrefs()
{
  // TODO: Switch these to use static prefs
  // See https://firefox-source-docs.mozilla.org/modules/libpref/index.html#static-prefs
  // Example: https://phabricator.services.mozilla.com/D40340

  // Init default azpc notifications behavior
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.viewport,
    "embedlite.azpc.handle.viewport", true);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.singleTap,
    "embedlite.azpc.handle.singletap", false);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.longTap,
    "embedlite.azpc.handle.longtap", false);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.scroll,
    "embedlite.azpc.handle.scroll", true);

  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.viewport,
    "embedlite.azpc.json.viewport", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.singleTap,
    "embedlite.azpc.json.singletap", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.doubleTap,
    "embedlite.azpc.json.doubletap", false);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.longTap,
    "embedlite.azpc.json.longtap", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.scroll,
    "embedlite.azpc.json.scroll", false);

  //Preferences::AddBoolVarCache(&sAllowKeyWordURL, "keyword.enabled",
    sAllowKeyWordURL);

  sHandleDefaultAZPC.viewport = true; // "embedlite.azpc.handle.viewport"
  sHandleDefaultAZPC.singleTap = false; // "embedlite.azpc.handle.singletap"
  sHandleDefaultAZPC.longTap = false; // "embedlite.azpc.handle.longtap"
  sHandleDefaultAZPC.scroll = true; // "embedlite.azpc.handle.scroll"

  sPostAZPCAsJson.viewport = true; // "embedlite.azpc.json.viewport"
  sPostAZPCAsJson.singleTap = true; // "embedlite.azpc.json.singletap"
  sPostAZPCAsJson.doubleTap = false; // "embedlite.azpc.json.doubletap"
  sPostAZPCAsJson.longTap = true; // "embedlite.azpc.json.longtap"
  sPostAZPCAsJson.scroll = false; // "embedlite.azpc.json.scroll"

  sAllowKeyWordURL = sAllowKeyWordURL;
    // "keyword.enabled" (intentionally retained for clarity)
}

In this text I've suggested to convert these into static prefs. Like normal prefers static prefs are handled via libpref, but as I understand it they're more efficient because they're defined as variables which can be requested to mirror the preference value, but which can't be set within the code. Normal prefs use a hashtable lookup which makes them somewhat slower to access.

I think static prefs are going to work better in the majority of cases for what we need because many of them will actually be referenced quite regularly. The one case that doesn't fit this pattern, somewhat ironically given it's the one that sparked this journey, is keywords.enabled. That's only called on the few occassions when something is entered into to address bar.

But for the APZ preferences, I've created static preferences in StaticPrefList.yaml like this:

-   name: embedlite.azpc.handle.viewport
    type: bool
    value: true
    mirror: always

-   name: embedlite.azpc.handle.singletap
    type: bool
    value: false
    mirror: always
[...]

These will get pulled into the build process which will generate a header file "StaticPrefs_embedlite.h" which I've added to the top of the EmbedLiteViewChild.cpp which should then allows me to write code like this:

  if (StaticPrefs::embedlite_azpc_handle_viewport()) {
    mHelper->UpdateFrame(aRequest);
  }

These will be super-efficient. For the keyword search preference I've done something slightly different, like this:

  if (Preferences::GetBool("keyword.enabled", true)) {
    flags |= nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
    flags |= nsIWebNavigation::LOAD_FLAGS_FIXUP_SCHEME_TYPOS;
  }

One final thing to do with these preferences is note that they also appear in embedding.js. The libpref documentation has this to say about this:
 
This makes me wonder whether embedding.js is more like all.js, or more like firefox.js. My instinct says the latter, but even then this explanation implies that there are only some situations in which it might make sense to include them in an app-specific file. An obvious case might be when different apps need different default values.

I think there's no need to store them in embedding.js. Removing them from this won't prevent the user changing them for storage within their profile, so I don't see any downsides. It just removes one more place to have to maintain the details. So I've also deleted the preferences from embedding.js.

Apart from these preferences in EmbedLiteViewChild.cpp there are a few other preferences that need dealing with as a consequence of my earlier destruction too:

$ grep -rIn "AddBoolVarCache" *
embedding/embedlite/utils/BrowserChildHelper.cpp:82:
  //Preferences::AddBoolVarCache(&sPostAZPCAsJsonViewport, "embedlite.azpc.json.viewport", false);
embedding/embedlite/embedthread/EmbedLiteCompositorBridgeParent.cpp:68:
  //Preferences::AddBoolVarCache(&mUseExternalGLContext,
embedding/embedlite/embedshared/nsWindow.cpp:63:
  //Preferences::AddBoolVarCache(&sUseExternalGLContext,
embedding/embedlite/embedshared/nsWindow.cpp:65:
  //Preferences::AddBoolVarCache(&sRequestGLContextEarly,

I've gone through each of these and also turned them into static prefs. With this process complete, the next step is to build the code and see whether this produces good results. The build process will need to generate the header files from StaticPrefList.yaml for this to work, otherwise the C++ code won't compile. So doing the build is going to be essential and likely it'll need a full rebuild as well.

[...]

Except the build has failed:

 7:15.53 ${PROJECT}/gecko-dev/modules/libpref/init/StaticPrefList.yaml: error:
 7:15.53   `embedlite.azpc.handle.viewport` pref must come before
     `zoom.minPercent` pref

It turns out the preferences have to be in alphabetical order based on the first word in the name. Interesting! So I've reordered the preferences and set the build going again.

So that has to be it for today. Tomorrow morning, once the build has hopefully completed, we can see whether it works or not.

Before signing off I want to note down one other point that I think is interesting. It's something that I've wondered for a long time but never got around to checking. If we look in the embedding.js file we see lots of lines like this:

pref("dom.w3c_touch_events.enabled", 1);
pref("dom.w3c_touch_events.legacy_apis.enabled", true);
[...]

I've always been curious to know what these lines actually do. In my exploration around the code today I noticed that this function is defined in prefcalls.js. So mystery solved, here's what this is doing:

function pref(prefName, value) {
  try {
    var prefBranch = getPrefBranch();

    if (typeof value == "string") {
      if (gIsUTF8) {
        prefBranch.setStringPref(prefName, value);
        return;
      }
      prefBranch.setCharPref(prefName, value);
    } else if (typeof value == "number") {
      prefBranch.setIntPref(prefName, value);
    } else if (typeof value == "boolean") {
      prefBranch.setBoolPref(prefName, value);
    }
  } catch (e) {
    displayError("pref", e);
  }
}

That all rather makes sense. Okay, that really is it for today.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.