Closed Bug 1521733 Opened 6 years ago Closed 6 years ago

[de-xbl] Migrate freebusy-timebar to custom element.

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 12 obsolete files)

freebusy-timebar_ce.patch
(deleted), patch
arshad
: review+
Details | Diff | Splinter Review
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED

I have been intermittently seeing this error

0:28.24 ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
 0:29.10 Undefined symbols for architecture x86_64:
 0:29.10   "nsURLFetcher::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:29.10       vtable for nsURLFetcher in nsURLFetcher.o
 0:29.28   "non-virtual thunk to nsMsgComposeSendListener::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:29.28       vtable for nsMsgComposeSendListener in nsMsgCompose.o
 0:29.47   "non-virtual thunk to nsMsgStatusFeedback::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:29.47       vtable for nsMsgStatusFeedback in nsMsgStatusFeedback.o
 0:29.67   "nsMsgComposeSendListener::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:29.67       vtable for nsMsgComposeSendListener in nsMsgCompose.o
 0:29.87   "non-virtual thunk to nsMsgPrintEngine::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:29.87       vtable for nsMsgPrintEngine in nsMsgPrintEngine.o
 0:30.06   "nsMsgProgress::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:30.06       vtable for nsMsgProgress in nsMsgProgress.o
 0:30.25   "nsMsgStatusFeedback::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:30.25       vtable for nsMsgStatusFeedback in nsMsgStatusFeedback.o
 0:30.45   "nsMsgPrintEngine::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:30.45       vtable for nsMsgPrintEngine in nsMsgPrintEngine.o
 0:30.64   "non-virtual thunk to nsURLFetcher::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:30.64       vtable for nsURLFetcher in nsURLFetcher.o
 0:30.83   "nsMsgContentPolicy::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:30.83       vtable for nsMsgContentPolicy in nsMsgContentPolicy.o
 0:31.02   "non-virtual thunk to nsMsgContentPolicy::OnContentBlockingEvent(nsIWebProgress*, nsIRequest*, unsigned int)", referenced from:
 0:31.02       vtable for nsMsgContentPolicy in nsMsgContentPolicy.o
 0:31.41 ld: symbol(s) not found for architecture x86_64
 0:31.47 clang: error: linker command failed with exit code 1 (use -v to seeinvocation)
 0:31.47 make[4]: *** [XUL] Error 1
 0:31.47 make[3]: *** [toolkit/library/target] Error 2
 0:31.47 make[2]: *** [compile] Error 2
 0:31.47 make[1]: *** [default] Error 2
 0:31.48 make: *** [build] Error 2
 0:31.48 390 compiler warnings present.

with FreebusyTimebar, attendees-list and freebusy-timegrid patch.. My build was successfuly but I can't see what I changed that led me to this error.

Attached patch freebusy-timebar-patch.patch (obsolete) (deleted) — — Splinter Review
Attached patch freebusy-timebar.patch (obsolete) (deleted) — — Splinter Review

dragging behaviour not working.

Attachment #9039409 - Attachment is obsolete: true
Component: Mail Window Front End → General
Product: Thunderbird → Calendar
Blocks: 1517040
Attached patch freebusy-timebar.patch (obsolete) (deleted) — — Splinter Review
Attachment #9039413 - Attachment is obsolete: true

Not sure why the dragging behavior isn't working.. Magnus could you please take a look?

Attached patch freebusy-timebar.patch (obsolete) (deleted) — — Splinter Review
Attachment #9040746 - Attachment is obsolete: true
Attachment #9040751 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9040751 [details] [diff] [review] freebusy-timebar.patch Review of attachment 9040751 [details] [diff] [review]: ----------------------------------------------------------------- Don't know about the dragging. You'll have to debug it ::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js @@ +66,5 @@ > + this.endDate = endTime.getInTimezone(kDefaultTimezone); > + let grid = document.getElementById("freebusy-grid"); > + this.mRange = Number(grid.getAttribute("range")); > + > + window.addEventListener("load", this.onLoad.bind(this), true); you're now both calling onLoad for "load", and then separeatly in the window global onLoad function. That seems like it would cause problems. (But perhaps not the ones you're seeing)
Attachment #9040751 - Flags: review?(mkmelin+mozilla) → feedback-
Summary: [de-xbl] Scriptify freebusy-timebar. → [de-xbl] Migrate freebusy-timebar to custom element.
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
  1. The scrolling behavior is fixed.
  2. Removed window arguments code from ce.
  3. Addressed onLoad function calls.
Attachment #9040751 - Attachment is obsolete: true
Attachment #9044593 - Flags: feedback+
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044593 - Attachment is obsolete: true
Attachment #9044594 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9044594 [details] [diff] [review] freebusy-timebar_ce.patch Review of attachment 9044594 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +5,5 @@ > +var { Preferences } = ChromeUtils.import("resource://gre/modules/Preferences.jsm"); > + > +/** > + * MozCalendarEventFreebusyTimebar is the row containing time slots. It can > + * be found in calendar-event-attendees dialog. MozCalendarEventFreebusyTimebar is a widget showing the time slot labels - dates and a number of times instances of each date. It is typically used in combination with a grid showing free and busy times for attendees going to an event, as used in the Invite Attendees dialog.
Attachment #9044594 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044594 - Attachment is obsolete: true
Attachment #9044625 - Flags: review?(philipp)
Attachment #9044625 - Flags: feedback+
Comment on attachment 9044625 [details] [diff] [review] freebusy-timebar_ce.patch Review of attachment 9044625 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +1,5 @@ > +/* 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/. */ > + > +var { Preferences } = ChromeUtils.import("resource://gre/modules/Preferences.jsm"); Geoff is removing Preferences.jsm, please instead use Services.prefs.get*Pref(). It also makes sense to use a lazy getter for Services.jsm @@ +10,5 @@ > + * It is typically used in combination with a grid showing free and busy times > + * for attendees going to an event, as used in the Invite Attendees dialog. > + * @extends {MozElements.RichListBox} > + */ > +class MozCalendarEventFreebusyTimebar extends MozElements.RichListBox { An empty line between the description and @extends or any other jsdoc directives. The line wrapping looks a bit strange as well, can you re-wrap to 100 characters? Also, the class methods need documentation here as well. @@ +32,5 @@ > + this.mEndHour = 24; > + > + this.mForce24Hours = false; > + > + this.mZoomFactor = 100; Drop the empty lines between please, this just makes it a lot longer.
Attachment #9044625 - Flags: review?(philipp) → review+

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #12)

It also makes sense to use a lazy getter for Services.jsm

For Services.jsm I don't think it makes sense to get it lazily. It's globally available in practice, so a lazy getter is just more overhead.

Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044625 - Attachment is obsolete: true
Attachment #9047101 - Flags: review?(philipp)
Attachment #9047101 - Flags: feedback+
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047101 - Attachment is obsolete: true
Attachment #9047101 - Flags: review?(philipp)
Attachment #9047103 - Flags: review?(philipp)
Attachment #9047103 - Flags: feedback+
Comment on attachment 9047103 [details] [diff] [review] freebusy-timebar_ce.patch Review of attachment 9047103 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments: ::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js @@ +31,5 @@ > + * Sets mZoomFactor to a new vlaue, clears freebusy-day's children, and updates zoomFactor and > + * force24Hours properties of freebusy-day element. > + * > + * @param {number} val - new mZoomFactor value. > + * @returns {number} new mZoomFactor value. Please fix the jsdoc as discussed. @@ +94,5 @@ > + /** > + * Calculate the difference between the first to day-elements, since the width of the head > + * element does not specify the width we need due to an arbitrary margin value. > + * > + * @returns {number} These @returns need a short description. @@ +229,5 @@ > + return this.mScrollOffset; > + } > + > + /** > + * Refreshes scroll-container children. What is the scroll-container? Maybe you can elaborate in the comment. @@ +236,5 @@ > + let date = this.mStartDate.clone(); > + let template = this.getElementsByTagName("freebusy-day")[0]; > + let parent = template.parentNode; > + let numChilds = parent.childNodes.length; > + for (let i = 0; i < numChilds; i++) { This makes more sense as a for..of @@ +248,5 @@ > + this.dayOffset = offset; > + } > + > + /** > + * Dispatches timebar event. What is a timebar event? This needs a bit more description. @@ +251,5 @@ > + /** > + * Dispatches timebar event. > + */ > + dispatchTimebarEvent() { > + setTimeout(() => { Why do we need a setTimeout here? Makes sense to add a comment about this.
Attachment #9047103 - Flags: review?(philipp) → review+
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047103 - Attachment is obsolete: true
Attachment #9047926 - Flags: review+
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047926 - Attachment is obsolete: true
Attachment #9047928 - Flags: review+
Keywords: checkin-needed
Attached patch freebusy-timebar_ce.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047928 - Attachment is obsolete: true
Attachment #9047954 - Flags: review+
Attached patch freebusy-timebar_ce.patch (deleted) — — Splinter Review
Attachment #9047954 - Attachment is obsolete: true
Attachment #9047955 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/12a57c3b8df8
Migrate freebusy-timebar binding to custom element. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: