Closed Bug 903665 Opened 8 years ago Closed 8 years ago

tabmail.xml: documentation for perTabPanel not matching implementation

Categories

(Thunderbird :: Toolbars and Tabs, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 26.0

People

(Reporter: tessarakt, Assigned: tessarakt)

Details

Attachments

(2 files)

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.
Summary: tabmail.xml: documentation for perTabPanel not current → tabmail.xml: documentation for perTabPanel not matching implementation
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
Flags: needinfo?(bugmail)
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 :)
Flags: needinfo?(bugmail)
Assignee: nobody → blog
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #788568 - Flags: review?(bugmail)
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.
Attachment #788568 - Flags: review?(mconley)
Attachment #788568 - Flags: review?(bugmail)
Attachment #788568 - Flags: feedback+
Comment on attachment 788568 [details] [diff] [review]
903665-tabmail-documentation.patch

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

LGTM - thanks Jens.
Attachment #788568 - Flags: review?(mconley) → review+
Patch concerns documentation commment only - please land DONTBUILD unless landing with other patches.
Keywords: checkin-needed
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.
Attachment #789295 - Flags: review?(mnyromyr)
Attachment #789295 - Flags: review?(mnyromyr) → review+
Keywords: checkin-needed
Whiteboard: [checkin still needed for attachment 789295 - please land DONTBUILD unless landing together with other patches]
https://hg.mozilla.org/comm-central/rev/dab224518b42
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin still needed for attachment 789295 - please land DONTBUILD unless landing together with other patches]
Target Milestone: --- → Thunderbird 26.0
You need to log in before you can comment on or make changes to this bug.