flypig.co.uk

Yesterday we were working through the bug fire hose. The last thing we looked at was the introduction of a couple of new Boolean parameters to the GoBack() and GoForward() methods. These methods are literally the tools used to navigate the history, including the methods to call when the user hits the Backwards and Forwards buttons on the browser. I promised to talk more about them today, so let's get straight to it.

There were two potential ways to fix this: either send in fixed values for the new parameters, or pass the parameters up the stack to allow the decision to be made by one of the components wrapping the Gecko library (e.g. QtMozEmbed or Sailfish-Browser). Both approaches would fix the compiler errors, but the decision ought to be made based on the functionality that the parameters trigger.

That led me to Bugzilla bugs 1648825 and 1647128.

The bugs describe an edge case involving iframes and the cross-site origin setting in relation to the use of the browser history. For example, if the JavaScript in an iframe calls the history.back() method, the set-fetch-site header gets messed up specifically because the internals of the browser don't know whether the request has come from the user (i.e. a human) or not (i.e. some bit of JavaScript code).

As these comment from Mozilla devs Andrea Marchesini and Niklas make clear, the solution chosen upstream is to pass in the exact nature of the request from the front-end downwards:
   
For Sailfish OS we do tie these two functions to front-end functionality. But what I'm not sure about is whether we expose the EmbedLite GoForward() and GoBack() calls to code as well, so that we'd need to distinguish between user- and code-triggered calls. There's certainly a possibility, for example, that these are exposed via the WebView interface, or even via our bespoke privileged JavaScript history implementation.

Consequently I ended up deciding to make the changes so that the two new parameters are exposed through the EmbedLite API. Although they may not ultimately be needed, this will at least force us to make an active decision later on. If it turns out we just pass the same values every time, we can always remove them later.

That's for the back and forward history functionality. Something similar has happened to FocusActivate() and FocusDeactivate(), which have both gained a new parameter uint64_t aActionId. These two methods are also showing up as errors in the build output as a result, like this:

195:08.97 In file included from Unified_cpp_mobile_sailfishos0.cpp:137:
195:08.98 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/EmbedLiteViewChild.cpp:
          In member function ‘virtual mozilla::ipc::IPCResult mozilla::embedlite::
          EmbedLiteViewChild::RecvSetIsActive(const bool&)’:
195:08.98 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/EmbedLiteViewChild.cpp:620:34:
          error: no matching function for call to ‘nsWebBrowser::FocusActivate()’
195:08.98        mWebBrowser->FocusActivate();
195:08.98                                   ^
195:08.98 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/
          embedshared/EmbedLiteViewChild.cpp:27,
195:08.98                  from Unified_cpp_mobile_sailfishos0.cpp:137:
195:08.98 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsWebBrowser.h:106:8:
          note: candidate: ‘void nsWebBrowser::FocusActivate(uint64_t)’
195:08.98    void FocusActivate(uint64_t aActionId);
195:08.98         ^~~~~~~~~~~~~
195:08.98 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsWebBrowser.h:106:8:
          note:   candidate expects 1 argument, 0 provided
195:08.98 In file included from Unified_cpp_mobile_sailfishos0.cpp:137:
195:08.98 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:624:34: error: no matching function for call
          to ‘nsWebBrowser::FocusDeactivate()’
195:08.98      mWebBrowser->FocusDeactivate();
195:08.98                                   ^
195:08.98 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/
          embedshared/EmbedLiteViewChild.cpp:27,
195:08.98                  from Unified_cpp_mobile_sailfishos0.cpp:137:
195:08.98 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsWebBrowser.h:107:8:
          note: candidate: ‘void nsWebBrowser::FocusDeactivate(uint64_t)’
195:08.98    void FocusDeactivate(uint64_t aActionId);
195:08.98         ^~~~~~~~~~~~~~~

Once again, we have an upstream commit to help with this.

$ git log -1 96ae695458d3b
commit 96ae695458d3beb6ca4614f07e1915eff3577fe3
Author: Henri Sivonen 
Date:   Mon Nov 16 19:16:20 2020 +0000

    Bug 1618386 - Add action ids to filter out stale active browsing context updates. r=nika
    
    Differential Revision: https://phabricator.services.mozilla.com/D94969

The following related commit affects the SetIsActive() call that's also causing issues. I'll try to address these two simultaneously:

$ git log -S "isActive" -- ./docshell/base/nsIDocShell.idl
commit 3987c781d028e4edc599659f0776d26b747bfbd6
Author: Emilio Cobos Álvarez 
Date:   Fri Dec 11 15:43:19 2020 +0000

    Bug 1635914 - Move active flag handling explicitly to BrowsingContext. r=nika
    
    And have it mirror in the parent process more automatically.
    
    The docShellIsActive setter in the browser-custom-element side needs to
    be there rather than in the usual DidSet() calls because the
    AsyncTabSwitcher code relies on getting an exact amount of notifications
    as response to that specific setter. Not pretty, but...
    
    BrowserChild no longer sets IsActive() on the docshell itself for OOP
    iframes. This fixes bug 1679521. PresShell activeness is used to
    throttle rAF as well, which handles OOP iframes nicely as well.
    
    Differential Revision: https://phabricator.services.mozilla.com/D96072

Here are the errors it's causing:

195:08.98 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsWebBrowser.h:107:8:
          note:   candidate expects 1 argument, 0 provided
195:08.98 In file included from Unified_cpp_mobile_sailfishos0.cpp:137:
195:08.99 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:639:13: error: ‘class nsIDocShell’ has no
          member named ‘SetIsActive’; did you mean ‘SetIsAppTab’?
195:08.99    docShell->SetIsActive(aIsActive);
195:08.99              ^~~~~~~~~~~
195:08.99              SetIsAppTab
195:08.99 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:642:12: error: ‘class mozilla::embedlite::
          BrowserChildHelper’ has no member named ‘SetParentIsActive’; did you
          mean ‘mParentIsActive’?
195:08.99    mHelper->SetParentIsActive(aIsActive);
195:08.99             ^~~~~~~~~~~~~~~~~
195:08.99             mParentIsActive

Before I get on to them I want to take a diversion. It's easy for me to always plough forwards addressing these issues. It's the bit I enjoy and it gives a sense of immediate progress that I find fulfilling.

But there are other tasks, just as important, but which I find it convenient to neglect: filing tasks related to issues that arise while I make these changes; requesting the help of others; checking the suggestions others have made; submitting patches to the Jolla repos so the Gecko dependencies can be included in future releases.

None of these are exactly coding tasks, which is why I don't always enjoy them so much. Today I need to deal with what I'm going to call "untechnical debt": the non-coding tasks that have been building up, which are essential and which until now I've been neglecting.

After a bit of focusing on this today, here's what I managed — and also failed — to achieve:

1. Set up an ICU project on OBS.
2. Created PRs on other packages (rust-cbindgen, icu).
3. Tried to test gcc, but unfortunately I failed with this one as I don't have access to the correct built packages yet.
4. Submitted various queries, including one about Linux kernel headers to the Community Meeting.
5. I didn't get a chance to create all the related issues. I'll have to try to do this during the week.

Having done all this I also worked my way through the remaining errors discussed above. So now finally, here's the output for the next few errors now in the queue after today's changes:

198:04.88 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/EmbedLiteViewChild.cpp:
          In member function ‘virtual mozilla::ipc::IPCResult mozilla::embedlite::
          EmbedLiteViewChild::RecvAddMessageListener(const nsCString&)’:
198:04.88 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:897:23: error: ‘nsTHashMap’ {aka ‘class nsBaseHashtable >’} has no member named ‘Put’
198:04.88    mRegisteredMessages.Put(NS_ConvertUTF8toUTF16(name), 1);
198:04.88                        ^~~
198:04.88 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/EmbedLiteViewChild.cpp:
          In member function ‘virtual mozilla::ipc::IPCResult mozilla::
          embedlite::EmbedLiteViewChild::RecvAddMessageListeners
          (nsTArray >&&)’:
198:04.88 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:911:25: error: ‘nsTHashMap’ {aka ‘class nsBaseHashtable >’} has no member named ‘Put’
198:04.88      mRegisteredMessages.Put(messageNames[i], 1);
198:04.88                          ^~~
198:04.88 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp: In member function ‘virtual
          mozilla::ipc::IPCResult mozilla::embedlite::EmbedLiteViewChild::
          RecvHandleDoubleTap(const LayoutDevicePoint&, const Modifiers&, const
          ScrollableLayerGuid&, const uint64_t&)’:
198:04.89 ${PROJECT}/gecko-dev/mobile/sailfishos/embedshared/
          EmbedLiteViewChild.cpp:1048:47: error: conversion from
          ‘mozilla::layers::ZoomTarget’ to non-scalar type ‘mozilla::CSSRect’
          {aka ‘mozilla::gfx::RectTyped’} requested
198:04.89      CSSRect zoomToRect = CalculateRectToZoomTo(document, point);
198:04.89                           ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
[...]
198:07.34 make[4]: *** [${PROJECT}/gecko-dev/config/rules.mk:676: Unified_cpp_mobile_sailfishos0.o] Error 1

No shortage of tasks to work on tomorrow then!

For all the other posts, check out my full Gecko Dev Diary.