flypig.co.uk

So that's a month! I was hoping I might be closer to a working build (as opposed to a working library) by now, but it looks like there's still a fair amount of terrain to traverse. But it also feels like there's progress being made, which is the most important thing.

Yesterday I tried to tackle some non-coding tasks. But we did also look at some compiler errors as well and today we'll start with that list. The first issue we're going to look at today therefore relates to our use of nsTHashMap. The Put() method seems to have been renamed to InsertOrUpdate(), which is therefore easily fixed. Here's the upstream change:

commit 9af107a8394c5e4acc5cb781660477d10d1ba10e
Author: Simon Giesecke 
Date:   Fri Feb 26 09:11:46 2021 +0000

    Bug 1691913 - Rename nsBaseHashtable::Put to InsertOrUpdate. r=xpcom-reviewers,necko-reviewers,jgilbert,dragana,nika
    
    This makes the naming more consistent with other functions called
    Insert and/or Update. Also, it removes the ambiguity whether
    Put expects that an entry already exists or not, in particular because
    it differed from nsTHashtable::PutEntry in that regard.
    
    Differential Revision: https://phabricator.services.mozilla.com/D105473

Next up we have an issue with CalculateRectToZoomTo() which seems to relate to the return value having changed type from CSSRect to ZoomTarget.

ZoomTarget CalculateRectToZoomTo(
    const RefPtr& aRootContentDocument,
    const CSSPoint& aPoint);

Here's the change (and how I found it):

$ git blame gfx/layers/apz/util/DoubleTapToZoom.h -L49,51
56bb178351c75 (Timothy Nikkel       2021-04-13 10:41:51 +0000 49) ZoomTarget CalculateRectToZoomTo(
d2ed260822271 (Emilio Cobos Álvarez 2019-01-02 14:05:23 +0100 50)     const RefPtr& aRootContentDocument,
d2ed260822271 (Emilio Cobos Álvarez 2019-01-02 14:05:23 +0100 51)     const CSSPoint& aPoint);
$ git log -1 56bb178351c75
commit 56bb178351c752050c0a94d3dfcedf73aa7387a6
Author: Timothy Nikkel 
Date:   Tue Apr 13 10:41:51 2021 +0000

    Bug 1702467. Double tap zoom can make us zoom to a part of an element when we could fit the entire element at the same zoom. r=botond
    
    If we double tap on an element that is narrower than the viewport at maximum we can get into a situation where we zoom on part (say the bottom half) of that element but we could easily fit the entire element.
    
    This happens because the code that calculate the rect to zoom to (CalculateRectToZoomTo) doesn't know about the maximum zoom. So it proceeds as though we will fit the width of the element. Under that assumption we will have to cut off part of the element vertically, so the code centers the rect on the users tap point.
    
    This ends up cutting off part of the element vertically when it is clear that the whole element can fit on screen, which is a pretty ugly result.
    
    This is not my favourite patch. This seemed to make the most sense. Another option I considered was passing the tap point through to AsyncPanZoomController::ZoomToRect but I think this approach came out better: the calculation all happens in one place at one time.
    
    Differential Revision: https://phabricator.services.mozilla.com/D110538\

It's always interesting when there's a bit more description and reasoning in the commit log entry. Looking at the diff, it's also worth noticing an important fact, which is that ApzcTreeManager::ZoomToRect() now appears to happily consume a ZoomToRect structure: Here's the code before:

-  CSSRect zoomToRect = CalculateRectToZoomTo(document, aPoint);
[...]
-  mApzcTreeManager->ZoomToRect(guid, zoomToRect, DEFAULT_BEHAVIOR);

And here's the code after:

+  ZoomTarget zoomTarget = CalculateRectToZoomTo(document, aPoint);
[...]
+  mApzcTreeManager->ZoomToRect(guid, zoomTarget, DEFAULT_BEHAVIOR);

Now in the EmbedLite code we have to respect the return value of our call to CalculateRectToZoomTo() to see where it ends up. My hope is that it'll also end up being passed into a ApzcTreeManager::ZoomToRect() call. If that's the case, we just need to propagate the type change from CSSRect to ZoomTarget as happens in the patch. Let's see.

In the EmbedLite code the zoomToRect value almost immediately gets passed into the EmbedLiteViewChild::ZoomToRect() method like this:

      ZoomToRect(presShellId, viewId, zoomToRect);

Scrolling down we see that this method is quite succinct. Here's what it does:

bool
EmbedLiteViewChild::ZoomToRect(const uint32_t& aPresShellId,
                               const ViewID& aViewId,
                               const CSSRect& aRect)
{
  return SendZoomToRect(aPresShellId, aViewId, aRect);
}

So, it really just sends the values to the parent. Let's take a look at where this therefore ends up in EmbedLiteViewParent:

mozilla::ipc::IPCResult EmbedLiteViewParent::RecvZoomToRect(const uint32_t &aPresShellId,
                                                            const ViewID &aViewId,
                                                            const CSSRect &aRect)
{
  LOGT("thread id: %ld", syscall(SYS_gettid));
  nsWindow *window = GetWindowWidget();
  if (GetApzcTreeManager() && window) {
    GetApzcTreeManager()->ZoomToRect(ScrollableLayerGuid(window->GetRootLayerId(),
                                                         aPresShellId,
                                                         aViewId),
                                     aRect);
  }
  return IPC_OK();
}

So the value ultimately ends up getting passed into ApzcTreeManager::ZoomToRect() just as we had hoped. As far as I can tell it's not used anywhere else either.

This is a nice example of how the message passing and Interface Definition Language works. To fix this we're going to need to change the type of the CSSRect value all the way through. But some of the underlying glue code, for example that found in PEmbedLiteViewParent.cpp is auto-generated from the pEmbedLiteView.ipdl file:

    /**
     * Instructs the EmbedLiteViewThreadParent to forward a request to zoom to a rect given in
     * CSS pixels. This rect is relative to the document.
     */
    async ZoomToRect(uint32_t aPresShellId, ViewID aViewId, CSSRect aRect);

So we have to be careful to update the interface definition as well. It's not hard, it's just a little less obvious and more involved than it might initially seem.

I've walked through this process in detail because this kind of thing comes up a lot: if a method signature changes somewhere it can have a cascading effect, with the head of the process often in some interface definition file somewhere rather than in the code itself. It's one of the "features" of working with the Gecko code.

I've applied some more fixes as well. For example the mSucceeded flag from the WidgetSelectionEvent class has been replaced by a Succeeded() method instead, see upstream Bugzilla bug 1678553:

$ git log -1 912a5bc76d005
commit 912a5bc76d0054385fcbc34a72745cedfcbce033
Author: Masayuki Nakano 
Date:   Wed Dec 2 05:32:19 2020 +0000

    Bug 1678553 - part 13: Make `WidgetQueryContentEvent` use `Maybe` to store some data r=m_kato,geckoview-reviewers
    
    Sorry for this big patch.
    
    This makes `WidgetQueryContentEvent::Reply` is stored with `Maybe` to get
    rid of `WidgetQueryContentEvent`.  And `Reply` stores offset and string
    with `Maybe` and ``OffsetAndData`, and also tentative caret offset
    with `Maybe`.  Then, we can get rid of `WidgetQueryContentEvent::NOT_FOUND`.
    
    Note that I tried to make `OffsetAndData` have a method to create `NSRange`
    for cocoa widget.  However, it causes the column limit`to 100 or longer
    and that causes unrelated changes in `TextEvents.h` and `IMEData.h`.
    Therefore, I create an inline function in `TextInputHandler.mm` instead.
    
    Differential Revision: https://phabricator.services.mozilla.com/D98264

In this same set of changes we can see that WidgetQueryContentEvent::mReply.mOffset is now WidgetQueryContentEvent::mReply.StartOffset().

There is also a new parameter needed for APZEventState::ProcessTouchEvent(). Thankfully there's a very clear example of how to fix it given in the changes to BrowserChild.cpp in the upstream diff:

$ git log -1 803a237d97998
commit 803a237d97998d0ac90a1ea467f871276c816883
Author: Kartikaya Gupta 
Date:   Wed Sep 9 19:57:36 2020 +0000

    Bug 1648491 - Have the main thread double-check APZ's consumable state. r=botond
    
    APZ can sometimes indicate that it will be consuming touch events, even though
    the touch-action properties prohibit it. This can happen if, for example, APZ
    is waiting on the main-thread for accurate touch-action information. In such
    cases, the main thread has enough information to filter out these false positives.
    This patch makes it do that, by plumbing the allowed touch behaviors into
    the APZEventState code that triggers the pointercancel event.
    
    Differential Revision: https://phabricator.services.mozilla.com/D89303

And that's literally all the errors that were output before the build process decided to give up. So, since I've at least had a stab at fixing them all, it's time to kick the build off again to see what happens.

Unfortunately I had to stash my changes to git at one point during this process, so it'll probably be a full rebuild.

In case this is the first post you happen to be reading, all the other posts can be found in my full Gecko Dev Diary.