Closed Bug 1519091 Opened 6 years ago Closed 6 years ago

adjust im browser converation to bug 1441935 changes - imconversation.xml, line 1638: TypeError: this.browser.init is not a function

Categories

(Thunderbird :: Instant Messaging, defect)

Product:
Thunderbird
Component:
Instant Messaging
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 66.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file, 4 obsolete files)

bug1519091_imconversations.patch
(deleted), patch
florian
: review+
Details | Diff | Splinter Review

JavaScript error: chrome://messenger/content/chat/imconversation.xml, line 1638: TypeError: this.browser.init is not a function

Summary: adjust im browser converation to bug bug 1441935 changes - imconversation.xml, line 1638: TypeError: this.browser.init is not a function → adjust im browser converation to bug 1441935 changes - imconversation.xml, line 1638: TypeError: this.browser.init is not a function

Got this kind of working, making convbrowser a custom element that inherits the new browser-custom-element. It's yet not complete, and needs a lot of clean-up.

Attached patch bug1519091_imconversations.patch (obsolete) (deleted) — — Splinter Review

This seems to be working though not very tested yet. I'm not sure what if anything I broke. It's a bit hard to tell since many of the things implemented by the "old" convbrowser.xml were not in browser since a long time ago. I removed all the things that <browser> already takes care of. There might be still some there.

I'd like to get it landed quickly since chat is totally busted atm.

Attachment #9036727 - Flags: review?(florian)
Attached patch bug1519091_imconversations.patch (obsolete) (deleted) — — Splinter Review

Some cleanup and make eslint happy

Attachment #9036727 - Attachment is obsolete: true
Attachment #9036727 - Flags: review?(florian)
Attachment #9036874 - Flags: review?(florian)
Severity: normal → critical
Status: NEW → ASSIGNED

I think we should land this r=bustage-fix for the next nightly (tomorrow), unless reviewed before that.

Agree. I'll arrange that.

Maybe not quite ready for prime time yet. I tried closing a conversation on the left (I had maildev, aceman, developers) and that didn't work.

While you're there, can you make it XXX FIXME and ... Added FIXME (uppercase A).

Flags: needinfo?(mkmelin+mozilla)
Depends on: 1520863
Comment on attachment 9036874 [details] [diff] [review] bug1519091_imconversations.patch Review of attachment 9036874 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this non-trivial but urgent refactoring. I found several issues, but you are most of the way there. Where did the code of _updateAutoScrollEnabled go? That autoscroll feature is what makes the chat browser scroll to the bottom automatically when we append a new message. This behavior gets disabled when the user scrolls up to look at the history, and we re-enable it when the user scrolls to (within 10px) of the bottom. I think you confused the autoscrollEnabled thing (part of the toolkit browser custom element) that's used to scroll after a click on the mousewheel with _autoScrollEnabled which is the chat thing related to the display of new messages. The names being almost the same is obviously confusing; sorry about that :-(. Looks like mDragDropHandler was dead code and fine to remove. _exposeMethodsToContent is removed by the patch, but message styles need it. I'm not sure if we care about swapDocShells (the patch removes it). Instantbird definitely used it when tearing off a conversation into a new window, but I don't remember any piece of Thunderbird UI that does something similar. About the multiple handling of the same keypress events, I think you missed the different "modifier" attribute on the <handler> elements. ::: chat/content/conversation-browser.js @@ +17,5 @@ > + super(); > + let self = this; > + this.progressListener = { > + // @see {nsIObserver} > + onStateChange(aProgress, aRequest, aStateFlags, aStatus) { I would change this to an arrow function and entirely get rid of the 'self' variable. @@ +19,5 @@ > + this.progressListener = { > + // @see {nsIObserver} > + onStateChange(aProgress, aRequest, aStateFlags, aStatus) { > + if ((aStateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) && > + (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP)) { I would reverse this test to make an early return, and then reduce the indent level of the whole function. @@ +28,5 @@ > + > + // A common use case is to click somewhere in the conversation and > + // start typing a command (often /me). If quick find is enabled, it > + // will pick up the "/" keypress and open the findbar. See bug 1161930. > + // FIXME: disablefastfind used to handle this - removed in bug 1486984 Adding a bug number in the fixme comment would be nice. This regression annoys me several times per day, thanks for figuring out the m-c change that caused it. @@ +154,5 @@ > + > + this.addEventListener("scroll", this.browserScroll); > + this.addEventListener("resize", this.browserResize); > + > + this.conversationController = { How did you decide which object to put in the constructor and which objects to put in the collectedCallback method? It's unclear to me why the progressListener is in the constructor and the controller and selection listener are here. @@ +182,5 @@ > + this.chatSelectionListener = { > + // @see {nsISelectionListener} > + notifySelectionChanged(document, selection, reason) { > + if (!(reason & Ci.nsISelectionListener.MOUSEUP_REASON || > + reason & Ci.nsISelectionListener.SELECTALL_REASON || broken indent here. @@ +189,5 @@ > + > + Cc["@mozilla.org/widget/clipboardhelper;1"] > + .getService(Ci.nsIClipboardHelper) > + .copyStringToClipboard(serializeSelection(selection), > + Ci.nsIClipboard.kSelectionClipboard); broken indent here too (but in this case it was already broken before the patch) @@ +228,5 @@ > + get theme() { > + return this._theme || (this._theme = getCurrentTheme()); > + } > + > + get fastFind() { I don't think we need this method. The convbrowser.xml binding was a fork of the browser.xml binding. In your patch you make our new conversation browser inherit from the browser custom element, so I think we could just use the fastFind getter implementation from the browser custom element. @@ +247,5 @@ > + get contentDocument() { > + return this.webNavigation.document; > + } > + > + get markupDocumentViewer() { same here. @@ +255,5 @@ > + get contentChatNode() { > + return this.contentDocument.getElementById("Chat"); > + } > + > + set fullZoom(val) { Same for get/set fullZoom/textZoom, and isSyntheticDocument. @@ +519,5 @@ > + } > + > + if (!aNoAutoScroll) { > + newElt.getBoundingClientRect(); // avoid ireflow bugs > + if (this.autoScrollEnabled) This was a call to the autoScrollEnabled method and should remain so. @@ +691,5 @@ > + this._scrollToElement(this._lastElement); > + } > + } > + > + resetFields() { It's not clear to me what the use case for this is, and why the pref observer isn't defined and added at the same time as other objects in the constructor or in the connectedCallback method. @@ +715,5 @@ > + ]), > + }; > + } > + > + _roundToZero(num) { The _roundToZero and _accelerate methods are part of the mousewheel autoscroll feature that we can inherit from the browser custom element, these methods are dead code here. ::: chat/content/jar.mn @@ +9,5 @@ > content/chat/browserRequest.js > content/chat/browserRequest.xul > content/chat/imAccountOptionsHelper.js > * content/chat/imtooltip.xml > + content/chat/conversation-browser.js Could you please preserve the alignment here? This was probably aligned with tabs on the other lines.
Attachment #9036874 - Flags: review?(florian) → review-

(In reply to Jorg K (GMT+1) from comment #7)

Maybe not quite ready for prime time yet. I tried closing a conversation on the left (I had maildev, aceman, developers) and that didn't work.

The error when closing a conversation is:

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver] conversation-browser.js:699
resetFields chrome://chat/content/conversation-browser.js:699
destroy chrome://global/content/elements/browser-custom-element.js:1156
destroy chrome://chat/content/conversation-browser.js:322
_forgetConv chrome://messenger/content/chat/imconversation.xml:100
destroy chrome://messenger/content/chat/imconversation.xml:83
destroy chrome://messenger/content/chat/imconv.xml:189
removeContact chrome://messenger/content/chat/imgroup.xml:127
observe chrome://messenger/content/chat/chat-messenger.js:1004
removeConversation file:///.../obj-x86_64-apple-darwin17.7.0/dist/Thunderbird Daily.app/Contents/Resources/components/imConversations.js:604
close resource:///modules/jsProtoHelper.jsm:496
close file:///.../obj-x86_64-apple-darwin17.7.0/dist/Thunderbird Daily.app/Contents/Resources/components/imConversations.js:399
closeConversation chrome://messenger/content/chat/imconv.xml:222
onxblmousedown chrome://messenger/content/chat/imconv.xml:261

(In reply to Florian Quèze [:florian] from comment #8)

  • let self = this;
  • this.progressListener = {
  •  // @see {nsIObserver}

  •  onStateChange(aProgress, aRequest, aStateFlags, aStatus) {

I would change this to an arrow function and entirely get rid of the 'self'
variable.

But this is not a function, it's a helper object. How can I use an arrow function here?

(In reply to Jorg K (GMT+1) from comment #7)

While you're there, can you make it XXX FIXME and

// FIXME: is fairly standard (many editors lets you find those directly), and way more used in the conde base than XXXFIXME. Filed bug 1520908 for the work.

Flags: needinfo?(mkmelin+mozilla)

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

(In reply to Florian Quèze [:florian] from comment #8)

  • let self = this;
  • this.progressListener = {
  •  // @see {nsIObserver}

  •  onStateChange(aProgress, aRequest, aStateFlags, aStatus) {

I would change this to an arrow function and entirely get rid of the 'self'
variable.

But this is not a function, it's a helper object. How can I use an arrow function here?

Do you see a problem with:
this.progressListener = {
onStateChange: (aProgress, aRequest, aStateFlags, aStatus) => {
/* code using this. */
}
};
?

Forgot to mention this in comment 8: recording in hg that chat/content/conversation-browser.js is a move of chat/content/convbrowser.xml would be nicer (even though it really requires a diff -w to make any use of the 2 files being similar when reviewing the changes).

(In reply to Jorg K (GMT+1) from comment #12)

I'd make it XXX FIXME.

Alternatively, we could also remove this comment entirely if a fix for bug 1520908 lands soon (I've just attached a patch).

Attached patch bug1519091_imconversations.patch (obsolete) (deleted) — — Splinter Review

Thanks for the comments, and testing. I think all the comments are addressed now.

Regarding the helper object placement, I put all the helper objects in connectedCallback now, to delay setting them up as much as possible (not needed before that anyway).

super does have it's own pref observer which they reset in resetFields(), but it is a bit unclear to me why that's needed. At any case that ever get called (differently) when <browser remote> changes, and we don't change that so it would all be more of a theoretical issue.

I've renamed autoScrollEnabled to convScrollEnabled for sanity. I was indeed not noticing the capital S and mixed it up with super.autoscrollEnabled

Good catch on the event modifiers. The xbl->CE converter code is apparently not handling that. It barfed on a bunch of other things in this particular binding also so I had to hack around it a bit.

Attachment #9036874 - Attachment is obsolete: true
Attachment #9037495 - Flags: review?(florian)
Comment on attachment 9037495 [details] [diff] [review] bug1519091_imconversations.patch Review of attachment 9037495 [details] [diff] [review]: ----------------------------------------------------------------- When looking at the "Message Styles" pane of the Chat section of the preferences and switching the selected theme, I get a "TypeError: this.browser.init is not a function" error and the preview is broken. ::: chat/content/conversation-browser.js @@ +56,2 @@ > > + this.addEventListener("keypress", (event) => { I guess you tried to do something that looks like a straight conversion of the former <handler> elements, but this would be cleaner in a single keypress event listener. Either with a big if/else if/... or with a switch (event.keyCode). I think the switch solution would look better. @@ +136,5 @@ > + this._lastMessageIsContext = true; > + this._firstNonContextElt = null; > + this._messageDisplayPending = false; > + this._pendingMessages = []; > + // getNextPendingMessage and getPendingMessagesCount are the I think it makes more sense to place this comment above the getNextPendingMessage and getPendingMessagesCount methods. @@ +190,3 @@ > > + this.contentChatNode.addEventListener("load", > + this.onContentElementLoad.bind(this), true); This is broken. You need to save the result of the .bind call so that you can later call removeEventListener on it. In the code before the conversion, _onContentElementLoad was the actual method, and onContentElementLoad was the bound version, and there was a comment explaining it. @@ +199,5 @@ > + onStatusChange(progress, request, status, message) { }, > + onSecurityChange(progress, request, state) { }, > + QueryInterface: ChromeUtils.generateQI([ > + Ci.nsIWebProgressListener, > + Ci.nsISupportsWeakReference], I doubt we use the weak reference support; was it automatically added by the conversion script? If you remove it, that leaves only one interface, and I would write this on a single line: QueryInterface: ChromeUtils.generateQI([Ci.nsIWebProgressListener]), (this comment also applies to the nsIController and nsISelectionListener objects) @@ +214,5 @@ > + } > + }, > + QueryInterface: ChromeUtils.generateQI([ > + Ci.nsIObserver, > + Ci.nsISupportsWeakReference, We are not using the weak reference support, so this observer can be simplified to a simple function (that xpconnect will magically convert to an object implementing nsIObserver, thanks to the 'function' attribute at https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/xpcom/ds/nsIObserver.idl#13) So this can become: this.prefObserver = (subject, topic, data) => { ... }; Or you can go even further and remove the parameters that are never used. @@ -787,2 @@ > > - // If images higher than one line of text load they will trigger a The code from here... @@ -792,5 @@ > - return; > - } > - > - // Enable or disable auto-scroll based on the scrollbar position. > - this._updateAutoScrollEnabled(); ... to here, got lost in the conversion.
Attachment #9037495 - Flags: review?(florian) → review-

I clicked the "Submit" button for comment 17 quickly before going afk for lunch so that you could see the comments I already had sooner than later. I was done reviewing by code inspection, but wasn't done testing locally.

  • The error in the in the 'Message Styles' preference happens even without attempting to change the selected theme. Are the custom elements applied within the preferences UI?

  • While testing in actual conversations, I noticed that the convScrollEnabled code doesn't seem to work as intended. If I scroll up to read history, the next time a new message is added the conversation is scrolled to the bottom when it shouldn't. And... looking again at my comments from code inspection, missing the code next to the "Enable or disable auto-scroll based on the scrollbar position." comment totally explains this problem.

If you need to land quickly, the r- in comment 17 is mostly for the onContentElementLoad.bind issue and the auto-scroll issue. The other points are either details or matters of personal preferences.

The broken message style preview really needs fixing too, but could be done in a follow-up if we don't find an obvious solution today.

(In reply to Florian Quèze [:florian] from comment #17)

  •  QueryInterface: ChromeUtils.generateQI([

  •    Ci.nsIWebProgressListener,

  •    Ci.nsISupportsWeakReference],

I doubt we use the weak reference support; was it automatically added by the
conversion script?

Ci.nsISupportsWeakReference is needed for the progressListener. Otherwise I get NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWebProgress.addProgressListener]

Attached patch bug1519091_imconversations.patch (obsolete) (deleted) — — Splinter Review

Everything working, except for the chat preview in prefs. Didn't have time to look at that yet.

Attachment #9037816 - Flags: review?(florian)
Comment on attachment 9037816 [details] [diff] [review] bug1519091_imconversations.patch Review of attachment 9037816 [details] [diff] [review]: ----------------------------------------------------------------- This review is from code inspection only, I haven't applied locally this version of the patch. This would still be r- due to the brokeness around the bound event listener, but if you fix that and need to land quickly you can assume r=me. In that case please file a follow-up for the broken message style preview. ::: chat/content/conversation-browser.js @@ +136,3 @@ > > + this.progressListener = { > + // @see {nsIObserver} This comment is just wrong. @@ +170,3 @@ > > + let onContentLoad = this.onContentElementLoad.bind(this); > + this.contentChatNode.addEventListener("load", onContentLoad, true); This still looks broken, because the onContentLoad variable isn't saved. @@ +186,2 @@ > > + // @see {nsIObserver} This comment is correct, but useless. @@ +186,4 @@ > > + // @see {nsIObserver} > + this.prefObserver = (subject, topic, data) => { > + if (this.magicCopyEnabled) { Please fix the indent here. @@ +195,3 @@ > > + this.conversationController = { > + // @see {nsIController} Useless comment too. @@ +217,3 @@ > > + this.chatSelectionListener = { > + // @see {nsISelectionListener} Same here. @@ +227,5 @@ > + .getService(Ci.nsIClipboardHelper) > + .copyStringToClipboard(serializeSelection(selection), > + Ci.nsIClipboard.kSelectionClipboard); > + }, > + QueryInterface: ChromeUtils.generateQI([ This can go on a single line. @@ +322,5 @@ > > + if (this.contentChatNode) { > + // Remove the listener only if the conversation was initialized. > + this.contentChatNode > + .removeEventListener("load", this.onContentElementLoad, true); This needs to be the result of the .bind call.
Attachment #9037816 - Flags: review?(florian)

No luck with the preview yet. I did fix a few issues around that, but the initial load still fails, with a _conv null.

Regarding the notes about which interface it implements, at least I find them useful in case I even have to look up something.

Attachment #9037816 - Attachment is obsolete: true
Attachment #9037862 - Flags: review?(florian)
Keywords: leave-open
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/bf2126d1ba5c convert convbrowser from xbl binding into a custom element (customized built in, extending <browser>), name it conversation-browser. r=florian DONTBUILD

After discussion with Magnus, we've landed this in its current form to get some chat functionality going on Daily again. This will need some follow-ups to fix the remaining problems.

I tested it and saw these problems:
Participant list incomplete. This may be due to
ReferenceError: reference to undefined property "label" - imconversation.xml:1148:23
TypeError: nicklist.getItemAtIndex(...).label is undefined - imconversation.xml:1148:23
which was already reported in bug 1521097.

And then in the conversations list on the left, there is a pink circle with a zero in it, so for example
#maildev (0).

You can see it here https://imgur.com/a/BR2E39w.

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

Regarding the notes about which interface it implements, at least I find them useful in case I even have to look up something.

I don't mind if you want to keep them, as long as they are correct. I said I find them useless because I usually search for the QueryInterface: ChromeUtils.generateQI line which tends to be easy to find. Another way to make it even more obvious is to make that line the first one of the object. But whatever :-).

Filed bug 1521480 and bug 1521481 about the other issues noted.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED

Hmm, we closed this very quickly, maybe there's a review issue that needs follow-up in this bug.

Target Milestone: --- → Thunderbird 66.0
Comment on attachment 9037862 [details] [diff] [review] bug1519091_imconversations.patch Review of attachment 9037862 [details] [diff] [review]: ----------------------------------------------------------------- This already landed, but r=me anyway as my previous comments were reasonably addressed and this seems to work. ::: chat/content/conversation-browser.js @@ +11,2 @@ > > +(function () { // Make <browser> defined now. It's lazily defined. This comment isn't enough for me to understand why we need this hack. Was it needed for the message style preview?
Attachment #9037862 - Flags: review?(florian) → review+

Yes I needed that to make it work.
Hopefully it can be removed at some point. There's some funkyness with the toolkit and thunderbird customElements loading currently.

But my theory is that since <browser> is defined through setElementCreationCallback, and we didn't see any <browser> yet, it's simply not set in customElements yet. So as a workaround, just create a browser to get it created an put into customElements, then just lose that browser.

Can you expand the comment a bit at the same time as fixing bug 1521480? (I was assuming you were going to finish the work there). Thanks for all the good work done here!

Yep, comment added there. Thanks for the review and other pointers!

Blocks: 1441935, tb-war-on-xbl
Attachment #9037495 - Attachment is obsolete: true
Depends on: 1525730
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: