Consolidate startup code

VERIFIED FIXED in 0.9

Status

Calendar
Internal Components
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 313320 [details] [diff] [review]
Consolidate startup code - v2

We have a lot of code that is being called at startup in two versions one for lightning and one for sunbird. This can be consolidated into a common function.

In addition, in the startup functions there is some code that is being called to (un)initialize certain parts of the application. This code is better kept with the part of the application that is being (un)initialized.

This patch takes care and probably fixes some leaks too. Some parts of the ltnFinish function were not being called. I don't quite remember why since this patch is a couple of weeks old, but I think it had to do with some exception being thrown.
Attachment #313320 - Flags: review?(daniel.boelzle)
Comment on attachment 313320 [details] [diff] [review]
Consolidate startup code - v2

drive-by comment:

>Index: calendar/base/content/calendar-decorated-base.xml
>         <implementation implements="calIDecoratedView">
>+          <constructor><![CDATA[
>+            const kWorkdaysCommand = "calendar_toggle_workdays_only_command";
>+            this.workdaysOnly = (document.getElementById(kWorkdaysCommand)
>+                                          .getAttribute("checked") == "true");

This seems to make the view xbl depend on the rest of the chrome. I think that xbl files should be independent.
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)

> This seems to make the view xbl depend on the rest of the chrome. I think that
> xbl files should be independent.

I know what you mean, but the alternative I see is to use attributes for the command ids. Does this satisfy you? :-)

Comment 3

10 years ago
I don't understand the changes to the datetimepickers. Moreover the patch has bitrotted a bit in messenger-overlay-sidebar.js in the meantime (sorry for that). Even when fixing that, I get some js errors on startup. Could you please refreshen the patch?

Comment 4

10 years ago
I think that the minimonth.xml hunk can be removed from the patch as this file is about to be overworked
(Assignee)

Comment 5

10 years ago
Created attachment 318480 [details] [diff] [review]
Consolidate startup code - v5

de-bitrotted patch.

I kept the view commands hardcoded, since I think its silly to make all decorated view tags look similar to:

<calendar-decorated-day-view
    id="day-view" flex="1"
    context="calendar-view-context-menu"
    item-context="calendar-item-context-menu"
    toggle_workdays_command="calendar_toggle_workdays_command"
    toggle_tasks_in_view_command="calendar_toggle_tasks_in_view_command"
    toggle_show_completed_command="calendar_toggle_show_completed_command"
    toggle_orientation_command="calendar_toggle_orientation_command"/>
Attachment #313320 - Attachment is obsolete: true
Attachment #318480 - Flags: review?(daniel.boelzle)
Attachment #313320 - Flags: review?(daniel.boelzle)
(Assignee)

Updated

10 years ago
Attachment #318480 - Flags: review?(daniel.boelzle) → review?(Berend.Cornelius)

Comment 6

10 years ago
Your patch works fine and basically looks good. Some remarks that I noticed:

>I kept the view commands hardcoded, since I think its silly to make all
>decorated view tags look similar to:
>
><calendar-decorated-day-view
>    id="day-view" flex="1"
>    context="calendar-view-context-menu"
>    item-context="calendar-item-context-menu"
>    toggle_workdays_command="calendar_toggle_workdays_command"
>    toggle_tasks_in_view_command="calendar_toggle_tasks_in_view_command"
>    toggle_show_completed_command="calendar_toggle_show_completed_command"
>    toggle_orientation_command="calendar_toggle_orientation_command"/>

I am wondering why we should keep these commmands in the Decorated Views at all. I know that the idl currently demands this but as far as I see we do not access these properties from the decoated Views, but instead we refer to the commands directly by "document.getElementById(..." - see calendar-views. I find it disturbing that we have various ways to access these commands and there would favour to remove that stuff from calIDecoratedView.idl an its implementation in calendar-decorated-base.xml.

>          // TODO Make sure the selected day is valid
>          // TODO select now if it is in the range?
>          return selected;

I can't really estimate how important these notes are, so I think it's Ok to leave it as it is.

Comment 7

10 years ago
Comment on attachment 318480 [details] [diff] [review]
Consolidate startup code - v5

r=berend based on my previous comments.
Attachment #318480 - Flags: review?(Berend.Cornelius) → review+
(Assignee)

Comment 8

10 years ago
(In reply to comment #6)
> Your patch works fine and basically looks good. Some remarks that I noticed:
> 
> I am wondering why we should keep these commmands in the Decorated Views at
> all. I know that the idl currently demands this but as far as I see we do not
> access these properties from the decoated Views, but instead we refer to the
> commands directly by "document.getElementById(..." - see calendar-views. I find
> it disturbing that we have various ways to access these commands and there
> would favour to remove that stuff from calIDecoratedView.idl an its
> implementation in calendar-decorated-base.xml.
I think this can be done in a folllow up bug, and has no high priority.

Checking in the version here.
(Assignee)

Comment 9

10 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9

Comment 10

10 years ago
Checked via mxr.mozilla.org -> VERIFIEd
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.