Closed Bug 953867 Opened 8 years ago Closed 8 years ago

Add support for tabs with arbitrary content in the conversation window

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: nhnt11)

References

Details

Attachments

(1 file, 14 obsolete files)

70.82 KB, patch
benediktp
: review+
florian
: review+
Details | Diff | Splinter Review
*** Original post on bio 426 at 2010-06-28 22:31:00 UTC ***

There were suggestions to add tabs with other content, like a buddy list, to the conversation window. Right now it's pretty complicated to do so.
Suggestion: make it easier.
Whiteboard: [0.3-nice-to-have]
*** Original post on bio 426 at 2011-05-23 16:31:06 UTC ***

It's too late to start working on this for 0.3.
Whiteboard: [0.3-nice-to-have] → [nice-to-have]
Blocks: 955013
Assignee: nobody → nhnt11
Hardware: x86 → All
Attached patch Part 1 (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2474 at 2013-06-05 19:35:00 UTC ***

This patch does the following:

- Adds a tabPanel binding which should be extended by anything that wants to add itself as a tab.
- Makes tabbrowser-tab transparent to the content of the panel.
- Makes the conversation binding extend tabPanel and override some of its methods accordingly
- Outsources handling of content-specific context menu items to tabPanel (conversation menu items are now in the conversation binding)
Attachment #8354241 - Flags: review?(benediktp)
*** Original post on bio 426 at 2013-06-06 00:20:03 UTC ***

That code isn't quite ready to be merged, but I could use some feedback.
*** Original post on bio 426 at 2013-06-06 11:04:49 UTC ***

Comment on attachment 8354241 [details] [diff] [review] (bio-attmnt 2474)
Part 1

I liked the idea of conversation.xml extending the new tabpanel binding better than the first approach (where you had a few more new bindings), but I see the point that it might be nice to keep the conversation binding free of tab-stuff to allow re-using it in a one window layout. (I don't think it actually blocks that but it's unneeded code in this case).

>@@ -1472,16 +1485,37 @@
>         <body>
>         <![CDATA[
>           this.editor.focus();
>           this.onSelect();
>         ]]>
>         </body>
>       </method>
> 
>+      <method name="handleSwitchingAwayFromTab">
>+        <parameter name="aTabWasVisible"/>
>+        <body>
>+          <![CDATA[
>+            // Remove the unread ruler if the tab has been visible without
>+            // interruptions for sufficiently long.
>+            if (aTabWasVisible)
>+              this.browser.removeUnreadRuler();
>+          ]]>
>+        </body>
>+      </method>

Is the was-long-enough-visible-flag interesting enough (and not too specific!) to have it on all tab-panels? Maybe we should put that on the conversation panel only. We'd need to forward both switching to and away from the tab then.

>+      <method name="getPanelSpecificMenuItems">
>+        <body>
>+          <![CDATA[
>+            document.getElementById("context_showLogs").disabled = !this.hasLogs();
>+            return document.getElementById("convContextMenu").childNodes;

That's a lot better than the iteration from an earlier version :)

>diff --git a/instantbird/content/tabbrowser.css b/instantbird/content/tabbrowser.css
>--- a/instantbird/content/tabbrowser.css
>+++ b/instantbird/content/tabbrowser.css
>@@ -48,8 +48,11 @@ tabconversation {
>+menuseparator+#context_tabSpecificSeparator {
>+  display: none;
>+}

Good idea to hide it this way, we format it with spaces around selectors though, i.e. "+" -> " + ".

>diff --git a/instantbird/content/tabbrowser.xml b/instantbird/content/tabbrowser.xml
>--- a/instantbird/content/tabbrowser.xml
>             <xul:menuitem id="context_openTabInWindow" label="&openTabInNewWindow.label;"
>                           accesskey="&openTabInNewWindow.accesskey;"
>                           tbattr="tabbrowser-multiple"
>+                          tbpersist="true"

Nice that you did and named this in a consistent way!

>@@ -204,21 +198,28 @@
>-            var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>-            for (var i = 0; i < menuItems.length; i++)
>-              menuItems[i].disabled = disabled;
>-            document.getElementById("context_showLogs").disabled =
>-              !this.mContextTab.linkedConversation.hasLogs();
>+            var multipleTabMenuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>+            for (var i = 0; i < multipleTabMenuItems.length; i++)
>+              multipleTabMenuItems[i].disabled = disabled;
>+            var garbageMenuItems = aPopupMenu.querySelectorAll(":not([tbpersist])");
>+            for (var i = 0; i < garbageMenuItems.length; i++)
>+              aPopupMenu.removeChild(garbageMenuItems[i]);
>+            let panelMenuItems = this.mContextTab.linkedTabPanel.getPanelSpecificMenuItems();
>+            for (let item of panelMenuItems) {
>+              // Clone node so that original doesn't get destroyed while removing garbageMenuItems
>+              aPopupMenu.insertBefore(item.cloneNode(true),
>+                              document.getElementById("context_tabSpecificSeparator"));
>+            }

I think I'd prefer a consistent way to get and iterate over the elements in a similar fashion (e.g. "querySelectorAll" and using "for ... of" for each of these cases).

(Part 2 of the review follows later)
*** Original post on bio 426 at 2013-06-06 11:41:43 UTC ***

>diff --git a/instantbird/content/tabpanel.css b/instantbird/content/tabpanel.css
>new file mode 100644
>--- /dev/null
>+++ b/instantbird/content/tabpanel.css
>@@ -0,0 +1,7 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+tabPanel {
>+  -moz-binding: url("chrome://instantbird/content/tabpanel.xml#tabPanel");
>+}

You don't need this if you don't intend to actually create a blank tabpanel by creating a "tabPanel" element. (It looks like you only have the tabpanel binding to be able extend it, not to actually instantiate it.)
*** Original post on bio 426 at 2013-06-07 09:55:27 UTC ***

>>diff --git a/instantbird/content/tabbrowser.css b/instantbird/content/tabbrowser.css
>>--- a/instantbird/content/tabbrowser.css
>>+++ b/instantbird/content/tabbrowser.css
>>@@ -48,8 +48,11 @@ tabconversation {
>>+menuseparator+#context_tabSpecificSeparator {
>>+  display: none;
>>+}
>
>Good idea to hide it this way, we format it with spaces around selectors
>though, i.e. "+" -> " + ".

Any reason a simple hidden="true" in the XBL definition of this separator doesn't work?

>+ oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;

I know it's the existing code, but maybe we can improve it? ;)

>@@ -204,21 +198,28 @@
>-            var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>-            for (var i = 0; i < menuItems.length; i++)
>-              menuItems[i].disabled = disabled;
>-            document.getElementById("context_showLogs").disabled =
>-              !this.mContextTab.linkedConversation.hasLogs();
>+            var multipleTabMenuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>+            for (var i = 0; i < multipleTabMenuItems.length; i++)
>+              multipleTabMenuItems[i].disabled = disabled;
>+            var garbageMenuItems = aPopupMenu.querySelectorAll(":not([tbpersist])");
>+            for (var i = 0; i < garbageMenuItems.length; i++)
>+              aPopupMenu.removeChild(garbageMenuItems[i]);
>+            let panelMenuItems = this.mContextTab.linkedTabPanel.getPanelSpecificMenuItems();
>+            for (let item of panelMenuItems) {
>+              // Clone node so that original doesn't get destroyed while removing garbageMenuItems
>+              aPopupMenu.insertBefore(item.cloneNode(true),
>+                              document.getElementById("context_tabSpecificSeparator"));
>+            }

This seems like it could use some comments, e.g. what is being done and what is tbattr and tbpersist for?
It would be better to clean up when the popup is hidden and not when it is opened.
Cloning a node because it is incorrectly marked as "garbage" seems a bit hackish. Can't you just not mark it as garbage?
If you know where in the popup the tab-specific menuitems are located, you shouldn't need to put flags on them (or on the others) to identify them.

I'm impressed at how empty the tabpanel implementation is now :) I agree that means we should probably consider it documentation for the API rather than something that is extended via XBL.

onSelect and handleSwitchingAwayFromTab sound like they could be handled by forwarding the corresponding event (which a panel can then listen to or not). 

For getPanelSpecificMenuItems(), how about making it addPanelMenuitems(aNode), letting the panel add whatever it wants at the position given by aNode?
*** Original post on bio 426 at 2013-06-07 10:46:51 UTC ***

>If you know where in the popup the tab-specific menuitems are located, you
>shouldn't need to put flags on them (or on the others) to identify them.
This can be used to remove all nodes between A and B
https://developer.mozilla.org/en-US/docs/Web/API/range
Blocks: 955429
*** Original post on bio 426 as attmnt 2477 at 2013-06-09 18:20:00 UTC ***

I've tried to incorporate the suggestions for the previous patch, but may have missed something - if so, I'll upload another patch.
This patch does quite a bit and I'll try and add a list of changes in a future comment.
Also waiting for bug 955432 (bio 1996) to be resolved before requesting review. I thought I'd put this up anyway though.
Attachment #8354244 - Flags: feedback?(benediktp)
Attachment #8354244 - Flags: feedback?(aleth)
Comment on attachment 8354241 [details] [diff] [review]
Part 1

*** Original change on bio 426 attmnt 2474 at 2013-06-09 18:20:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354241 - Attachment is obsolete: true
Attachment #8354241 - Flags: review?(benediktp)
Comment on attachment 8354244 [details] [diff] [review]
Updated patch, covering all needed changes to fix this bug (I think)

*** Original change on bio 426 attmnt 2477 at 2013-06-09 22:35:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354244 - Flags: feedback?(benediktp) → feedback+
*** Original post on bio 426 at 2013-06-09 22:35:10 UTC ***

Comment on attachment 8354244 [details] [diff] [review] (bio-attmnt 2477)
Updated patch, covering all needed changes to fix this bug (I think)

Here's feedback in a timely fashion so that you aren't blocked on me this time;) I think I missed a few nits and other comments but I'm planning on commenting again anyways.

> diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml
> --- a/instantbird/content/buddytooltip.xml
> +++ b/instantbird/content/buddytooltip.xml
> @@ -336,16 +338,32 @@
>           this.addRow(this.bundle.GetStringFromName("buddy.account"), account.name);
>           if (aConv.topic)
>             this.addRow(this.bundle.GetStringFromName("conversation.topic"), aConv.topic);
>           return true;
>         ]]>
>         </body>
>       </method>
>  
> +     <method name="updateTooltipFromTab">
> +       <parameter name="aTab"/>
> +       <body>
> +       <![CDATA[
> +         this.elt = aTab;
> +         this.reset();
> +         this.setAttribute("displayname", aTab.getAttribute("label"));
> +         this.setAttribute("iconPrpl", "");
> +         document.getAnonymousElementByAttribute(this, "anonid", "prplTooltipBuddyIcon")
> +                  .collapsed = true;

Nit: we format code that the dots align when continuing lines.

> diff --git a/instantbird/content/convZoom.js b/instantbird/content/convZoom.js
> --- a/instantbird/content/convZoom.js
> +++ b/instantbird/content/convZoom.js
> @@ -76,16 +76,19 @@ var FullZoom = {
>     * Set the zoom level for the current browser.
>     *
>     * Per nsPresContext::setFullZoom, we can set the zoom to its current value
>     * without significant impact on performance, as the setting is only applied
>     * if it differs from the current setting.  In fact getting the zoom and then
>     * checking ourselves if it differs costs more.
>     **/
>    applyPrefValue: function FullZoom_applyPrefValue() {
> +    if (!getBrowser().selectedBrowser) {
> +      return;
> +    }

getBrowser() as it is is misleading. Please make it return selectedBrowser directly and add a new getTabBrowser() method in instantbird.js for the other cases (see below, there's more on this topic). You can start here by looking for getBrowser() with lxr and decide in very case if the selectedBrowser or only the tabbrowser is needed. The log viewer also has a getBrowser method that would need changes then.

> diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
> --- a/instantbird/content/conversation.xml
> +++ b/instantbird/content/conversation.xml
> @@ -48,16 +48,29 @@
>            </xul:vbox>
>          </xul:hbox>
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
>          <xul:vbox anonid="conv-bottom" class="conv-bottom">
>            <xul:textbox anonid="inputBox" class="conv-textbox" multiline="true" flex="1"
>                         onclick = "delete document.getBindingParent(this)._completions;"/>
>          </xul:vbox>
>        </xul:vbox>
> +      <xul:menupopup id="convContextMenu">
> +        <xul:menuitem id="context_showLogs" accesskey="&contextShowLogs.accesskey;" label="&contextShowLogs.label;"
> +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> +                                 tabbrowser.mContextTab.linkedTabPanel.showLogs();"/>
> +        <xul:menuitem id="context_hideConv" accesskey="&contextHideConv.accesskey;" label="&contextHideConv.label;"
> +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> +                                 tabbrowser.mContextTab.linkedTabPanel.hide();
> +                                 tabbrowser.removeTab(tabbrowser.mContextTab);"/>
> +        <xul:menuitem id="context_closeConv" accesskey="&contextCloseConv.accesskey;" label="&contextCloseConv.label;"
> +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> +                                 tabbrowser.mContextTab.linkedTabPanel.close();
> +                                 tabbrowser.removeTab(tabbrowser.mContextTab);"/>
> +      </xul:menupopup>
> 

There's really no nicer way (i.e. getting rid of the parentNodes) to do this? :(

> @@ -1481,16 +1497,38 @@

> +
> +      <method name="getPanelSpecificMenuItems">
> +        <body>
> +          <![CDATA[
> +            document.getElementById("context_showLogs").disabled = !this.hasLogs();


Nit: please make sure that you keep the length of lines <= 80 characters.

> diff --git a/instantbird/content/instantbird.js b/instantbird/content/instantbird.js
> --- a/instantbird/content/instantbird.js
> +++ b/instantbird/content/instantbird.js
> @@ -31,30 +31,30 @@ var convWindow = {
>    },
>    unload: function mo_unload() {
>      Conversations.unregisterWindow(window);
>    },
>    onactivate: function mo_onactivate(aEvent) {
>      Conversations.onWindowFocus(window);
>      setTimeout(function () {
>        // setting the focus to the textbox just after the window is
> -      // activated puts the textbox in an unconsistant state, some
> +      // activated puts the textbox in an inconsistent state, some
>        // special characters like ^ don't work, so delay the focus
>        // operation...
> -      getBrowser().selectedConversation.focus();
> +      getBrowser().selectedTabPanel.focus();

This is a place where getBrowser() actually wants to be a getTabBrowser()-call for example.

> diff --git a/instantbird/content/instantbird.xul b/instantbird/content/instantbird.xul
> --- a/instantbird/content/instantbird.xul
> +++ b/instantbird/content/instantbird.xul
> @@ -49,28 +49,33 @@
>  
>  #ifdef XP_MACOSX
>  #include menus.xul.inc
>  #endif
>  
>    <commandset id="conversationsCommands">
>      <command id="cmd_close" oncommand="document.getElementById('conversations').removeCurrentTab()"/>
>      <command id="cmd_putOnHold" oncommand="var tabbrowser = document.getElementById('conversations');
> -                                           tabbrowser.mCurrentTab.linkedConversation.hide();
> +                                           if (!tabbrowser.selectedConversation) /* Not a conversation */ return;
> +                                           tabbrowser.selectedConversation.hide();
>                                             tabbrowser.removeTab(tabbrowser.mCurrentTab);"/>
> -    <command id="cmd_showLogs" oncommand="document.getElementById('conversations').mCurrentTab.linkedConversation.showLogs();"/>
> -    <command id="cmd_textZoomReduce" oncommand="FullZoom.reduce();"/>
> -    <command id="cmd_textZoomEnlarge" oncommand="FullZoom.enlarge();"/>
> -    <command id="cmd_textZoomReset" oncommand="FullZoom.reset();"/>
> +    <command id="cmd_showLogs" oncommand="let conv = document.getElementById('conversations').selectedConversation;
> +                                           if (conv) /* It's a conversation */ conv.showLogs();"/>
> +    <command id="cmd_textZoomReduce" oncommand="if (document.getElementById('conversations').selectedBrowser) FullZoom.reduce();"/>
> +    <command id="cmd_textZoomEnlarge" oncommand="if (document.getElementById('conversations').selectedBrowser) FullZoom.enlarge();"/>
> +    <command id="cmd_textZoomReset" oncommand="if (document.getElementById('conversations').selectedBrowser) FullZoom.reset();"/>

These are also places where the get(Tab)Browser functions will come in handy.

>      <command id="cmd_find"
> -             oncommand="document.getElementById('conversations').findbar.onFindCommand();"/>
> +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> +                        if (conv) conv.browser.findbar.onFindCommand();"/>
>      <command id="cmd_findAgain"
> -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(false);"/>
> +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> +                        if (conv) conv.browser.findbar.onFindAgainCommand(false);"/>
>      <command id="cmd_findPrevious"
> -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(true);"/>
> +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> +                        if (conv) conv.browser.findbar.onFindAgainCommand(true);"/>

Is the newly added ".browser" in there really correct?

> diff --git a/instantbird/content/jar.mn b/instantbird/content/jar.mn
> --- a/instantbird/content/jar.mn
> +++ b/instantbird/content/jar.mn
> @@ -49,16 +49,17 @@ instantbird.jar:
>  *	content/instantbird/menus.js
>  	content/instantbird/nsContextMenu.js
>  	content/instantbird/proxies.js
>  	content/instantbird/proxies.xul
>  	content/instantbird/proxies.css
>  	content/instantbird/proxy.xml
>  *	content/instantbird/tabbrowser.xml
>  	content/instantbird/tabbrowser.css
> +	content/instantbird/tabpanel.xml

Nice that we are down to only one new file now!

> diff --git a/instantbird/content/tabbrowser.xml b/instantbird/content/tabbrowser.xml
> --- a/instantbird/content/tabbrowser.xml
> +++ b/instantbird/content/tabbrowser.xml
> @@ -107,27 +96,24 @@
>        </field>
>        <field name="mTabContainer" readonly="true">
>          document.getAnonymousElementByAttribute(this, "anonid", "tabcontainer");
>        </field>
>        <field name="mPanelContainer" readonly="true">
>          document.getAnonymousElementByAttribute(this, "anonid", "panelcontainer");
>        </field>
>        <field name="mTabs" readonly="true">
> -        this.mTabContainer.childNodes
> +        this.mTabContainer.childNodes;

Why did you change this? I've never seen semi-colons in the definition of fields in a binding before.

> @@ -137,109 +123,109 @@
>  #endif
>        </field>
>        <field name="_browsers">
>          null
>        </field>

This field and its getter "browsers" aren't really used and should go away. The places where "browsers" is used ("moveTab*") will need fixing anyways (I think (_)tabPanels is the choice there now).

>        <method name="updateTitlebar">

> -            if (this.mCurrentTab)
> -              docTitle = this.mCurrentTab.getAttribute("label");
> -
> -            if (!docTitle)
> -              docTitle = docElement.getAttribute("titledefault");
> +            docTitle = this.mCurrentTab.getAttribute("label");

Do you have an idea what "titledefault" was? It's only mentioned here as it seems...

> -      <method name="updatePopupMenu">
> +      <method name="popupMenuShowing">

> -            var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> -            for (var i = 0; i < menuItems.length; i++)
> -              menuItems[i].disabled = disabled;
> -            document.getElementById("context_showLogs").disabled =
> -              !this.mContextTab.linkedConversation.hasLogs();
> +            var multipleTabMenuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> +            for (var i = 0; i < multipleTabMenuItems.length; i++)
> +              multipleTabMenuItems[i].disabled = disabled;

Use for ... of for consistency here?

> @@ -281,33 +267,33 @@
>                                   KeyEvent.DOM_VK_LEFT, KeyEvent.DOM_VK_RIGHT];
>  
>              if (tabKeyCodes.indexOf(event.keyCode) != -1)
>                return;
>  
>              // Focus the editbox and pass the key to it.
>              event.preventDefault();
>              event.stopPropagation();
> -            this.mCurrentConversation.editor.focus();
> +            this.selectedConversation.focus();

Focusing the conversation does more than just setting the focus to the editor. Have you checked for side-effects of this change?

> @@ -446,28 +433,73 @@
> +      <method name="addTabPanelByBindingName">
> +        <parameter name="aBindingName"/>
> +        <body>
> +          <![CDATA[
> +            newTabPanel = document.createElementNS(
> +                "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +                                              aBindingName);
> +            this.addTabPanel(newTabPanel);
> +          ]]>
> +        </body>
> +      </method>
> +
> +      <method name="addTabPanel">
> +        <parameter name="aTabPanel"/>
> +        <body>
> +          <![CDATA[
> +            // invalidate cache, because mTabContainer is about to change
> +            this._browsers = null;
> +            this._conversations = null;
> +            this._tabPanels = null;
> +
> +            this.mPanelContainer.appendChild(aTabPanel);
> +
> +            var t = this.mTabContainer.addTab();
> +            aTabPanel.tab = t;
> +
> +            this.setStripVisibilityTo(!(Services.prefs.getBoolPref("browser.tabs.autoHide") &&
> +                mFirstTabIsDummy));

Flo suggested that we might be able to get rid of mFirstTabIsDummy. Is this not possible for some reason?

> @@ -635,36 +667,34 @@
>              // JS properties set on them, the browser element won't be
>              // destroyed until the document goes away.  So we force a
>              // cleanup ourselves.
>              // This has to happen before we remove the child so that the
>              // XBL implementation of nsIObserver still works.
> -            conversation.destroy();
> -
> -            if (conversation == this.mCurrentConversation)
> -              this.mCurrentConversation = null;
> +            if (conversation) conversation.destroy();

Nit: we put if clauses with only one statement on two different lines (like in the code that was replaced here)

> @@ -672,23 +702,24 @@
>              if (this.selectedTab) // can be null if we removed the last tab
>                this.selectedTab._selected = true;
>  
>              // This will unload the document. An unload handler could remove
>              // dependant tabs, so it's important that the tabbrowser is now in
>              // a consistent state (tab removed, tab positions updated, etc.).
>              // Also, it's important that another tab has been selected before
>              // the panel is removed; otherwise, a random sibling panel can flash.
> -            this.mPanelContainer.removeChild(conversation);
> +            // The parentNode check is required because this fails in case of the dummy tab.
> +            if (tabPanel.parentNode == this.mPanelContainer) this.mPanelContainer.removeChild(tabPanel);

Nit: same here, it will also fix the line length problem.

> @@ -827,37 +835,56 @@
> 
>        <property name="browsers" readonly="true">

This property is no longer needed as mentioned above.

> -            return this._conversations ||
> -                   (this._conversations = Array.map(this.mTabs, function (tab) tab.linkedConversation));
> +            if (!this._conversations) {
> +              this._conversations = [];
> +              for (let tab of this.mTabs) {
> +                if (tab.linkedConversation)
> +                  this._conversations.push(tab.linkedConversation);
> +              }
> +            }
> +            return this._conversations;
> +          ]]>
> +        </getter>
> +      </property>

We've used a combination of map() and filter() in similar situations elsewhere already. You might want to try something like map(function(t) t.linkedConversation || null).filter(function(i) i != null) here.

> @@ -1237,42 +1266,41 @@
>        <!-- BEGIN FORWARDED BROWSER PROPERTIES.  IF YOU ADD A PROPERTY TO THE BROWSER ELEMENT
>             MAKE SURE TO ADD IT HERE AS WELL. -->
>  
>        <property name="docShell"
>        <property name="contentWindow"
>        <property name="markupDocumentViewer"
>        <property name="contentDocument"
>        <property name="contentTitle"
>        <property name="findbar"

These are the already mentioned forwarded properties that should be removed because tabbrowser shouldn't fake being a browser anymore.

> @@ -1393,37 +1421,36 @@
>          tabbrowser: this,
>          handleEvent: function handleEvent(aEvent)
>            this.tabbrowser.mCurrentTab.switchingAwayFromTab()
>        })]]>
>        </field>
>  
>        <constructor>
>          <![CDATA[
> -          this.mCurrentConversation = this.mPanelContainer.firstChild;
> -          this.mCurrentBrowser = this.mCurrentConversation.browser;
>            this.mCurrentTab = this.mTabContainer.firstChild;
> +          // First tab is always a conversation
> +          this.mCurrentTab.linkedConversation = this.mCurrentTab.linkedTabPanel = this.mPanelContainer.firstChild;

Does this assumption hold when a non-conversation tab is moved into an own window?

> diff --git a/instantbird/content/tabpanel.xml b/instantbird/content/tabpanel.xml
> new file mode 100644
> --- /dev/null
> +++ b/instantbird/content/tabpanel.xml
> @@ -0,0 +1,78 @@
> +<?xml version="1.0"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<bindings id="tabPanelBindings"
> +          xmlns="http://www.mozilla.org/xbl"
> +          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +          xmlns:xbl="http://www.mozilla.org/xbl">
> +
> +  <binding id="tabPanel">
> +    <resources>
> +      <stylesheet src="chrome://instantbird/content/tabpanel.css"/>
> +    </resources>

This included file no longer exists or never existed, depending on your point of view;)


Thanks for investing a lot of time and effort to get this right!
*** Original post on bio 426 at 2013-06-09 22:37:48 UTC ***

(In reply to comment #6)
> >>diff --git a/instantbird/content/tabbrowser.css b/instantbird/content/tabbrowser.css
> >>--- a/instantbird/content/tabbrowser.css
> >>+++ b/instantbird/content/tabbrowser.css
> >>@@ -48,8 +48,11 @@ tabconversation {
> >>+menuseparator+#context_tabSpecificSeparator {
> >>+  display: none;
> >>+}
> >
> >Good idea to hide it this way, we format it with spaces around selectors
> >though, i.e. "+" -> " + ".
> 
> Any reason a simple hidden="true" in the XBL definition of this separator
> doesn't work?

" + " is the adjacent sibling selector. This rule will only match when the separators are consecutive.
Depends on: 955432
*** Original post on bio 426 at 2013-06-10 09:09:31 UTC ***

> > diff --git a/instantbird/content/convZoom.js b/instantbird/content/convZoom.js
> > --- a/instantbird/content/convZoom.js
> > +++ b/instantbird/content/convZoom.js
> > @@ -76,16 +76,19 @@ var FullZoom = {
> >     * Set the zoom level for the current browser.
> >     *
> >     * Per nsPresContext::setFullZoom, we can set the zoom to its current value
> >     * without significant impact on performance, as the setting is only applied
> >     * if it differs from the current setting.  In fact getting the zoom and then
> >     * checking ourselves if it differs costs more.
> >     **/
> >    applyPrefValue: function FullZoom_applyPrefValue() {
> > +    if (!getBrowser().selectedBrowser) {
> > +      return;
> > +    }
> 
> getBrowser() as it is is misleading. Please make it return selectedBrowser
> directly and add a new getTabBrowser() method in instantbird.js for the other
> cases (see below, there's more on this topic). You can start here by looking
> for getBrowser() with lxr and decide in very case if the selectedBrowser or
> only the tabbrowser is needed. The log viewer also has a getBrowser method that
> would need changes then.

I'll look into this for the next patch.

> > diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
> > --- a/instantbird/content/conversation.xml
> > +++ b/instantbird/content/conversation.xml
> > @@ -48,16 +48,29 @@
> >            </xul:vbox>
> >          </xul:hbox>
> >          <xul:splitter class="splitter" anonid="splitter-bottom"/>
> >          <xul:vbox anonid="conv-bottom" class="conv-bottom">
> >            <xul:textbox anonid="inputBox" class="conv-textbox" multiline="true" flex="1"
> >                         onclick = "delete document.getBindingParent(this)._completions;"/>
> >          </xul:vbox>
> >        </xul:vbox>
> > +      <xul:menupopup id="convContextMenu">
> > +        <xul:menuitem id="context_showLogs" accesskey="&contextShowLogs.accesskey;" label="&contextShowLogs.label;"
> > +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> > +                                 tabbrowser.mContextTab.linkedTabPanel.showLogs();"/>
> > +        <xul:menuitem id="context_hideConv" accesskey="&contextHideConv.accesskey;" label="&contextHideConv.label;"
> > +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> > +                                 tabbrowser.mContextTab.linkedTabPanel.hide();
> > +                                 tabbrowser.removeTab(tabbrowser.mContextTab);"/>
> > +        <xul:menuitem id="context_closeConv" accesskey="&contextCloseConv.accesskey;" label="&contextCloseConv.label;"
> > +                      oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
> > +                                 tabbrowser.mContextTab.linkedTabPanel.close();
> > +                                 tabbrowser.removeTab(tabbrowser.mContextTab);"/>
> > +      </xul:menupopup>
> > 
> 
> There's really no nicer way (i.e. getting rid of the parentNodes) to do this?
> :(

document.getElementById('conversations') should work, or perhaps get(Tab)Browser(). I've kept it the way it was already being done for consistency. I'll go ahead and change all occurrences, since it's probably for the best.

> >      <command id="cmd_find"
> > -             oncommand="document.getElementById('conversations').findbar.onFindCommand();"/>
> > +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> > +                        if (conv) conv.browser.findbar.onFindCommand();"/>
> >      <command id="cmd_findAgain"
> > -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(false);"/>
> > +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> > +                        if (conv) conv.browser.findbar.onFindAgainCommand(false);"/>
> >      <command id="cmd_findPrevious"
> > -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(true);"/>
> > +             oncommand="let conv = document.getElementById('conversations').selectedConversation;
> > +                        if (conv) conv.browser.findbar.onFindAgainCommand(true);"/>
> 
> Is the newly added ".browser" in there really correct?
>

Yes. It used to be the findbar property in tabbrowser, which forwarded the current browser's findbar. I'll change this to use tabbrowser's selectedBrowser though. In addition, this code apparently isn't working.

> > diff --git a/instantbird/content/tabbrowser.xml b/instantbird/content/tabbrowser.xml
> > --- a/instantbird/content/tabbrowser.xml
> > +++ b/instantbird/content/tabbrowser.xml
> > @@ -107,27 +96,24 @@
> >        </field>
> >        <field name="mTabContainer" readonly="true">
> >          document.getAnonymousElementByAttribute(this, "anonid", "tabcontainer");
> >        </field>
> >        <field name="mPanelContainer" readonly="true">
> >          document.getAnonymousElementByAttribute(this, "anonid", "panelcontainer");
> >        </field>
> >        <field name="mTabs" readonly="true">
> > -        this.mTabContainer.childNodes
> > +        this.mTabContainer.childNodes;
> 
> Why did you change this? I've never seen semi-colons in the definition of
> fields in a binding before.

I saw another field with a semicolon and thought that was the correct way. I'll change it back.

> > @@ -137,109 +123,109 @@
> >  #endif
> >        </field>
> >        <field name="_browsers">
> >          null
> >        </field>
> 
> This field and its getter "browsers" aren't really used and should go away. The
> places where "browsers" is used ("moveTab*") will need fixing anyways (I think
> (_)tabPanels is the choice there now).

Acknowledged, I'll look into this.

> >        <method name="updateTitlebar">
> 
> > -            if (this.mCurrentTab)
> > -              docTitle = this.mCurrentTab.getAttribute("label");
> > -
> > -            if (!docTitle)
> > -              docTitle = docElement.getAttribute("titledefault");
> > +            docTitle = this.mCurrentTab.getAttribute("label");
> 
> Do you have an idea what "titledefault" was? It's only mentioned here as it
> seems...

Honestly, I'm not quite sure. I don't know why it's needed either, this is why I removed it. If required, I can add it back.

> > -      <method name="updatePopupMenu">
> > +      <method name="popupMenuShowing">
> 
> > -            var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> > -            for (var i = 0; i < menuItems.length; i++)
> > -              menuItems[i].disabled = disabled;
> > -            document.getElementById("context_showLogs").disabled =
> > -              !this.mContextTab.linkedConversation.hasLogs();
> > +            var multipleTabMenuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> > +            for (var i = 0; i < multipleTabMenuItems.length; i++)
> > +              multipleTabMenuItems[i].disabled = disabled;
> 
> Use for ... of for consistency here?

Noted. I only changed the name of the variable, so didn't bother with the for loop :)

> > @@ -281,33 +267,33 @@
> >                                   KeyEvent.DOM_VK_LEFT, KeyEvent.DOM_VK_RIGHT];
> >  
> >              if (tabKeyCodes.indexOf(event.keyCode) != -1)
> >                return;
> >  
> >              // Focus the editbox and pass the key to it.
> >              event.preventDefault();
> >              event.stopPropagation();
> > -            this.mCurrentConversation.editor.focus();
> > +            this.selectedConversation.focus();
> 
> Focusing the conversation does more than just setting the focus to the editor.
> Have you checked for side-effects of this change?

Yes. It marks the conversation as read. I guess it's a waste, I only did this because I didn't like accessing the editor directly.

> > @@ -446,28 +433,73 @@
> > +      <method name="addTabPanelByBindingName">
> > +        <parameter name="aBindingName"/>
> > +        <body>
> > +          <![CDATA[
> > +            newTabPanel = document.createElementNS(
> > +                "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> > +                                              aBindingName);
> > +            this.addTabPanel(newTabPanel);
> > +          ]]>
> > +        </body>
> > +      </method>
> > +
> > +      <method name="addTabPanel">
> > +        <parameter name="aTabPanel"/>
> > +        <body>
> > +          <![CDATA[
> > +            // invalidate cache, because mTabContainer is about to change
> > +            this._browsers = null;
> > +            this._conversations = null;
> > +            this._tabPanels = null;
> > +
> > +            this.mPanelContainer.appendChild(aTabPanel);
> > +
> > +            var t = this.mTabContainer.addTab();
> > +            aTabPanel.tab = t;
> > +
> > +            this.setStripVisibilityTo(!(Services.prefs.getBoolPref("browser.tabs.autoHide") &&
> > +                mFirstTabIsDummy));
> 
> Flo suggested that we might be able to get rid of mFirstTabIsDummy. Is this not
> possible for some reason?

Currently I haven't yet experimented with this. When a new conversation window is opened a dummy conversation is automatically added - the suggestion was, since this will probably become an Awesometab in the future, the dummy tab concept could be discarded. Is it okay to look into this when I start working on Awesometab?

> > -            return this._conversations ||
> > -                   (this._conversations = Array.map(this.mTabs, function (tab) tab.linkedConversation));
> > +            if (!this._conversations) {
> > +              this._conversations = [];
> > +              for (let tab of this.mTabs) {
> > +                if (tab.linkedConversation)
> > +                  this._conversations.push(tab.linkedConversation);
> > +              }
> > +            }
> > +            return this._conversations;
> > +          ]]>
> > +        </getter>
> > +      </property>
> 
> We've used a combination of map() and filter() in similar situations elsewhere
> already. You might want to try something like map(function(t)
> t.linkedConversation || null).filter(function(i) i != null) here.

Noted.

> > @@ -1237,42 +1266,41 @@
> >        <!-- BEGIN FORWARDED BROWSER PROPERTIES.  IF YOU ADD A PROPERTY TO THE BROWSER ELEMENT
> >             MAKE SURE TO ADD IT HERE AS WELL. -->
> >  
> >        <property name="docShell"
> >        <property name="contentWindow"
> >        <property name="markupDocumentViewer"
> >        <property name="contentDocument"
> >        <property name="contentTitle"
> >        <property name="findbar"
> 
> These are the already mentioned forwarded properties that should be removed
> because tabbrowser shouldn't fake being a browser anymore.

I'll try to address this in the next patch. I left it as it is because it requires a bit more refactoring elsewhere, and I wanted to focus on getting new tabs to work :)

> > @@ -1393,37 +1421,36 @@
> >          tabbrowser: this,
> >          handleEvent: function handleEvent(aEvent)
> >            this.tabbrowser.mCurrentTab.switchingAwayFromTab()
> >        })]]>
> >        </field>
> >  
> >        <constructor>
> >          <![CDATA[
> > -          this.mCurrentConversation = this.mPanelContainer.firstChild;
> > -          this.mCurrentBrowser = this.mCurrentConversation.browser;
> >            this.mCurrentTab = this.mTabContainer.firstChild;
> > +          // First tab is always a conversation
> > +          this.mCurrentTab.linkedConversation = this.mCurrentTab.linkedTabPanel = this.mPanelContainer.firstChild;
> 
> Does this assumption hold when a non-conversation tab is moved into an own
> window?

Yes. I've tested this. Notice the addTabPanel method checks for the dummy and removes it if necessary. This isn't necessary for conversations, because the addConversation method checks for dummy and uses it if it exists. Again, this dummy concept can hopefully be discarded in the future.

> > diff --git a/instantbird/content/tabpanel.xml b/instantbird/content/tabpanel.xml
> > new file mode 100644
> > --- /dev/null
> > +++ b/instantbird/content/tabpanel.xml
> > @@ -0,0 +1,78 @@
> > +<?xml version="1.0"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> > +<bindings id="tabPanelBindings"
> > +          xmlns="http://www.mozilla.org/xbl"
> > +          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > +          xmlns:xbl="http://www.mozilla.org/xbl">
> > +
> > +  <binding id="tabPanel">
> > +    <resources>
> > +      <stylesheet src="chrome://instantbird/content/tabpanel.css"/>
> > +    </resources>
> 
> This included file no longer exists or never existed, depending on your point
> of view;)

Oops. :)
*** Original post on bio 426 at 2013-06-10 09:30:55 UTC ***

(In reply to comment #10)
> > >>@@ -48,8 +48,11 @@ tabconversation {
> > >>+menuseparator+#context_tabSpecificSeparator {
> > >>+  display: none;
> > >>+}
> > >
> > >Good idea to hide it this way, we format it with spaces around selectors
> > >though, i.e. "+" -> " + ".
> > 
> > Any reason a simple hidden="true" in the XBL definition of this separator
> > doesn't work?
> 
> " + " is the adjacent sibling selector. This rule will only match when the
> separators are consecutive.

The latest patch makes it nicely explicit *why* we are matching consecutive separators :)
*** Original post on bio 426 at 2013-06-10 10:36:35 UTC ***

(In reply to comment #11)

> > > -            this.mCurrentConversation.editor.focus();
> > > +            this.selectedConversation.focus();
> > 
> > Focusing the conversation does more than just setting the focus to the editor.
> > Have you checked for side-effects of this change?
> 
> Yes. It marks the conversation as read. I guess it's a waste, I only did this
> because I didn't like accessing the editor directly.

We purposefully don't mark conversations as read immediately when switching between tabs with some keyboard shortcuts.
*** Original post on bio 426 at 2013-06-10 10:37:55 UTC ***

(In reply to comment #11)

> > > +            this.setStripVisibilityTo(!(Services.prefs.getBoolPref("browser.tabs.autoHide") &&
> > > +                mFirstTabIsDummy));
> > 
> > Flo suggested that we might be able to get rid of mFirstTabIsDummy. Is this not
> > possible for some reason?
> 
> Currently I haven't yet experimented with this. When a new conversation window
> is opened a dummy conversation is automatically added - the suggestion was,
> since this will probably become an Awesometab in the future, the dummy tab
> concept could be discarded. Is it okay to look into this when I start working
> on Awesometab?

Yes.

Mic: I said we wouldn't need this any more once we are sure there's always at least one panel (and we will be sure of this once we have the awesometab).
*** Original post on bio 426 at 2013-06-10 10:50:27 UTC ***

A few questions:
- Why do we know need to mess with the collapsed property of the buddy icon in the tooltip?

- What's |aConversation.conv = null;| for in the finishImport method?

- I still greatly dislike the new tabPanel empty biding. If we kept it, I think it would go in the tabbrowser.xml file rather than in a new file. Its id would probably be tabbrowser-tabpanel or tabbrowser-panel.
*** Original post on bio 426 at 2013-06-10 10:52:43 UTC ***

Are you testing this with the JS-strict pref set? Code like
> let conv = this.selectedConversation;
and
> return this.mCurrentTab.linkedConversation;
will cause reference errors. Decide whether properties like selectedConversation which may not exist should be null or undefined in that case, and stick to it. (null would have the advantage that the value is then defined for any code using it.)

> > This field and its getter "browsers" aren't really used and should go away. The
> > places where "browsers" is used ("moveTab*") will need fixing anyways (I think
> > (_)tabPanels is the choice there now).
> 
> Acknowledged, I'll look into this.

Generally we try to use _xxx for "private" variables internal to the object, and xxx for variables that we expect to be accessed from outside.

> > >              // Focus the editbox and pass the key to it.
> > >              event.preventDefault();
> > >              event.stopPropagation();
> > > -            this.mCurrentConversation.editor.focus();
> > > +            this.selectedConversation.focus();
> > 
> > Focusing the conversation does more than just setting the focus to the editor.
> > Have you checked for side-effects of this change?
> 
> Yes. It marks the conversation as read. I guess it's a waste, I only did this
> because I didn't like accessing the editor directly.

Please don't make any changes to when things are marked read etc in this patch! The way all that behaviour works is nontrivial. If something can be optimized there it should be done in a followup.

> Currently I haven't yet experimented with this. When a new conversation window
> is opened a dummy conversation is automatically added - the suggestion was,
> since this will probably become an Awesometab in the future, the dummy tab
> concept could be discarded. Is it okay to look into this when I start working
> on Awesometab?

Maybe just check if exposing the "+" tab actually adds a tab that is then always present, or whether the "+" is really just a button. That should tell you what you need to know.
Comment on attachment 8354244 [details] [diff] [review]
Updated patch, covering all needed changes to fix this bug (I think)

*** Original change on bio 426 attmnt 2477 at 2013-06-10 11:25:48 UTC ***

> Maybe just check if exposing the "+" tab actually adds a tab that is then
> always present, or whether the "+" is really just a button. That should tell
> you what you need to know.

And even if the "+ tab" still looks like a tab at all... I think FX may have changed that a while ago (or at least I never see it for some reason).


Do we really need tabs to have tooltips that just contain the label of the tab, and no actual extra info? I think no tooltip at all would be better in that case - it's just visual noise.

>           let tabSpecificStartSeparator = document.getElementById('context_tabSpecificStartSeparator');
>           let tabSpecificEndSeparator = document.getElementById('context_tabSpecificEndSeparator');
>           let prevItem = tabSpecificEndSeparator.previousSibling;
>           while (prevItem != tabSpecificStartSeparator) {
>             aPopupMenu.removeChild(prevItem);
>             prevItem = tabSpecificEndSeparator.previousSibling;
>           }

As I suggested in an earlier comment, try
let range = document.createRange();
range.setStartAfter(tabSpecificStartSeparator);
range.setEndBefore(tabSpecificEndSeparator);
range.deleteContents();

>            var newTabPanel = aOtherTab.linkedTabPanel.conv ?
>                     this._addConversation(aOtherTab.linkedTabPanel.conv) :
>                     this.addTabPanel(aOtherTab.linkedTabPanel)

No doubt this works, but didn't you add an API to check whether a given panel is a conversation? Or is there a reason we are not using it here and looking directly for the existence of the conv property? If so, a comment would be good.

Is there any reason for the conversation binding to extend the tabpanel binding other than "it's what we would do if we were doing OOP"? All the methods are overridden.

>onSelect and handleSwitchingAwayFromTab sound like they could be handled by
>forwarding the corresponding event (which a panel can then listen to or not). 
>
>For getPanelSpecificMenuItems(), how about making it addPanelMenuitems(),
>letting the panel code add whatever menuitems it wants directly?

What were your thoughts on this?

If the tabpanel binding is going to be used mainly as a template for documentation of the API, comments on what to do with it's non-obvious methods would be useful.
Attachment #8354244 - Flags: feedback?(aleth) → feedback+
Summary: Make it easier to add tabs with arbitrary content to the "conversation window" → Add support for tabs with arbitrary content in the conversation window
Whiteboard: [nice-to-have]
*** Original post on bio 426 at 2013-06-10 11:51:45 UTC ***

(In reply to comment #17)

> Do we really need tabs to have tooltips that just contain the label of the tab,
> and no actual extra info? I think no tooltip at all would be better in that
> case - it's just visual noise.

Firefox has them. The point is to show the whole tab title if it's been cropped because you have many tabs and the tabs are too small.
Attached patch Address comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2479 at 2013-06-11 00:57:00 UTC ***

Here's an updated patch! I don't think there's much left to be done aside from needing some comments (I may be wrong), so I'm requesting review.
Attachment #8354246 - Flags: review?(benediktp)
Attachment #8354246 - Flags: feedback?(aleth)
Attachment #8354246 - Flags: feedback?(florian)
Comment on attachment 8354244 [details] [diff] [review]
Updated patch, covering all needed changes to fix this bug (I think)

*** Original change on bio 426 attmnt 2477 at 2013-06-11 00:57:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354244 - Attachment is obsolete: true
Comment on attachment 8354246 [details] [diff] [review]
Address comments

*** Original change on bio 426 attmnt 2479 at 2013-06-11 12:40:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354246 - Flags: feedback?(aleth) → feedback+
*** Original post on bio 426 at 2013-06-11 12:40:09 UTC ***

Comment on attachment 8354246 [details] [diff] [review] (bio-attmnt 2479)
Address comments

>     <content>
>+      <xul:stringbundle anonid="convstringbundle" src="chrome://instantbird/locale/conversation.properties"/>

As you are adding a l10n file, can we drop the dependence of conversation.xml on instantbird.dtd and move the single string used from there to it too?

>+ <field name="mStringBundle">

As discussed on IRC, the mXxx convention is only used in tabbrowser.xml, and there only for variables which correspond to those inherited from the FX browser.xml. Please use _stringBundle or just stringBundle for conversation.xml ;)

>+          aConversation.conv = null;

Please add an explanatory comment here. (I'd generally encourage you to go through and add comments wherever you were initially confused when encountering the code.)

>+            const XUL_NS =
>+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+
>+            let showLogsItem = document.createElementNS(XUL_NS, "menuitem");
>+            showLogsItem.setAttribute("accesskey",
>+              this.mStringBundle.getString("contextShowLogs.accesskey"));
>+            showLogsItem.setAttribute("label",
>+              this.mStringBundle.getString("contextShowLogs.label"));
>+            showLogsItem.addEventListener("command", function(aEvent) {
>+              document.getBindingParent(aEvent.target)
>+                      .linkedConversation.showLogs();
>+            });

There is a lot of duplication here, making this hard to read. If you are going to generate these items dynamically, rather than copying them from the <content>, how about using a local helper function e.g something like

function createMenuItem(aAccessKeyId, aLabelId, aCommandHandler) {
  let item = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem");
  item.setAttribute("accesskey", this.mStringBundle.getString(aAccessKeyId));
  item.setAttribute("label", this.mStringBundle.getString(aLabelId));
  item.addEventListener("command", aCommandHandler);
  return item;
};

>+    <command id="cmd_showLogs"
>+             oncommand="let conv = getTabBrowser().selectedConversation;
>+                        if (conv) conv.showLogs();"/>

Why does the show logs tab menu entry in conversation.xml not use this command instead of having its own handler?

>+                  onselect="if (!('updateCurrentBrowser' in document.getBindingParent(this)) || event.target.localName != 'tabpanels') return;
>+                            document.getBindingParent(this).updateCurrentBrowser();">

This will be more readable and have less duplication if you store document.getBindingParent(this) in a local variable whose name explains what it is ;)

>+          <xul:menupopup id="tabContextMenu" onpopupshowing="return document.getBindingParent(this).popupMenuShowing(this);"
>+                  onpopuphiding="return document.getBindingParent(this).popupMenuHiding(this)">

Can we call these handlers tabContextMenuShowing/Hiding ? That's more specific than "popup".

>       <method name="onTabKeypress">
>         <parameter name="event"/>
>         <body>
>           <![CDATA[
>@@ -281,33 +264,33 @@
>                                  KeyEvent.DOM_VK_LEFT, KeyEvent.DOM_VK_RIGHT];
> 
>             if (tabKeyCodes.indexOf(event.keyCode) != -1)
>               return;
> 
>             // Focus the editbox and pass the key to it.
>             event.preventDefault();
>             event.stopPropagation();
>-            this.mCurrentConversation.editor.focus();
>+            this.selectedConversation.editor.focus();

Doesn't this (and the subsequent) code assume the tab is a conversation tab? Where do you check for this?

>       <method name="handleEvent">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>           switch (aEvent.type) {
>             case "sizemodechange":
>               if (aEvent.target == window) {
>-                this.mCurrentBrowser.docShell.isActive =
>+                this.selectedBrowser.docShell.isActive =

Does this event handler never get called when selectedBrowser is null?

>+          // First tab is always a conversation

Why? ;) Please expand the comment.

>-          this._autoScrollPopup = this.mCurrentBrowser._createAutoScrollPopup();
>+          this._autoScrollPopup = this.selectedBrowser._createAutoScrollPopup();

Naming question for flo: Should we call this variable mCurrentBrowser (parallel to FX I suppose?) or selectedBrowser? Ditto for selectedConversation vs mCurrentConversation. (I suppose the 'm' would be incorrect as these vars are not just used privately.)
*** Original post on bio 426 at 2013-06-11 13:27:27 UTC ***

> >+          aConversation.conv = null;
> 
> Please add an explanatory comment here. (I'd generally encourage you to go
> through and add comments wherever you were initially confused when encountering
> the code.)

I'm not sure I know why this is done! It was done previously in tabbrowser, I just moved it to the conversation binding without thinking about it. I'll take a look though.

> >+            const XUL_NS =
> >+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+
> >+            let showLogsItem = document.createElementNS(XUL_NS, "menuitem");
> >+            showLogsItem.setAttribute("accesskey",
> >+              this.mStringBundle.getString("contextShowLogs.accesskey"));
> >+            showLogsItem.setAttribute("label",
> >+              this.mStringBundle.getString("contextShowLogs.label"));
> >+            showLogsItem.addEventListener("command", function(aEvent) {
> >+              document.getBindingParent(aEvent.target)
> >+                      .linkedConversation.showLogs();
> >+            });
> 
> There is a lot of duplication here, making this hard to read. If you are going
> to generate these items dynamically, rather than copying them from the
> <content>, how about using a local helper function e.g something like [...]

That makes sense, thanks.

> >+    <command id="cmd_showLogs"
> >+             oncommand="let conv = getTabBrowser().selectedConversation;
> >+                        if (conv) conv.showLogs();"/>
> 
> Why does the show logs tab menu entry in conversation.xml not use this command
> instead of having its own handler?

Because the command uses the selectedConversation, whereas the context menu need not be for the selected one.

> >       <method name="onTabKeypress">
> >         <parameter name="event"/>
> >         <body>
> >           <![CDATA[
> >@@ -281,33 +264,33 @@
> >                                  KeyEvent.DOM_VK_LEFT, KeyEvent.DOM_VK_RIGHT];
> > 
> >             if (tabKeyCodes.indexOf(event.keyCode) != -1)
> >               return;
> > 
> >             // Focus the editbox and pass the key to it.
> >             event.preventDefault();
> >             event.stopPropagation();
> >-            this.mCurrentConversation.editor.focus();
> >+            this.selectedConversation.editor.focus();
> 
> Doesn't this (and the subsequent) code assume the tab is a conversation tab?
> Where do you check for this?

Ah. I suppose this was the reason I had used this.selectedConversation.focus() previously - all tab panels are guaranteed to have a focus method.

> >       <method name="handleEvent">
> >         <parameter name="aEvent"/>
> >         <body><![CDATA[
> >           switch (aEvent.type) {
> >             case "sizemodechange":
> >               if (aEvent.target == window) {
> >-                this.mCurrentBrowser.docShell.isActive =
> >+                this.selectedBrowser.docShell.isActive =
> 
> Does this event handler never get called when selectedBrowser is null?

I seem to have missed this, thanks.

> >+          // First tab is always a conversation
> 
> Why? ;) Please expand the comment.

It's always the dummy conversation tab, I thought it would be apparent to someone reading this code but sure.
Attached patch Some bugfixes and comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2480 at 2013-06-13 05:18:00 UTC ***

I've added some comments as well as fixed some bugs that I missed.
Attachment #8354247 - Flags: review?(benediktp)
Attachment #8354247 - Flags: feedback?(florian)
Attachment #8354247 - Flags: feedback?(aleth)
Comment on attachment 8354246 [details] [diff] [review]
Address comments

*** Original change on bio 426 attmnt 2479 at 2013-06-13 05:18:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354246 - Attachment is obsolete: true
Attachment #8354246 - Flags: review?(benediktp)
Attachment #8354246 - Flags: feedback?(florian)
Attached patch Some bugfixes and comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2481 at 2013-06-13 05:25:00 UTC ***

Sorry for posting another patch so soon. I found a nit and thought I'd fix it.

Also I forgot to mention, I've renamed updateCurrentBrowser to updateCurrentTab, since the current tab panel needn't contain a browser anymore.
Attachment #8354248 - Flags: review?(benediktp)
Attachment #8354248 - Flags: feedback?(florian)
Attachment #8354248 - Flags: feedback?(aleth)
Comment on attachment 8354247 [details] [diff] [review]
Some bugfixes and comments

*** Original change on bio 426 attmnt 2480 at 2013-06-13 05:25:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354247 - Attachment is obsolete: true
Attachment #8354247 - Flags: review?(benediktp)
Attachment #8354247 - Flags: feedback?(florian)
Attachment #8354247 - Flags: feedback?(aleth)
Blocks: 955438
*** Original post on bio 426 at 2013-06-13 10:52:41 UTC ***

The patch's not reviewed yet but I've started doing test and while it works great most of the time there are a few problems that we need to tackle.

There was the following error that I didn't know how to reproduce (maybe using Ctrl+Shift+H (on Windows)for the first time?):
> Timestamp: 13.06.2013 12:22:16
> Error: SyntaxError: missing ; before statement
> Source File: chrome://instantbird/content/instantbird.xul
> Line: 1, Column: 4
> Source Code:
> let conv = getTabBrowser().selectedConversation;


I'm getting this one frequently:
> Timestamp: 13.06.2013 12:09:55
> Error: [Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://instantbird/content/conversation.xml :: finishImport :: line 163"  data: no]
> Source File: chrome://instantbird/content/conversation.xml
> Line: 163


Pressing Enter when the tab is focused (dotted outline around the label of the tab) gives:
> Timestamp: 13.06.2013 12:11:53
> Error: TypeError: this.selectedConversation is null
> Source File: chrome://instantbird/content/tabbrowser.xml
> Line: 265


Trying to "Close other tabs" from the context menu of any tab (conversation or not) fails:
> Timestamp: 13.06.2013 12:18:11
> Error: TypeError: this.mTabs.map is not a function
> Source File: chrome://instantbird/content/tabbrowser.xml
> Line: 835


Keyboard shortcuts Ctrl+<n> with n being any of 1-9 to switch to the n-th (or last) tab do not work any more. No errors on the console.


Keyboard shortcut to open chat logs from a conversation (Ctrl+Shift+H on Windows) does not work any more. No errors on the console.


(Not reliably reproducable) When switching from a conversation tab to a non-conversation tab using Ctrl+PageUp/PageDown:
> Timestamp: 13.06.2013 12:27:56
> Error: [Exception... "'JavaScript component does not have a method named: "supportsCommand"' when calling method: [nsIController::supportsCommand]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 71"  data: no]
> Source File: chrome://global/content/globalOverlay.js
> Line: 71


(Not reliably reproducable) Using Ctrl+Home/End:
> Timestamp: 13.06.2013 12:30:04
> Error: [Exception... "'JavaScript component does not have a method named: "supportsCommand"' when calling method: [nsIController::supportsCommand]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://instantbird/content/conversation.xml :: inputKeyPress :: line 520"  data: no]
> Source File: chrome://instantbird/content/conversation.xml
> Line: 520


Closing a window with a conversation and several tabtest-tabs: 
> Timestamp: 13.06.2013 12:31:49
> Error: TypeError: this.mTabs.map is not a function
> Source File: chrome://instantbird/content/tabbrowser.xml
> Line: 835


What should happen if tabbed conversations aren't enabled in the preferences? The new tab will open in the same window anyways at the moment.


Hiding the tabbar (using the "Always show the tab bar"-preference) and creating a tabtest-tab doesn't show the tab until one toggles the pref again and the error console shows:
> Timestamp: 13.06.2013 12:34:52
> Error: NS_ERROR_XPC_JS_THREW_JS_OBJECT: 'ReferenceError: mFirstTabIsDummy is not defined' when calling method: [imICommand::run]
> Source File: file:///C:/Programme/Internet/Instantbird%20-%20Awesometab%202013-06-11/components/imCommands.js
> Line: 229
*** Original post on bio 426 at 2013-06-13 10:54:38 UTC ***

Comment on attachment 8354248 [details] [diff] [review] (bio-attmnt 2481)
Some bugfixes and comments

>+            let showLogsItem = createMenuItem("contextShowLogs.label",
>+              "contextShowLogs.accesskey", function(aEvent) conv.showLogs());
>+            showLogsItem.disabled = !this.hasLogs();
>+            items.push(showLogsItem);

Where is conv set? (Ditto for the other handlers)
Comment on attachment 8354248 [details] [diff] [review]
Some bugfixes and comments

*** Original change on bio 426 attmnt 2481 at 2013-06-13 11:16:50 UTC ***

onTabKeyPress still looks broken (see feedback comments on the previous patch). We should probably move some of that to conversation.xml.
Attachment #8354248 - Flags: feedback?(aleth) → feedback+
*** Original post on bio 426 at 2013-06-13 11:37:47 UTC ***

(In reply to comment #26)
> onTabKeyPress still looks broken (see feedback comments on the previous patch).
> We should probably move some of that to conversation.xml.
This referred to an older version of the patch, sorry.
Attached patch Some more fixes as per comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2488 at 2013-06-14 19:36:00 UTC ***

Hopefully this patch should eliminate those errors.
Attachment #8354255 - Flags: review?(benediktp)
Attachment #8354255 - Flags: feedback?(florian)
Attachment #8354255 - Flags: feedback?(aleth)
Comment on attachment 8354248 [details] [diff] [review]
Some bugfixes and comments

*** Original change on bio 426 attmnt 2481 at 2013-06-14 19:36:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354248 - Attachment is obsolete: true
Attachment #8354248 - Flags: review?(benediktp)
Attachment #8354248 - Flags: feedback?(florian)
Attached patch Add patch info (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2489 at 2013-06-14 19:40:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354256 - Flags: review?(benediktp)
Attachment #8354256 - Flags: feedback?(florian)
Attachment #8354256 - Flags: feedback?(aleth)
Comment on attachment 8354255 [details] [diff] [review]
Some more fixes as per comments

*** Original change on bio 426 attmnt 2488 at 2013-06-14 19:40:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354255 - Attachment is obsolete: true
Attachment #8354255 - Flags: review?(benediktp)
Attachment #8354255 - Flags: feedback?(florian)
Attachment #8354255 - Flags: feedback?(aleth)
*** Original post on bio 426 at 2013-06-15 00:13:40 UTC ***

Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489)
Add patch info

>diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml

>@@ -158,16 +158,18 @@
>            img.parentNode.collapsed = true;
>        ]]>
>        </body>
>      </method>
> 
>      <method name="reset">
>        <body>
>        <![CDATA[
>+         document.getAnonymousElementByAttribute(this, "anonid", "prplTooltipBuddyIcon")
>+                  .collapsed = false;

Nit: This isn't aligned correctly.

>          if (localName == "tab") {
>-           let conv = elt.linkedConversation.conv;
>-           if (conv)
>-             return updateTooltipFromConversation(conv, elt);
>-           return false;
>+           return elt.linkedConversation ?
>+             updateTooltipFromConversation(elt.linkedConversation.conv, elt) :
>+             updateTooltipFromTab(elt);
>          }

Nit: this would be more readable:
if (elt.linkedConversation)
  return updateTooltipFromConversation(...
return updateTooltipFromTab(...


>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
>--- a/instantbird/content/conversation.xml
>+++ b/instantbird/content/conversation.xml
>@@ -14,16 +14,17 @@
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="conversation">
>     <resources>
>       <stylesheet src="chrome://instantbird/skin/conversation.css"/>
>     </resources>
>     <content>
>+      <xul:stringbundle anonid="convstringbundle" src="chrome://instantbird/locale/conversation.properties"/>

This file is used only when opening a context menu, so it would be sad to load it whenever a conversation is displayed. Please remove this and use Services.strings.createBundle(url) instead in getPanelSpecificMenuItems.

>@@ -84,16 +85,20 @@
>       </constructor>
> 
>       <destructor>
>         <![CDATA[
>           this.destroy();
>         ]]>
>       </destructor>
> 
>+      <field name="stringBundle">
>+        document.getAnonymousElementByAttribute(this, "anonid", "convstringbundle");
>+      </field>

Remove this too.

>       <method name="finishImport">
>         <parameter name="aConversation"/>
>         <body>
>         <![CDATA[
>+          // Swap the docshells
>+          this.browser.swapDocShells(aConversation.browser);
>+          // Invalidate previous conversation's conv - it's with us now.

"Invalidate previous conversation's conv" is the kind of meaningless comments I hate reading when (re)discovering code.

>+          aConversation.conv = null;

From what I remember (I haven't checked), the point of setting this to null is to remove observers.


>+      <method name="handleSwitchingAwayFromTab">
>+        <parameter name="aTabWasVisible"/>
>+        <body>
>+          <![CDATA[
>+            // Remove the unread ruler if the tab has been visible without
>+            // interruptions for sufficiently long.

This comment wants to be just before "if (aTabWasVisible)"

>+            this.browser.docShell.isActive = false;
>+            // Make sure editor stops receiving input
>+            this.editor.blur();
>+            if (aTabWasVisible)
>+              this.browser.removeUnreadRuler();
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="getPanelSpecificMenuItems">
>+        <body>
>+          <![CDATA[
>+            let items = [];
>+            const XUL_NS =
>+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+
>+            let conv = this;
>+            function createMenuItem(aLabelId, aAccessKeyId, aCommandHandler) {

You can reduce code duplication if you replace aLabelId and aAccessKeyId by just aId, and append ".label"/".acccesskey" to the string id to get the localizable string.

>+              let item = document.createElementNS(XUL_NS, "menuitem");
>+              item.setAttribute("label",
>+                conv.stringBundle.getString(aLabelId));
>+              item.setAttribute("accesskey",
>+                conv.stringBundle.getString(aAccessKeyId));
>+              item.addEventListener("command", aCommandHandler);
>+              return item;
>+            }
>+            let showLogsItem = createMenuItem("contextShowLogs.label",
>+              "contextShowLogs.accesskey", function(aEvent) conv.showLogs());
>+            showLogsItem.disabled = !this.hasLogs();
>+            items.push(showLogsItem);
>+
>+            let hideConvItem = createMenuItem("contextHideConv.label",
>+              "contextHideConv.accesskey", function(aEvent) {
>+                conv.hide();
>+                document.getBindingParent(conv).removeTab(conv.tab);
>+            });
>+            items.push(hideConvItem);
>+
>+            let closeConvItem = createMenuItem("contextCloseConv.label",
>+              "contextCloseConv.accesskey", function(aEvent) {
>+                conv.close();
>+                document.getBindingParent(conv).removeTab(conv.tab);
>+            });
>+            items.push(closeConvItem);
>+
>+            return items;
>+          ]]>
>+        </body>
>+      </method>

>diff --git a/instantbird/content/instantbird.js b/instantbird/content/instantbird.js
>--- a/instantbird/content/instantbird.js
>+++ b/instantbird/content/instantbird.js
>@@ -10,17 +10,17 @@ var convWindow = {
>     Components.utils.import("resource:///modules/imWindows.jsm");
>     Conversations.registerWindow(window);
> 
>     if ("arguments" in window) {
>       while (window.arguments[0] instanceof XULElement) {
>         // swap the given tab with the default dummy conversation tab
>         // and then close the original tab in the other window.
>         let tab = window.arguments.shift();
>-        document.getElementById("conversations").importConversation(tab);
>+        document.getElementById("conversations").importTabPanel(tab);

I'm not satisfied by the importTabPanel method, but will need to think more about it to offer useful improvement suggestions.

> function getConvWindowURL() "chrome://instantbird/content/instantbird.xul"
> 
>+function getTabBrowser()
>+{
>+  return document.getElementById("conversations");
>+}
>+
> function getBrowser()
> {
>-  return document.getElementById("conversations");
>+  return getTabBrowser().selectedBrowser;
> }

Nit: You can simplify:

function getTabBrowser() document.getElementById("conversations")
function getBrowser() getTabBrowser().selectedBrowser



I haven't reviewed the changes to tabbrowser.xml yet. Changes to other files on which I haven't commented seemed good.
Comment on attachment 8354256 [details] [diff] [review]
Add patch info

*** Original change on bio 426 attmnt 2489 at 2013-06-18 22:29:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354256 - Flags: review-
Attachment #8354256 - Flags: feedback?(florian)
Attachment #8354256 - Flags: feedback+
*** Original post on bio 426 at 2013-06-18 22:29:40 UTC ***

Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489)
Add patch info

>-      <method name="updatePopupMenu">
>+      <method name="tabContextMenuShowing">

>+            let tabSpecificEndSeparator
>+              = document.getElementById("context_tabSpecificEndSeparator");

Nit: let tabSpecificEndSeparator = (line break after the '=' rather than before.)

>+      <method name="updateCurrentTab">

>+            if (this.selectedBrowser)
>+              this.mCurrentTab.linkedBrowser.docShell.isActive =
>+                (window.windowState != window.STATE_MINIMIZED);

nit: You need some {} if the instruction after the if is on more than 1 line.

>@@ -255,59 +257,60 @@
>               // the user stays on the tab for more than a moment, to prevent
>               // tabs from being marked as read if the user is just scrolling
>               // past them with the arrow keys. 400ms is between "200ms - a bit
>               // too quick for people who repeat-keypress more slowly than me"
>               // and "600ms - a bit too noticeable already".
>               if (this._tabSelectTimer)
>                 clearTimeout(this._tabSelectTimer);
>               this._tabSelectTimer = setTimeout(function() {
>-                this.mCurrentConversation.onSelect();
>+                this.selectedTabPanel.onSelect();

Couldn't this just be this.selectedPanel rather than selectedTabPanel?



>       <method name="_addConversation">

This looks like it really want to call your new add(Tab)Panel method. Please save us from this code duplication! ;)


>+      <method name="addTabPanelFromBindingName">

I think we neither want nor need this method.

>+      <method name="addTabPanel">

Can't this just be named addPanel?


>@@ -567,20 +613,20 @@
>         <parameter name="aTab"/>
>         <parameter name="aTabWillBeMoved"/>
>         <parameter name="aCloseWindowFastpath"/>
>         <body>
>           <![CDATA[
>             if (this._removingTabs.indexOf(aTab) > -1 || this._windowIsClosing)
>               return null;
> 
>-            var browser = this.getBrowserForTab(aTab);
>+            var conv = aTab.linkedConversation;

Shouldn't this be browser = aTab.linkedBrowser?


>             // This will unload the document. An unload handler could remove
>             // dependant tabs, so it's important that the tabbrowser is now in
>             // a consistent state (tab removed, tab positions updated, etc.).
>             // Also, it's important that another tab has been selected before
>             // the panel is removed; otherwise, a random sibling panel can flash.
>-            this.mPanelContainer.removeChild(conversation);
>+            // The parentNode check is required because this fails in case of the dummy tab.
>+            if (tabPanel.parentNode == this.mPanelContainer)
>+              this.mPanelContainer.removeChild(tabPanel);

Why would any code try to remove the dummy tab?

>-      <method name="importConversation">
>+      <method name="importTabPanel">

What about 'importPanel'?


>+            // Create a new tab panel of the same type as the one in the other window
>+            var newTabPanel = aOtherTab.linkedConversation ?
>+                      this._addConversation(aOtherTab.linkedConversation.conv) :

This indent is wrong. Indent 2 spaces.

>+                      this.addTabPanelFromBindingName(aOtherTab.linkedTabPanel.nodeName);

Would this.addPanel(aOtherTab.linkedTabPanel.cloneNode()) work for the generic case?


>+<!-- Use as a template for custom tab panels -->
>+  <binding id="tabbrowser-panel">

I still don't see the point of this binding. It looks like we can easily get rid of it by adding 6 null checks in the rest of the code, so we should just do it.

Then for the documentation purpose, write a nice comment before the addPanel method, and include the syntax of each of these 6 methods in a much more concise way (I would suggest using a syntax similar to what we have in .idl files).


>+      <!-- Called when switching away from this tab -->
>+      <method name="handleSwitchingAwayFromTab">

I suspect we can find a better and shorter name for this. How does onDeselect sound?
Comment on attachment 8354256 [details] [diff] [review]
Add patch info

*** Original change on bio 426 attmnt 2489 at 2013-06-19 09:06:52 UTC ***

flo has already covered all the things I wanted to say (and more) ;)
Attachment #8354256 - Flags: feedback?(aleth) → feedback+
Comment on attachment 8354256 [details] [diff] [review]
Add patch info

*** Original change on bio 426 attmnt 2489 at 2013-06-19 21:00:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354256 - Flags: review?(benediktp) → review-
*** Original post on bio 426 at 2013-06-19 21:00:46 UTC ***

Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489)
Add patch info

Flo suggested to change some names (e.g. importTabPanel to importPanel) but what about renaming *TabPanel to *Panel (almost) everywhere?

> --- a/instantbird/content/convZoom.js
> +++ b/instantbird/content/convZoom.js
> @@ -76,16 +76,19 @@ var FullZoom = {
>     * Set the zoom level for the current browser.
>     *
>     * Per nsPresContext::setFullZoom, we can set the zoom to its current value
>     * without significant impact on performance, as the setting is only applied
>     * if it differs from the current setting.  In fact getting the zoom and then
>     * checking ourselves if it differs costs more.
>     **/
>    applyPrefValue: function FullZoom_applyPrefValue() {
> +    // If there's no browser (non-conversation tabs), don't do anything.
> +    if (!getBrowser())
> +      return;
>      let value = parseFloat(Services.prefs.getCharPref(FullZoom.prefName));
>      if (isNaN(value))
>        value = 1;
>      else if (value < ZoomManager.MIN)
>        value = ZoomManager.MIN;
>      else if (value > ZoomManager.MAX)
>        value = ZoomManager.MAX;
>      ZoomManager.zoom = value;
> diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
> --- a/instantbird/content/conversation.xml
> +++ b/instantbird/content/conversation.xml
> @@ -1481,16 +1490,72 @@
>          <body>
>          <![CDATA[
>            this.editor.focus();
>            this.onSelect();
>          ]]>
>          </body>
>        </method>
>  
> +      <method name="handleSwitchingAwayFromTab">
> +        <parameter name="aTabWasVisible"/>
> +        <body>
> +          <![CDATA[
> +            // Remove the unread ruler if the tab has been visible without
> +            // interruptions for sufficiently long.
> +            this.browser.docShell.isActive = false;
> +            // Make sure editor stops receiving input
> +            this.editor.blur();
> +            if (aTabWasVisible)
> +              this.browser.removeUnreadRuler();
> +          ]]>
> +        </body>
> +      </method>

I'm still not convinced that non-conversation tabs will need the "tab was visible" functionality. Maybe it should be a feature of conversation tabs only and the "was visible"-code moved from the tabbrowser-tab implemention to the conv-tab implementation as a consequence?

> +      <method name="getPanelSpecificMenuItems">
> +        <body>
> +          <![CDATA[
> +            let items = [];
> +            const XUL_NS =
> +              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +            let conv = this;
> +            function createMenuItem(aLabelId, aAccessKeyId, aCommandHandler) {
> +              let item = document.createElementNS(XUL_NS, "menuitem");
> +              item.setAttribute("label", conv.stringBundle.getString(aLabelId));
> +              item.setAttribute("accesskey",
> +                conv.stringBundle.getString(aAccessKeyId));

I wouldn't mind breaking the 80 char limit at a few places if it increases readability of the code. Shortening variable names could help to reduce the number of excess characters though. This applies to some other places further down too.

> +              item.addEventListener("command", aCommandHandler);
> +              return item;
> +            }
> +            let showLogsItem = createMenuItem("contextShowLogs.label",
> +              "contextShowLogs.accesskey", function(aEvent) conv.showLogs());
> +            showLogsItem.disabled = !this.hasLogs();
> +            items.push(showLogsItem);
> +
> +            let hideConvItem = createMenuItem("contextHideConv.label",
> +              "contextHideConv.accesskey", function(aEvent) {
> +                conv.hide();
> +                document.getBindingParent(conv).removeTab(conv.tab);
> +            });
> +            items.push(hideConvItem);
> +
> +            let closeConvItem = createMenuItem("contextCloseConv.label",
> +              "contextCloseConv.accesskey", function(aEvent) {
> +                conv.close();
> +                document.getBindingParent(conv).removeTab(conv.tab);
> +            });
> +            items.push(closeConvItem);

I don't like the indentation as it doesn't help to understand the code in my opinion. flo's suggestion to pass only the key of the item and appending ".id" and ".accesskey" sounds good as it will reduce the parameters for each createMenuItem call and might help to format the other code in a nicer way. Maybe like this? :

> +            let closeConvItem = createMenuItem("contextCloseConv", function(aEvent) {
> +                conv.close();
> +                document.getBindingParent(conv).removeTab(conv.tab);
> +            });
> +            items.push(closeConvItem);



> diff --git a/instantbird/content/instantbird.js b/instantbird/content/instantbird.js
>          // swap the given tab with the default dummy conversation tab
>          // and then close the original tab in the other window.
>          let tab = window.arguments.shift();
> -        document.getElementById("conversations").importConversation(tab);
> +        document.getElementById("conversations").importTabPanel(tab);
>        }
>      }

Shouldn't we use getTabBrowser() here?

> diff --git a/instantbird/content/instantbird.xul b/instantbird/content/instantbird.xul
>  #include menus.xul.inc
>  #endif
>  
>    <commandset id="conversationsCommands">
> -    <command id="cmd_close" oncommand="document.getElementById('conversations').removeCurrentTab()"/>
> -    <command id="cmd_putOnHold" oncommand="var tabbrowser = document.getElementById('conversations');
> -                                           tabbrowser.mCurrentTab.linkedConversation.hide();
> -                                           tabbrowser.removeTab(tabbrowser.mCurrentTab);"/>
> -    <command id="cmd_showLogs" oncommand="document.getElementById('conversations').mCurrentTab.linkedConversation.showLogs();"/>
> -    <command id="cmd_textZoomReduce" oncommand="FullZoom.reduce();"/>
> -    <command id="cmd_textZoomEnlarge" oncommand="FullZoom.enlarge();"/>
> -    <command id="cmd_textZoomReset" oncommand="FullZoom.reset();"/>
> +    <command id="cmd_close" oncommand="getTabBrowser().removeCurrentTab()"/>
> +    <command id="cmd_putOnHold"
> +             oncommand="var tabbrowser = getTabBrowser();
> +                        if (!tabbrowser.selectedConversation) return;
> +                        tabbrowser.selectedConversation.hide();
> +                        tabbrowser.removeCurrentTab();"/>
> +    <command id="cmd_showLogs"
> +             oncommand="var conv = getTabBrowser().selectedConversation;
> +                        if (conv) conv.showLogs();"/>
> +    <command id="cmd_textZoomReduce" oncommand="if (getBrowser()) FullZoom.reduce();"/>
> +    <command id="cmd_textZoomEnlarge" oncommand="if (getBrowser()) FullZoom.enlarge();"/>
> +    <command id="cmd_textZoomReset" oncommand="if (getBrowser()) FullZoom.reset();"/>
>      <command id="cmd_find"
> -             oncommand="document.getElementById('conversations').findbar.onFindCommand();"/>
> +             oncommand="var conv = getTabBrowser().selectedConversation;
> +                        if (conv) conv.findbar.onFindCommand();"/>
>      <command id="cmd_findAgain"
> -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(false);"/>
> +             oncommand="var conv = getTabBrowser().selectedConversation;
> +                        if (conv) conv.findbar.onFindAgainCommand(false);"/>
>      <command id="cmd_findPrevious"
> -             oncommand="document.getElementById('conversations').findbar.onFindAgainCommand(true);"/>
> +             oncommand="var conv = getTabBrowser().selectedConversation;
> +                        if (conv) conv.findbar.onFindAgainCommand(true);"/>
>      <commandset id="editMenuCommands"/>
>    </commandset>

I wonder if we should move the implementation of the commands to a separate object in instantbird.js and only call methods of it from here like oncommand="uiCmds.putOnHold()" / oncommand="uiCmds.textZoomReduce()" /... 

> diff --git a/instantbird/content/macgestures.js b/instantbird/content/macgestures.js
> --- a/instantbird/content/macgestures.js
> +++ b/instantbird/content/macgestures.js
> @@ -143,24 +143,29 @@ let gGestureSupport = {
>          if (this._tabs)
>            this._tabs.selectedIndex--;
>          break;
>        case "twist-right":
>          if (this._tabs)
>            this._tabs.selectedIndex++;
>          break;
>        case "swipe-down":
> +        // Don't do anything if there's no brower (non-conversation tabs)

"// This gesture is only available on conversation tabs."?

> +        if (!getBrowser())
> +          break;
>          if (aEvent.originalTarget.ownerDocument == getBrowser().contentDocument)
>            getBrowser().contentWindow.focus();
> -        getBrowser().selectedBrowser.scrollToNextSection();
> +        getBrowser().scrollToNextSection();
>          break;
>        case "swipe-up":

Same here, if you added the comment above?

> +        if (!getBrowser())
> +          break;
>          if (aEvent.originalTarget.ownerDocument == getBrowser().contentDocument)
>            getBrowser().contentWindow.focus();
> -        getBrowser().selectedBrowser.scrollToPreviousSection();
> +        getBrowser().scrollToPreviousSection();


> diff --git a/instantbird/content/tabbrowser.xml b/instantbird/content/tabbrowser.xml
> -      <method name="updatePopupMenu">
> +      <method name="tabContextMenuShowing">
>          <parameter name="aPopupMenu"/>
>          <body>
>            <![CDATA[
>              let tagName = document.popupNode.localName;
>              if (tagName == "tabs")
>                return false;
>              this.mContextTab = tagName == "tab" ?
>                                 document.popupNode : this.selectedTab;
>              var disabled = this.mTabs.length == 1;
> -            var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> -            for (var i = 0; i < menuItems.length; i++)
> -              menuItems[i].disabled = disabled;
> -            document.getElementById("context_showLogs").disabled =
> -              !this.mContextTab.linkedConversation.hasLogs();
> +            var multipleTabMenuItems =
> +              aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> +            for (let item of multipleTabMenuItems)
> +              item.disabled = disabled;
> +            let tabSpecificEndSeparator
> +              = document.getElementById("context_tabSpecificEndSeparator");
> +            // Add in tab-specific menu items from the tab panel
> +            let panelMenuItems =
> +              this.mContextTab.linkedTabPanel.getPanelSpecificMenuItems();
> +            for (let item of panelMenuItems)
> +              aPopupMenu.insertBefore(item, tabSpecificEndSeparator);

I'd say shorten the variable names and don't mind breaking the 80 char limit if it helps.

> -      <method name="updateCurrentBrowser">
> +      <method name="tabContextMenuHiding">
> +        <parameter name="aPopupMenu"/>
> +        <body>
> +          <![CDATA[
> +            // Remove tab specific menu items added onpopupshowing.
> +            let range = document.createRange();
> +            range.setStartAfter(
> +              document.getElementById("context_tabSpecificStartSeparator"));
> +            range.setEndBefore(
> +              document.getElementById("context_tabSpecificEndSeparator"));
> +            range.deleteContents();
> +            return true;
> +          ]]>
> +        </body>
> +      </method>
> +
> +      <method name="updateCurrentTab">

Same here.

> -            if (conversation == this.mCurrentConversation)
> -              this.mCurrentConversation = null;
> -
> -            // Invalidate browsers cache, as the tab is removed from the
> +            // Invalidate cache, as the tab is removed from the
>              // tab container.

Nit: this won't need two lines any longer.
*** Original post on bio 426 at 2013-06-19 22:17:45 UTC ***

(In reply to comment #33)
> Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489) [details]
> Add patch info
> 
> Flo suggested to change some names (e.g. importTabPanel to importPanel) but
> what about renaming *TabPanel to *Panel (almost) everywhere?

That's basically what I suggested. The unfortunate exception is linkedTabPanel that we can't remove to linkedPanel because linkedPanel already exists in the binding from toolkit. If their are other exceptions, it's because I missed them.

> > +      <method name="handleSwitchingAwayFromTab">
> > +        <parameter name="aTabWasVisible"/>
> > +        <body>
> > +          <![CDATA[
> > +            // Remove the unread ruler if the tab has been visible without
> > +            // interruptions for sufficiently long.
> > +            this.browser.docShell.isActive = false;
> > +            // Make sure editor stops receiving input
> > +            this.editor.blur();
> > +            if (aTabWasVisible)
> > +              this.browser.removeUnreadRuler();
> > +          ]]>
> > +        </body>
> > +      </method>
> 
> I'm still not convinced that non-conversation tabs will need the "tab was
> visible" functionality. Maybe it should be a feature of conversation tabs only
> and the "was visible"-code moved from the tabbrowser-tab implemention to the
> conv-tab implementation as a consequence?

Any tab containing a browser should be able to update the docShell.isActive value (it's required to support a web feature that I see no good reason to be break).


> I wonder if we should move the implementation of the commands to a separate
> object in instantbird.js and only call methods of it from here like
> oncommand="uiCmds.putOnHold()" / oncommand="uiCmds.textZoomReduce()" /... 

I'm not sure how much I like this idea (I guess what I dislike here is really the name "uiCmds", not the idea), but if we do this, I think it would be better in a separate bug. The patch here already contains things that I think would have been better in 3 separate bugs. Let's not add a fourth :).

> > diff --git a/instantbird/content/macgestures.js b/instantbird/content/macgestures.js
> > --- a/instantbird/content/macgestures.js
> > +++ b/instantbird/content/macgestures.js
> > @@ -143,24 +143,29 @@ let gGestureSupport = {
> >          if (this._tabs)
> >            this._tabs.selectedIndex--;
> >          break;
> >        case "twist-right":
> >          if (this._tabs)
> >            this._tabs.selectedIndex++;
> >          break;
> >        case "swipe-down":
> > +        // Don't do anything if there's no brower (non-conversation tabs)
> 
> "// This gesture is only available on conversation tabs."?

Oh, good catch. In tab containing a brower, it should scroll to top/bottom.
Maybe from here we should just dispatch an event to the panel, and then deal with it in each panel implementation that cares?

> > -            // Invalidate browsers cache, as the tab is removed from the
> > +            // Invalidate cache, as the tab is removed from the
> >              // tab container.
> 
> Nit: this won't need two lines any longer.

I almost commented on this line yesterday, I think you want 'invalidate cache*s*'.
*** Original post on bio 426 at 2013-06-19 22:30:45 UTC ***

(In reply to comment #34)
> (In reply to comment #33)
> > Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489) [details]
> > > +      <method name="handleSwitchingAwayFromTab">
> > > +        <parameter name="aTabWasVisible"/>
> > > +        <body>
> > > +          <![CDATA[
> > > +            // Remove the unread ruler if the tab has been visible without
> > > +            // interruptions for sufficiently long.
> > > +            this.browser.docShell.isActive = false;
> > > +            // Make sure editor stops receiving input
> > > +            this.editor.blur();
> > > +            if (aTabWasVisible)
> > > +              this.browser.removeUnreadRuler();
> > > +          ]]>
> > > +        </body>
> > > +      </method>
> > 
> > I'm still not convinced that non-conversation tabs will need the "tab was
> > visible" functionality. Maybe it should be a feature of conversation tabs only
> > and the "was visible"-code moved from the tabbrowser-tab implemention to the
> > conv-tab implementation as a consequence?
> 
> Any tab containing a browser should be able to update the docShell.isActive
> value (it's required to support a web feature that I see no good reason to be
> break).

I was talking about the code [1] that decides if a tab was visible for long enough to acknowledge what was shown there (e.g. by removing the unread ruler when switching away from it).

[1] http://pastebin.instantbird.com/227243
*** Original post on bio 426 at 2013-06-20 09:26:05 UTC ***

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > Comment on attachment 8354256 [details] [diff] [review] (bio-attmnt 2489) [details]
> > > > +      <method name="handleSwitchingAwayFromTab">
> > > > +        <parameter name="aTabWasVisible"/>
> > > > +        <body>
> > > > +          <![CDATA[
> > > > +            // Remove the unread ruler if the tab has been visible without
> > > > +            // interruptions for sufficiently long.
> > > > +            this.browser.docShell.isActive = false;
> > > > +            // Make sure editor stops receiving input
> > > > +            this.editor.blur();
> > > > +            if (aTabWasVisible)
> > > > +              this.browser.removeUnreadRuler();
> > > > +          ]]>
> > > > +        </body>
> > > > +      </method>
> > > 
> > Any tab containing a browser should be able to update the docShell.isActive
> > value (it's required to support a web feature that I see no good reason to be
> > break).
> 
> I was talking about the code [1] that decides if a tab was visible for long
> enough to acknowledge what was shown there (e.g. by removing the unread ruler
> when switching away from it).
> 
> [1] http://pastebin.instantbird.com/227243

The problem here is that this patch moves the comment "Remove the unread ruler..." to the wrong place. It has nothing to do with this.browser.docShell.isActive = false. See the existing code,
http://lxr.instantbird.org/instantbird/source/instantbird/content/tabbrowser.xml#2100

> > > > +            // Make sure editor stops receiving input
> > > > +            this.editor.blur();
Where is this from, in the existing code?
Attached image newConversation.png, Icon (obsolete) —
*** Original post on bio 426 as attmnt 2494 at 2013-06-20 20:13:00 UTC ***

Here's the icon I created. I even fixed some details that nobody will ever notice ;)
Comment on attachment 8354261 [details]
newConversation.png, Icon

*** Original change on bio 426 attmnt 2494 at 2013-06-20 20:37:42 UTC ***

Sorry, this was the wrong bug for the icon. Appending it at bug 955013 (bio 1583) instead.
Attachment #8354261 - Attachment is obsolete: true
Attached patch Address comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2497 at 2013-06-21 11:11:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354264 - Flags: review?(florian)
Attachment #8354264 - Flags: feedback?(aleth)
Attachment #8354264 - Flags: review?(benediktp)
Comment on attachment 8354256 [details] [diff] [review]
Add patch info

*** Original change on bio 426 attmnt 2489 at 2013-06-21 11:11:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354256 - Attachment is obsolete: true
Comment on attachment 8354264 [details] [diff] [review]
Address comments

*** Original change on bio 426 attmnt 2497 at 2013-06-21 11:45:01 UTC ***

>-      <method name="updateCurrentBrowser">
>+      <method name="updateCurrentTab">
...
>               // the user stays on the tab for more than a moment, to prevent
>               // tabs from being marked as read if the user is just scrolling
>               // past them with the arrow keys. 400ms is between "200ms - a bit
>               // too quick for people who repeat-keypress more slowly than me"
>               // and "600ms - a bit too noticeable already".
>               if (this._tabSelectTimer)
>                 clearTimeout(this._tabSelectTimer);
>               this._tabSelectTimer = setTimeout(function() {
>-                this.mCurrentConversation.onSelect();
>+                if ("onSelect" in this.selectedPanel)
>+                  this.selectedPanel.onSelect();
>               }.bind(this), 400);
>               return;

What's the point of even having this timeout if the onSelect method does not exist? Don't we know if the method exists at the time we create the timer?

Check the other instances where you check if a method exists for possible similar "savings".

>             delete this._tabSelectTimer;
>-            this.mCurrentConversation.focus();
>+            this.selectedPanel.focus();

Don't we want an additional this.selectedPanel.onSelect() here now?

>+   this.addPanel(newPanel, aOtherTab.linkedTabPanel.conv);

Don't you need aOtherTab.linkedTabPanel.conv || null ?

In the addPanel comment, move the switchingTo* method comments before the onSelect comment to make it clearer that switchingToTab is called in more cases than onSelect.
Attachment #8354264 - Flags: feedback?(aleth) → feedback+
Attached patch Address aleth's comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2498 at 2013-06-21 12:04:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354266 - Flags: review?(florian)
Attachment #8354266 - Flags: feedback?(aleth)
Attachment #8354266 - Flags: review?(benediktp)
Comment on attachment 8354264 [details] [diff] [review]
Address comments

*** Original change on bio 426 attmnt 2497 at 2013-06-21 12:04:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354264 - Attachment is obsolete: true
Attachment #8354264 - Flags: review?(florian)
Attachment #8354264 - Flags: review?(benediktp)
Comment on attachment 8354266 [details] [diff] [review]
Address aleth's comments

*** Original change on bio 426 attmnt 2498 at 2013-06-21 12:07:49 UTC ***

Thanks!
Attachment #8354266 - Flags: feedback?(aleth) → feedback+
Comment on attachment 8354266 [details] [diff] [review]
Address aleth's comments

*** Original change on bio 426 attmnt 2498 at 2013-06-22 00:38:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354266 - Flags: review?(benediktp) → review-
*** Original post on bio 426 at 2013-06-22 00:38:57 UTC ***

Comment on attachment 8354266 [details] [diff] [review] (bio-attmnt 2498)
Address aleth's comments

Great, I would say that you're almost there! :) Here's the comments that I had on your latest iteration. Only nits and trivial changes left to fix from my point of view. Let's see what flo has to say on this :)

> diff --git a/instantbird/content/buddytooltip.xml b/instantbird/content/buddytooltip.xml

> +     <method name="updateTooltipFromTab">
> +       <parameter name="aTab"/>
> +       <body>
> +       <![CDATA[
> +         this.elt = aTab;
> +         this.reset();
> +         this.setAttribute("displayname", aTab.getAttribute("label"));
> +         this.setAttribute("iconPrpl", "");
> +         // Make sure there's no awkward gap before the label where the prpl icon should be

Nit: There's a missing period at the end of the sentence. "Hide awkward gap..." would be shorter by the way.

> diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml

>        <method name="finishImport">
>          <parameter name="aConversation"/>
>          <body>
>          <![CDATA[
> +          // Swap the docshells

Nit: comments should be complete sentences and should end in a period.

> +          this.browser.swapDocShells(aConversation.browser);
> +          // Ensure observers are removed

Nit: same here.

> +      <method name="getPanelSpecificMenuItems">
> ...
> +            let showLogsItem = createMenuItem("contextShowLogs",
> +              function(aEvent) conv.showLogs());

Parameter aEvent is unused.

> +            let hideConvItem = createMenuItem("contextHideConv",
> +              function(aEvent) {

Same here, ...

> +            let closeConvItem = createMenuItem("contextCloseConv",
> +              function(aEvent) {

...and here. The "function() {" bits should fit on the previous lines then.

> diff --git a/instantbird/content/instantbird.js b/instantbird/content/instantbird.js

>  function getConvWindowURL() "chrome://instantbird/content/instantbird.xul"
>  
> +function getTabBrowser()
> +{
> +  return document.getElementById("conversations");
> +}

As flo suggested earlier this should be: function getTabBrowser() document.getElementById("conversations")
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8#Expression_closures_%28Merge_into_own_page.2Fsection%29 for documentation.

>  function getBrowser()
>  {
> -  return document.getElementById("conversations");
> +  return getTabBrowser().selectedBrowser;
>  }

Same here: function getBrowser() getTabBrowser().selectedBrowser

>  
> diff --git a/instantbird/content/tabbrowser.xml b/instantbird/content/tabbrowser.xml
> -      <method name="updatePopupMenu">
> +      <method name="tabContextMenuShowing">
>  ...
> +            var multipleTabMenuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> +            for (let item of multipleTabMenuItems)
> +              item.disabled = disabled;
> +            let tabSpecificEndSeparator = document.getElementById("context_tabSpecificEndSeparator");
> +            if ("getPanelSpecificMenuItems" in this.mContextTab.linkedTabPanel) {
> +              // Add in tab-specific menu items from the tab panel
> +              let panelMenuItems = this.mContextTab.linkedTabPanel.getPanelSpecificMenuItems();
> +              for (let item of panelMenuItems)
> +                aPopupMenu.insertBefore(item, tabSpecificEndSeparator);
> +              return true;
> +            }

At the moment this method has no return value if the panel doesn't have a getPanelSpecificMenuItems method. Returning undefined would prevent the context menu from appearing (since tabContextMenuShowing is set as onpopupshowing-handler, see https://developer.mozilla.org/en-US/docs/XUL/Attribute/onpopupshowing).

> -      <method name="updateCurrentBrowser">
> +      <method name="updateCurrentTab">
>          <parameter name="aForceUpdate"/>
>          <body>
>            <![CDATA[
> -            var newConversation = this.getConversationAtIndex(this.mTabContainer.selectedIndex);
> -            if (!aForceUpdate && this.mCurrentConversation == newConversation)
> +            /* This method handles transitioning when switching tabs.
> +             * When a new tab is selected, mCurrentTab still refers to the
> +             * previously selected tab until we set it in this method.
> +             * this.selectedTab always refers to the actual selected tab
> +             * (see the "selectedTab" property).
> +             * Also, the selected* properites other than selectedTab use

Nit: "properties"

> +      <!-- Adds a generic tab panel. Conversation specific properties and
> +           attributes are set if aConv is defined.
> +           Panels can implement the following methods to customize behavior in certain situations:
> +           destroy: Use to perform custom actions before this tab is removed.
> +                    (See comment in _endRemoveTab)
> +           finishImport: Called after reattaching to a new window. Use to
> +                         initialize from the instance in the previous window.
> +           focus: It is assumed that a panel focuses a child element in its focus
> +                  method. If it doesn't, a previous tab's focused element may be
> +                  left focused, and hence may continue to receive input.

"focus" shouldn't be among these items that "[p]anels can implement". It's mandatory to implement it to prevent focus issues from what I understand.

> +           getPanelSpecificMenuItems: Return an array of menu items specific to this panel.
> +           onResize: Use to perform custom actions on resizing.
> +           onSelect: Called when the tab is selected and the user is not just
> +                     scrolling past it. See comments in updateCurrentTab and
> +                     mousedown handler in tabbrowser-tab binding for details.
> +           switchingToPanel: Called when switching to this tab, even just in passing.
> +           switchingAwayFromPanel: Called when switching away from this tab. -->

I still don't like the wording and formatting of this comment. Maybe indenting continued lines consistently would help with the latter?

> +      <method name="addPanel">
> +        <parameter name="aPanel"/>
> +        <!-- imIConversation instance -->
>          <parameter name="aConv"/>
>          <body>
>            <![CDATA[
>              // invalidate cache, because mTabContainer is about to change
> -            this._browsers = null;
>              this._conversations = null;
> +            this._tabPanels = null;
>  
> -            var conv;
> +            this.mPanelContainer.appendChild(aPanel);
> +
> +            var t = this.mTabContainer.addTab();
> +            aPanel.tab = t;
> +
> +            if (aConv) {

I think there should be a check that aConv is actually an instance of imIConversation and an error thrown if it isn't.

> @@ -672,23 +696,25 @@
>              if (this.selectedTab) // can be null if we removed the last tab
>                this.selectedTab._selected = true;
>  
>              // This will unload the document. An unload handler could remove
>              // dependant tabs, so it's important that the tabbrowser is now in
>              // a consistent state (tab removed, tab positions updated, etc.).
>              // Also, it's important that another tab has been selected before
>              // the panel is removed; otherwise, a random sibling panel can flash.
> -            this.mPanelContainer.removeChild(conversation);
> +            // The parentNode check is required because this fails in case of the dummy tab.
> +            // if (panel.parentNode == this.mPanelContainer)
> +              this.mPanelContainer.removeChild(panel);

Why is this "if" commented out?

> @@ -712,51 +738,49 @@
>                } while (tab && this._removingTabs.indexOf(tab) != -1);
>              }
>  
>              this.selectedTab = tab;
>            ]]>
>          </body>
>        </method>
>  
> -      <method name="importConversation">
> +      <method name="importPanel">
>          <parameter name="aOtherTab"/>
>          <body>
>            <![CDATA[
>              // That's gBrowser for the other window, not the tab's browser!

Does this comment still make sense? LXR can't find any other reference to gBrowser than this one.

>              var remoteBrowser =
> -              aOtherTab.ownerDocument.defaultView.getBrowser();
> +              aOtherTab.ownerDocument.defaultView.getTabBrowser();

Should the variable be renamed to "remoteTabBrowser"?

> -            // Swap the docshells
> -            ourBrowser.swapDocShells(aOtherTab.linkedBrowser);
> -            aOtherTab.linkedConversation.conv = null;
> -            conv.finishImport(aOtherTab.linkedConversation);
> +            // Tell the new panel to sync up with the other one

Nit: Comment is missing a period.

> +            if ("finishImport" in newPanel)
> +              newPanel.finishImport(aOtherTab.linkedTabPanel);

>        <property name="conversations" readonly="true">
>         <getter>
>            <![CDATA[
> -            return this._conversations ||
> -                   (this._conversations = Array.map(this.mTabs, function (tab) tab.linkedConversation));
> +            if (!this._conversations) {
> +              this._conversations =
> +                Array.map(this.mTabs, function(aTab) aTab.linkedConversation)
> +                          .filter(function(aConv) aConv);

What about formatting this like this:
if (!this._conversations) {
  this._conversations = Array.map(this.mTabs, function(aTab) aTab.linkedConversation)
                             .filter(function(aConv) aConv);
}

> @@ -1120,29 +1134,30 @@
>        <method name="moveTabTo">
>          <parameter name="aTab"/>
>          <parameter name="aIndex"/>
>          <body>
>          <![CDATA[
> -          this._browsers = null; // invalidate cache
> +          // invalidate cache

"// Invalidate caches."

>            // use .item() instead of [] because dragging to the end of the strip goes out of
>            // bounds: .item() returns null (so it acts like appendChild), but [] throws
>            this.mTabContainer.insertBefore(aTab, this.mTabContainer.childNodes.item(aIndex));
>            // invalidate cache, because mTabContainer is about to change

Inconsistent but I wouldn't want to change this comment just for the sake of changing it.

>        <field name="_windowActivateHandler" readonly="true">
>        <![CDATA[({
>          tabbrowser: this,
> -        handleEvent: function handleEvent(aEvent)
> -          this.tabbrowser.mCurrentTab.switchingToTab()
> +        handleEvent: function handleEvent(aEvent) {

Event handlers don't need to specify a parameter for the event if they aren't going to use it. You can just leave it away.

>        <field name="_windowDeactivateHandler" readonly="true">
>        <![CDATA[({
>          tabbrowser: this,
> -        handleEvent: function handleEvent(aEvent)
> -          this.tabbrowser.mCurrentTab.switchingAwayFromTab()
> +        handleEvent: function handleEvent(aEvent) {

Same here.

> diff --git a/instantbird/locales/en-US/chrome/instantbird/tabbrowser.dtd diff --git a/instantbird/modules/imWindows.jsm b/instantbird/modules/imWindows.jsm
> --- a/instantbird/modules/imWindows.jsm
> +++ b/instantbird/modules/imWindows.jsm
> @@ -72,16 +72,17 @@ var Conversations = {
>      // The conversation may still not be displayed if we are waiting
>      // for a new window. In this case the conversation will be focused
>      // automatically anyway.
>      if (this.isUIConversationDisplayed(uiConv)) {
>        let conv = this._uiConv[uiConv.id];
>        let doc = conv.ownerDocument;
>        doc.getElementById("conversations").selectedTab = conv.tab;
>        conv.focus();
> +      conv.onSelect();

Please put a comment here what "onSelect" is doing.
*** Original post on bio 426 at 2013-06-22 00:55:03 UTC ***

I didn't have time to test this yet by the way!
*** Original post on bio 426 at 2013-06-22 08:58:50 UTC ***

(In reply to comment #43)

> > +      <method name="addPanel">
> > +        <parameter name="aPanel"/>
> > +        <!-- imIConversation instance -->
> >          <parameter name="aConv"/>
> >          <body>
> >            <![CDATA[
> >              // invalidate cache, because mTabContainer is about to change
> > -            this._browsers = null;
> >              this._conversations = null;
> > +            this._tabPanels = null;
> >  
> > -            var conv;
> > +            this.mPanelContainer.appendChild(aPanel);
> > +
> > +            var t = this.mTabContainer.addTab();
> > +            aPanel.tab = t;
> > +
> > +            if (aConv) {
> 
> I think there should be a check that aConv is actually an instance of
> imIConversation and an error thrown if it isn't.

I'm not sure this is worth doing. It could be annoying to add-on authors trying to create a new kind of conversations (although I'm not aware of any such add-on, so if you do it, it's OK with me).
If you do it, instanceof isn't reliable for XPCOM objects implemented in JS so you would right it as:
aConv.QueryInterface(Ci.imIConversation);

QueryInterface will throw if the interface isn't supported by aConv.
Attached patch Address Mic's comments (obsolete) — Splinter Review
*** Original post on bio 426 as attmnt 2503 at 2013-06-22 09:43:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354271 - Flags: review?(florian)
Attachment #8354271 - Flags: feedback?(aleth)
Attachment #8354271 - Flags: review?(benediktp)
Comment on attachment 8354266 [details] [diff] [review]
Address aleth's comments

*** Original change on bio 426 attmnt 2498 at 2013-06-22 09:43:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354266 - Attachment is obsolete: true
Attachment #8354266 - Flags: review?(florian)
*** Original post on bio 426 as attmnt 2504 at 2013-06-22 10:37:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354272 - Flags: review?(florian)
Attachment #8354272 - Flags: feedback?(aleth)
Attachment #8354272 - Flags: review?(benediktp)
Comment on attachment 8354271 [details] [diff] [review]
Address Mic's comments

*** Original change on bio 426 attmnt 2503 at 2013-06-22 10:37:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354271 - Attachment is obsolete: true
Attachment #8354271 - Flags: review?(florian)
Attachment #8354271 - Flags: review?(benediktp)
Attachment #8354271 - Flags: feedback?(aleth)
Attached file Fix an oops (obsolete) —
*** Original post on bio 426 as attmnt 2507 at 2013-06-22 16:41:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354275 - Flags: review?(florian)
Attachment #8354275 - Flags: feedback?(aleth)
Attachment #8354275 - Flags: review?(benediktp)
Comment on attachment 8354272 [details] [diff] [review]
Handle "Enable tabbed conversations" pref

*** Original change on bio 426 attmnt 2504 at 2013-06-22 16:41:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354272 - Attachment is obsolete: true
Attachment #8354272 - Flags: review?(florian)
Attachment #8354272 - Flags: review?(benediktp)
Attachment #8354272 - Flags: feedback?(aleth)
*** Original post on bio 426 as attmnt 2508 at 2013-06-22 16:45:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354276 - Flags: review?(florian)
Attachment #8354276 - Flags: feedback?(aleth)
Attachment #8354276 - Flags: review?(benediktp)
Comment on attachment 8354275 [details]
Fix an oops

*** Original change on bio 426 attmnt 2507 at 2013-06-22 16:45:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354275 - Attachment is obsolete: true
Attachment #8354275 - Flags: review?(florian)
Attachment #8354275 - Flags: review?(benediktp)
Attachment #8354275 - Flags: feedback?(aleth)
Comment on attachment 8354276 [details] [diff] [review]
Previous patch was broken for some reason

*** Original change on bio 426 attmnt 2508 at 2013-06-23 21:48:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354276 - Flags: review?(florian) → review-
*** Original post on bio 426 at 2013-06-23 21:48:30 UTC ***

Comment on attachment 8354276 [details] [diff] [review] (bio-attmnt 2508)
Previous patch was broken for some reason

>+      <method name="getPanelSpecificMenuItems">

>+
>+            let hideConvItem = createMenuItem("contextHideConv", function() {
>+                conv.hide();
>+                document.getBindingParent(conv).removeTab(conv.tab);

Nit: please indent only 2 spaces.

>+            let closeConvItem = createMenuItem("contextCloseConv", function() {
>+                conv.close();
>+                document.getBindingParent(conv).removeTab(conv.tab);

Here too.




>         if (aEvent.originalTarget.ownerDocument == getBrowser().contentDocument)
>           getBrowser().contentWindow.focus();
>-        getBrowser().selectedBrowser.scrollToNextSection();
>+        getBrowser().scrollToNextSection();

This won't work if the panel isn't a conversation binding. I think http://hg.instantbird.org/instantbird/rev/8d564c726cfb#l3.1 will help you see what you need to do for the case of a regular browser.

>-        getBrowser().selectedBrowser.scrollToPreviousSection();
>+        getBrowser().scrollToPreviousSection();

Same here.



>       <method name="addConversation">
>         <parameter name="aConv"/>
>         <body>
>           <![CDATA[
>             if (!this.mFirstTabIsDummy) {
>-              if (!Services.prefs.getBoolPref("messenger.conversations.openInTabs"))
>-                return null;
>-
>               if (Services.prefs.getBoolPref("messenger.conversations.useSeparateWindowsForMUCs") &&
>                   aConv.isChat != this.conversations[0].hasAttribute("chat"))

Can this.conversations[0] now be null/undefined?


>                 return null;
>             }
> 
>-            return this._addConversation(aConv);
>-        ]]>
>+            let convPanel = document.createElementNS(
>+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                            "conversation");

Please align "conversation" with "http://"


>+           destroy:
>+             Called before panel is removed.
>+             The destructor doesn't always get called the panel is removed, so use

Looks like some words are missing. Maybe rephrase as: "The destructor may not always get called when the panel is removed"

>+           switchingToPanel:
>+             Called when switching to the panel, even just in passing.
>+             Use it to customize behavior when the panel is switched to.

Replace "is switched to" by "is displayed" please.

>+           switchingAwayFromPanel:
>+             Called when switching away from the panel.
>+             Use it to customize behavior when the panel is switched away from. -->

"when the panel is hidden".

Overall, these comments are _much_ better than before :-).


>+      <method name="addPanel">
>+        <!-- Node containing content of panel -->
>+        <parameter name="aPanel"/>
>+        <!-- imIConversation instance -->
>         <parameter name="aConv"/>

I would put these 2 comments after the <parameter> tags, on the same line.



>+        if (event.attrName == "label") {
>+          if ("getTabBrowser" in window)
>+            getTabBrowser().updateTitlebar();
>+          // Update our tooltiptext, but only if a tooltip hasn't been set.
>+          if (!this.hasAttribute("tooltip"))
>+            this.setAttribute("tooltiptext", event.newValue);
>+        } else if (event.attrName == "tooltip") {

Nit: We always put "else" on a new line so that the "else" is aligned with the "if" it refers to.

>+          if (event.attrChange == event.ADDITION) {
>+            // Tooltip was added. Stop using our tooltiptext attribute.
>+            this.removeAttribute("tooltiptext");
>+          }
>+          if (event.attrChange == event.REMOVAL) {

I think you wanted "else if" here.

>+            // Tooltip was removed. Switch to using our label as tooltiptext.
>+            this.setAttribute("tooltiptext", this.getAttribute("label"));
>+          }
>+        } else if (event.attrName == "selected") {

Line break missing here too.


I reviewed quickly, but it looks like we are getting there :-). I will likely just verify that my comments here have been addressed as intended before r+ing the next iteration.
*** Original post on bio 426 as attmnt 2510 at 2013-06-24 03:43:00 UTC ***

Please note that I can't test any Mac gestures at the moment since I don't have an Apple trackpad.
Attachment #8354278 - Flags: review?(florian)
Attachment #8354278 - Flags: review?(benediktp)
Comment on attachment 8354276 [details] [diff] [review]
Previous patch was broken for some reason

*** Original change on bio 426 attmnt 2508 at 2013-06-24 03:43:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354276 - Attachment is obsolete: true
Attachment #8354276 - Flags: review?(benediktp)
Attachment #8354276 - Flags: feedback?(aleth)
Comment on attachment 8354278 [details] [diff] [review]
Address flo's comments

*** Original change on bio 426 attmnt 2510 at 2013-06-24 10:10:31 UTC ***

I reviewed the interdiff(s) and haven't seen anything that I'd object against anymore. Testing the changes with the code from bug 955438 (bio 2002) also worked fine for me. Thanks a lot!
Attachment #8354278 - Flags: review?(benediktp) → review+
*** Original post on bio 426 at 2013-06-24 10:45:20 UTC ***

This looks good to me (though I haven't tested it)!

I would suggest adding a focusPanel method to tabbrowser.xml, generalizing http://lxr.instantbird.org/instantbird/source/instantbird/modules/imWindows.jsm#78:
 doc.getElementById("conversations").selectedTab = conv.tab;
 conv.focus();
 doc.defaultView.focus();
#ifdef XP_MACOSX
 Components.classes["@mozilla.org/widget/macdocksupport;1"]
           .getService(Components.interfaces.nsIMacDockSupport)
           .activateApplication(true
#endif
It seems to me this would be useful?
Blocks: 955449
*** Original post on bio 426 at 2013-06-24 15:57:53 UTC ***

(In reply to comment #53)
> This looks good to me (though I haven't tested it)!
> 
> I would suggest adding a focusPanel method to tabbrowser.xml, generalizing
> http://lxr.instantbird.org/instantbird/source/instantbird/modules/imWindows.jsm#78:
Fixed by bug 955449 (bio 2012).
Comment on attachment 8354278 [details] [diff] [review]
Address flo's comments

*** Original change on bio 426 attmnt 2510 at 2013-06-25 00:06:11 UTC ***

r=me! \o/
Attachment #8354278 - Flags: review?(florian) → review+
*** Original post on bio 426 at 2013-06-25 02:06:42 UTC ***

http://hg.instantbird.org/instantbird/rev/ea36babac4be

Awesome! Thanks for fixing this.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Depends on: 955455
Depends on: 955473
You need to log in before you can comment on or make changes to this bug.