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)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
Since <browser> is now a custom element, the im conversation browser binding needs changing
https://searchfox.org/comm-central/source/chat/content/convbrowser.xml#11
Assignee | ||
Comment 1•6 years ago
|
||
JavaScript error: chrome://messenger/content/chat/imconversation.xml, line 1638: TypeError: this.browser.init is not a function
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
Some cleanup and make eslint happy
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
I think we should land this r=bustage-fix for the next nightly (tomorrow), unless reviewed before that.
Comment 6•6 years ago
|
||
Agree. I'll arrange that.
Comment 7•6 years ago
|
||
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).
Comment 8•6 years ago
|
||
Comment 9•6 years ago
|
||
(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
Assignee | ||
Comment 10•6 years ago
|
||
(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?
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Comment 12•6 years ago
|
||
https://dxr.mozilla.org/comm-central/search?q=XXX+path%3Acomm%2F&redirect=false
1063 hits
https://dxr.mozilla.org/comm-central/search?q=FIXME+path%3Acomm%2F&redirect=false
88 hits.
I'd make it XXX FIXME.
Comment 13•6 years ago
|
||
(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. */
}
};
?
Comment 14•6 years ago
|
||
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).
Comment 15•6 years ago
|
||
(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).
Assignee | ||
Comment 16•6 years ago
|
||
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.
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
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.
Assignee | ||
Comment 19•6 years ago
|
||
(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]
Assignee | ||
Comment 20•6 years ago
|
||
Everything working, except for the chat preview in prefs. Didn't have time to look at that yet.
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 23•6 years ago
|
||
Comment 24•6 years ago
|
||
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.
Comment 25•6 years ago
|
||
(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 :-).
Assignee | ||
Comment 26•6 years ago
|
||
Filed bug 1521480 and bug 1521481 about the other issues noted.
Comment 27•6 years ago
|
||
Hmm, we closed this very quickly, maybe there's a review issue that needs follow-up in this bug.
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years ago
|
||
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.
Comment 30•6 years ago
|
||
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!
Assignee | ||
Comment 31•6 years ago
|
||
Yep, comment added there. Thanks for the review and other pointers!
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Description
•