1480530 - Update to boxobjects no longer having a QI method

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1480527.

Now in Thunderbird, things are a bit complicated because of FakeTreeBoxObject.  As a result, there's code in mail/base/content/SearchDialog.js and mail/base/content/mailTabs.js that does:

  something.tree.boxObject.QueryInterface(Ci.nsITreeBoxObject);

I don't know whether those QIs are really needed.  You could try/catch around them to ensure that things work even if "boxObject" is an actual TreeBoxObject.

There is also a completely useless QI in suite/components/bindings/toolbar-xpfe.xml that can just go away.

Comment 1

[Jorg K (CEST = GMT+2)]

There's one more in  comm-central/mail/test/mozmill/shared-modules/test-folder-display-helpers.js
https://dxr.mozilla.org/comm-central/search?q=Ci.nsITreeBoxObject)&redirect=false

That one might kill all our MozMill tests.

So what are you suggesting exactly?

try {
  xxx = something.tree.boxObject
} catch (e) {}

I don't think so, so perhaps where it's used?
mail/base/content/mailContextMenus.js
67 let row = gFolderDisplay.treeBox.getRowAt(event.clientX, event.clientY);

That's the only use I can see apart form the immediate use in test-folder-display-helpers.js.

Comment 2

[Boris Zbarsky [:bzbarsky]]

A QI call on a WebIDL object either throws or is a no-op.

A QI call on an XPConnect object either throws or flattens the relevant interface onto the object.

In all cases the return value of the call doesn't actually matter (in JS).

The  code you link to right now clearly doesn't expect the call to throw.  What it expects is that it flattens the nsITreeBoxObject interface onto the object in the xpconnect case and does absolutely nothing in the WebIDL case.

So you could just as easily write this code as:

  let boxObject = mc.threadTree.boxObject;
  try { 
    boxObject.QueryInterface(Ci.nsITreeBoxObject);
  } catch (e) { /* don't care */ }

  if (viewIndex < boxObject.getFirstVisibleRow() ||

etc.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1480530-boxobjects.patch

Here is Boris suggestion.

After bug 1480527 landed, tried one MozMill test at random (composition/test-image-display.js) and it failed. With this patch, it passes.

So let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=825d575d885736ab7d3166ce64dcfa419f272262

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1480530-remove-QI.patch

Previous try was successful, but I wonder whether we can get rid of the QI altogether:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ebf07d20cdb282a48da2bfcd91513b6136e0790

My "random" test works without it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/130d70148c34
Port bug 1480527: boxobjects no longer have a QI method. rs=bustage-fix DONTBUILD

Comment 6

[Jorg K (CEST = GMT+2)]

Landed folded patch that just removes the QI calls.

Comment 7

[:aceman]

Comment on attachment 8997282 [details] [diff] [review]
1480530-remove-QI.patch

Review of attachment 8997282 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, let's see what happens ;)