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

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Arshad Khan [:arshad]]

https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml#894

Comment 1

[Arshad Khan [:arshad]]

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.

Comment 2

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar-patch.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar.patch

dragging behaviour not working.

Comment 4

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar.patch

Comment 5

[Arshad Khan [:arshad]]

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

Comment 6

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar.patch

Comment 7

[Magnus Melin [:mkmelin]]

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)

Comment 8

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

1. The scrolling behavior is fixed.
2. Removed window arguments code from ce.
3. Addressed onLoad function calls.

Comment 9

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 10

[Magnus Melin [:mkmelin]]

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.

Comment 11

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 13

[Magnus Melin [:mkmelin]]

(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.

Comment 14

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 15

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 17

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 18

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b98d7dc628c8dfb52f0fd144620f44d75a7f599b

Comment 19

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 20

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 21

[Arshad Khan [:arshad]]

Attachment: freebusy-timebar_ce.patch

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/12a57c3b8df8
Migrate freebusy-timebar binding to custom element. r=philipp