903665 - tabmail.xml: documentation for perTabPanel not matching implementation

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Jens Müller (:tessarakt)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130805 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130805004006

Steps to reproduce:

The documentation at the beginning of tabmail.xml says:

    - The tab type definition should include the following attributes:
    - * name: The name of the tab-type, mainly to aid in debugging.
    - * panelId or perTabPanel: If using a single tab panel, the id of the
    -     panel must be provided in panelId.  If using one tab panel per tab,
    -     perTabPanel should be the XUL element name that should be created for
    -     each tab.  If a reason arises, in the future we might support
    -     this being a helper function to create and return the element.

Apparently, this has already been done, see lines 552ff.:

            // should we create the element for them, or will they do it?
            if (typeof(tab.mode.tabType.perTabPanel) == "string") {
              tab.panel = document.createElementNS(
                "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
                tab.mode.tabType.perTabPanel);
            }
            else {
              tab.panel = tab.mode.tabType.perTabPanel(tab);
            }


Actual results:

-


Expected results:

The documentation should accurately describe the implementation.

Comment 1

[Jens Müller (:tessarakt)]

Hi Andrew,

the implementation from your attachment 342401 [details] [diff] [review] already allows perTabPanel to be a function. Is this already usable or currently "unsupported"?

BR, Jens

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

As I wrote the comment I was probably thinking that only letting you specify a string might be limiting.  And then I probably acted on that concern and added the other code path but neglected to update the string.

I would personally treat perTabPanel being a function as supported regardless of the comment right now.  Since you are being very diligent about this, I would encourage you to provide a patch to update the comment for the benefit of others and so there's no disagreement down the road.  I suspect you were planning to do that anyways :)

Comment 3

[Jens Müller (:tessarakt)]

Attachment: 903665-tabmail-documentation.patch

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 788568 [details] [diff] [review]
903665-tabmail-documentation.patch

I'm not a reviewer for Thunderbird code anymore, but this looks fine to me.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 788568 [details] [diff] [review]
903665-tabmail-documentation.patch

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

LGTM - thanks Jens.

Comment 6

[Jens Müller (:tessarakt)]

Patch concerns documentation commment only - please land DONTBUILD unless landing with other patches.

Comment 7

[Jens Müller (:tessarakt)]

Attachment: Parallel change for the tabmail.xml clone in SeaMonkey.

Apparently, the tabmail.xml in SeaMonkey has quite some differences to the one in Thunderbird - only one of the changes in the TB patch seems to be applicable there.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/db7b7d1628cd

Leaving open for the SM patch.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/dab224518b42