Closed Bug 1546332 Opened 6 years ago Closed 5 years ago

[de-xbl] convert the tabmail-tab binding to custom element (following m-c tabbox.xml#tab conversion in bug 1519514)

Categories

(Thunderbird :: Toolbars and Tabs, task)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(3 files, 4 obsolete files)

Bug-1546332_de-xbl_tabbrowser-tab.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
Bug-1546332_fix_mochitest.patch
(deleted), patch
darktrojan
: review-
Details | Diff | Splinter Review
Bug-1546332_fix_mochitest.patch
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Attached patch Bug-1546332_de-xbl_tabmail-tab.patch (obsolete) (deleted) — — Splinter Review

Drag and Drop behavior is not working. That thing is implemented in the tab custom element in the mozilla central and they are still working on it.

Attachment #9061175 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached patch Bug-1546332_de-xbl_tabmail-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9061175 - Attachment is obsolete: true
Attachment #9061175 - Flags: review?(mkmelin+mozilla)
Attachment #9061176 - Flags: review?(mkmelin+mozilla)

Bug 1519514 now looks ready (except for sorting out test failure). Anything we need to update here to reflect the latest changes to that patch?

Comment on attachment 9061176 [details] [diff] [review] Bug-1546332_de-xbl_tabmail-tab.patch Review of attachment 9061176 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/tabmail.xml @@ +323,4 @@ > <parameter name="event"/> > <body><![CDATA[ > event.stopPropagation(); > + var tab = document.tab; could make these "let" while you're here ::: mail/base/content/tabmailTab.js @@ +71,5 @@ > + this.addEventListener("mouseout", (event) => { > + if (event.originalTarget.classList.contains("tab-close-button")) { > + this.mOverCloseButton = false; > + } > + }); you now have multiple "mouseover" and "mouseout" listeners. please make it only one per event type
Attachment #9061176 - Flags: review?(mkmelin+mozilla)
Attached patch Bug-1546332_de-xbl_tabmail-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9061176 - Attachment is obsolete: true
Attachment #9063083 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063083 [details] [diff] [review] Bug-1546332_de-xbl_tabmail-tab.patch Review of attachment 9063083 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin ::: mail/base/content/messenger.xul @@ +151,4 @@ > <script src="chrome://messenger/content/chat/chat-contact.js"/> > <script src="chrome://messenger/content/chat/chat-group.js"/> > <script src="chrome://messenger/content/chat/chat-imconv.js"/> > +<script type="application/javascript" src="chrome://messenger/content/tabmailTab.js"/> no type please, we removed those
Attachment #9063083 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Bug-1546332_de-xbl_tabmail-tab.patch (obsolete) (deleted) — — Splinter Review
Attachment #9063083 - Attachment is obsolete: true
Attachment #9064251 - Flags: review+
Comment on attachment 9064251 [details] [diff] [review] Bug-1546332_de-xbl_tabmail-tab.patch Review of attachment 9064251 [details] [diff] [review]: ----------------------------------------------------------------- Bug 1519514 is now on autoland. This patch needs to be updated to use createXULElement instead of createElement (or we'd crash). Looking at it again, can you also rename the new file to tabmail-tab.js to match tabbrowser-tab.js

(In reply to Magnus Melin [:mkmelin] from comment #8)

Bug 1519514 is now on autoland.
This patch needs to be updated to use createXULElement instead of
createElement (or we'd crash).

Looking at it again, can you also rename the new file to tabmail-tab.js to
match tabbrowser-tab.js

Yes, will update the patch in a while.

Attachment #9064251 - Attachment is obsolete: true
Attachment #9068431 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/69ac929f4cbb
[de-xbl] convert tabmail-tab binding to custom element. r=mkmelin CLOSED TREE

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

I guess this needed landing since bug 1519514 landed. The try looked terrible, due to the URL bustage (bug 1550945). Let's see how it pans out now.

Target Milestone: --- → Thunderbird 69.0

Mozmill is green, but you busted Mochitests. That's not the fault of the URL stuff, since "M(bct)" was green on try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a91fe5ab7ea4cb2253f5220ae92a1e95f3339fd5

TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_menus.js

Status: RESOLVED → REOPENED
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(khushil324)
Flags: needinfo?(geoff)
Resolution: FIXED → ---

Checking it. Will submit the patch after testing.

Flags: needinfo?(khushil324)
Attached patch Bug-1546332_fix_mochitest.patch (deleted) — — Splinter Review
Attachment #9068542 - Flags: review?(jorgk)
Comment on attachment 9068542 [details] [diff] [review] Bug-1546332_fix_mochitest.patch Thanks, but I'm certainly the (completely) wrong reviewer for this. FYI, I dig around the C++ back-end an try to keep away from anything related to "GUI toolkit" as much as possible.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)
Attachment #9068542 - Flags: review?(jorgk) → review?(geoff)
Comment on attachment 9068542 [details] [diff] [review] Bug-1546332_fix_mochitest.patch Review of attachment 9068542 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/extensions/parent/ext-menus.js @@ +894,5 @@ > handleEvent(event) { > const menu = event.target; > if (menu.id === "tabContextMenu") { > + let tab = menu.triggerNode; > + while(tab.localName != "tab") { nit: add space after while

Actually, that's not a nit, that a lint bustage ;-) - ext-menus.js:898:7 | Expected space(s) after "while". (keyword-spacing)

Comment on attachment 9068542 [details] [diff] [review] Bug-1546332_fix_mochitest.patch Review of attachment 9068542 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/extensions/parent/ext-menus.js @@ +894,5 @@ > handleEvent(event) { > const menu = event.target; > if (menu.id === "tabContextMenu") { > + let tab = menu.triggerNode; > + while(tab.localName != "tab") { This should be `while (tab && tab.localName != "tab") {` to be on the safe side, no? Otherwise you could loop forever.
Comment on attachment 9068542 [details] [diff] [review] Bug-1546332_fix_mochitest.patch You haven't handled the case where tab is null, which was handled before. Just do what m-c did for the same problem (especially considering that's where the original code came from): https://hg.mozilla.org/mozilla-central/diff/89b0dcdb08b1f44382af48716a73f88668258fd8/browser/components/extensions/parent/ext-menus.js
Attachment #9068542 - Flags: review?(geoff) → review-
Attached patch Bug-1546332_fix_mochitest.patch (deleted) — — Splinter Review

Thanks, Geoff, I grabbed that hunk.

Attachment #9068639 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d4ef59e9ed0
fix mochitest of browser_ext_menus.js, hint by :darktrojan. r=jorgk DONTBUILD

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Regressions: 1556375
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: