flypig.co.uk

It's the morning, the world is waking up after a night of torrential rain here in Cambridgeshire. And my build has completed.

I've copied the packages over to my development phone, installed them and have them ready to run. The thing I'm testing today is the change I made over the last couple of days to fix the preferences that were using the VarCache, but which are now using static preferences.

There are a bunch of user-defined preferences kept in the ~/.local/share/org.sailfishos/browser/.mozilla/prefs.js file which I want to take a look at first.

$ cat prefs.js 
// Mozilla User Preferences

// DO NOT EDIT THIS FILE.
//
// If you make changes to this file while the application is running,
// the changes will be overwritten when the application exits.
//
// To change a preference value, you can either:
// - modify it via the UI (e.g. via about:config in the browser); or
// - set it within a user.js file in your profile.

user_pref("app.update.lastUpdateTime.addon-background-update-timer", 1701505984);
user_pref("app.update.lastUpdateTime.region-update-timer", 1701624416);
user_pref("app.update.lastUpdateTime.search-engine-update-timer", 1701624536);
user_pref("app.update.lastUpdateTime.services-settings-poll-changes", 1701505864);
user_pref("app.update.lastUpdateTime.user-agent-updates-timer", 1701419251);
user_pref("app.update.lastUpdateTime.xpi-signature-verification", 1701505588);
[...]

I want to remove all the preferences in the embedlite group, as well as the keyword.enabled preference, since these are the ones I made changes to:

user_pref("embedlite.compositor.external_gl_context", true);
user_pref("embedlite.inputItemSize", "50");
user_pref("embedlite.zoomMargin", "20");
[...]
user_pref("keyword.enabled", true);

All four are now purged from the file. Time to fire up the browser to see what happens. My plan here is to test a couple of things:

1. Whether changing the values sticks across execution runs.
2. Whether changing the values affects behaviour.

Both of these should be the case. Let's see.

The results, I would say, are interesting. First of all the embedlite static preferences. I'm testing with the embedlite.azpc.handle.longtap preference because this has a very clear and testable impact on the user experience. When active, pressing and holding on an image will bring up a menu that allows you to save the image to disk. When disabled, pressing and holding on an image should have no effect.
 
From testing, the preference is working as expected. Long tap on an image works when the preference is set to true, but does nothing when it's set to false. That's the most important part of the test and gets a confirmatory tick: test passed.

Checking between execution runs I can see the preference is also sticky. Closing and reopening the browser doesn't affect the setting; on reopening it's set to false if it was disabled before closing and set to true otherwise: test passed.

I also checked that the preference value gets written to the prefs.js file and it does when it's set to false. When it's set to true it's removed because that's the default value. Great!

The static preferences are looking good. But one of the changes we made used a normal preference rather than a static preference and that was the keyword.enabled flag. This controls whether search is allowed in the address bar.

For this test I set the preference to true, typed some text in the address bar and checked that search was performed using my chosen search provider. I then set the preference to false and checked that the error page is displayed. All of this worked as expected: test passed.
 
Finally, does the keyword.enabled value stick across runs? I'm surprised to discover that it doesn't; every time the browser is restarted it's set back to true. I can also see that it's always set to true in the prefs.js file: test failed.

This last failure surprises me and it makes me wonder whether there's some code somewhere that's forcing it to be true? A quick grep of the code throws the following up:

$ grep -rIn "keyword.enabled" * -B1
apps/core/declarativewebutils.cpp-138-    // Enable internet search
apps/core/declarativewebutils.cpp:139:
    webEngineSettings->setPreference(QString("keyword.enabled"), QVariant(true));

I guess that explains it then: every time the browser is started it forcefully sets this value to true. I don't really know why this is done, it could be that the original value was false and it had to be enabled somewhere, or that it was to protect users from accidentally messing up their own settings. The commit message for the change from over a decade ago doesn't really help either:

$ git log -1 eeea59ae2
commit eeea59ae23466204efbbafca417f3f7542deff66
Author: Dmitry Rozhkov <dmitry.rozhkov@jollamobile.com>
Date:   Thu Aug 22 11:13:41 2013 +0300

    [sailfish-browser] Enable internet search. Fixes JB#5503, JB#5504, JB#5505,
        JB#1200

Nevertheless it does explain the behaviour we're seeing now, so that means everything is working as it should after all.

I've pushed my changes to the remote repository and so have also closed the issue.

This evening my plan is to move on to issue #1030 to try to fix printing. You might think that printing on a mobile browser is a bit redundant, but in sailfish-browser we use it to support export to PDF. I broke the printing functionality quite badly by essentially removing chunks of it to ensure the code would build. It'll be good to go back and set that straight.

[...]

So it's now the evening and time to start looking into the printing situation. The easiest way to check this is to try out the "Save web page as PDF" feature of the browser. But when I press the menu option to try this out nothing happens and I see the following output at the console:

CONSOLE message:
[JavaScript Error: "aSerializable.url is undefined"
    {file: "resource://gre/modules/DownloadCore.jsm" line: 1496}]
DownloadSource.fromSerializable@resource://gre/modules/DownloadCore.jsm:1496:5
Download.fromSerializable@resource://gre/modules/DownloadCore.jsm:1282:38
D_createDownload@resource://gre/modules/Downloads.jsm:108:39
observe/<@file:///usr/lib64/mozembedlite/components/EmbedliteDownloadManager.js:257:48

That immediately gives us a few leads to follow up. Looking inside the EmbedliteDownloadManager.js file, which is part of the embedlite-components repository, we can see the following code which relates to this:

          case "saveAsPdf":
            if (Services.ww.activeWindow) {
              (async function() {
                let list = await Downloads.getList(Downloads.ALL);
                let download = await Downloads.createDownload({
                  source: Services.ww.activeWindow,
                  target: data.to,
                  saver: "pdf",
                  contentType: "application/pdf"
                });
                download["saveAsPdf"] = true;
                download.start();
                list.add(download);
              })().then(null, Cu.reportError);
            } else {
              Logger.warn("No active window to print to pdf")
            }
            break;

The call that's causing the error is this one:

                let download = await Downloads.createDownload({
                  source: Services.ww.activeWindow,
                  target: data.to,
                  saver: "pdf",
                  contentType: "application/pdf"
                });

That takes us here:

  createDownload: function D_createDownload(aProperties) {
    try {
      return Promise.resolve(Download.fromSerializable(aProperties));
    } catch (ex) {
      return Promise.reject(ex);
    }
  },

Which takes us here:

Download.fromSerializable = function(aSerializable) {
  let download = new Download();
  if (aSerializable.source instanceof DownloadSource) {
    download.source = aSerializable.source;
  } else {
    download.source = DownloadSource.fromSerializable(aSerializable.source);
  }

Which ends up here:

DownloadSource.fromSerializable = function(aSerializable) {
  let source = new DownloadSource();
  if (isString(aSerializable)) {
    // Convert String objects to primitive strings at this point.
    source.url = aSerializable.toString();
  } else if (aSerializable instanceof Ci.nsIURI) {
    source.url = aSerializable.spec;
  } else {
    // Convert String objects to primitive strings at this point.
    source.url = aSerializable.url.toString();

Leaving us with the error "aSerializable.url is undefined". Unravelling all of this we can see that what went in as aProperties ended up as aSerializable. The properties set in aProperties are source, target, saver and contentType. Definitely no url.

In practice though, what it really wants is the source parameter sent in at the start to be either an instance of DownloadSource or a serialised version of it. We're setting it to Services.ww.activeWindow so it's probably also worth working out what Services.ww.activeWindow is returning.

The Services.ww reference appears to be to an implementation of nsIWindowWatcher. Here's the property in question:

  /**
      Retrieves the active window from the focus manager.
  */
  readonly attribute mozIDOMWindowProxy activeWindow;

I did have to make changes to some of the window referencing code, so it's possible I broke something. But it's also possible that the DownloadSource interface has changed. To check the latter I'm going to compare it against the equivalent ESR 78 code.

And there is a difference. It's right down at the DownloadSource.fromSerializable() level where there used to be a check for whether source was an instance of Ci.nsIDOMWindow and which now does something slightly different.

Here's the commit that made the change:

$ git log -1 -S "Ci.nsIDOMWindow" -- toolkit/components/downloads/DownloadCore.jsm
commit 258369999a13027375f4fa496a7d4b23fb1eddfa
Author: Jonathan Kew <jkew@mozilla.com>
Date:   Mon Jul 20 16:04:35 2020 +0000

    Bug 1641805 - Remove support for creating a DownloadSource from an
    nsIDOMWindow. r=mak
    
    Differential Revision: https://phabricator.services.mozilla.com/D84137

This is actually quite a significant change and the issue that describes it is even more alarming.
 
As noted further down in the issue description, the changes that introduced this code may be a useful template.

It's been a long day for me today already, so I'm going to call it a night here. This clearly needs some more investigation which I'll need to pick up tomorrow.

I note with some trepidation that it's Day 99 today. That means tomorrow is the next order of magnitude of time spent working of this, from a base 10 perspective at least (my preference would be to work to base 2, which would make 128 the next big event). That may feel like an awfully long time and I know there are many Sailfish OS users who would just like this to be done and ready. All I can say is that every step brings us closer to a proper release and the quality really does improve with each one. I'm confident we'll get there.

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