Closed Bug 426746 Opened 16 years ago Closed 16 years ago

Consolidate startup code

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Consolidate startup code - v2 (obsolete) β€” β€” Splinter Review
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.
(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? :-)
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?
I think that the minimonth.xml hunk can be removed from the patch as this file is about to be overworked
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)
Attachment #318480 - Flags: review?(daniel.boelzle) → review?(Berend.Cornelius)
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 on attachment 318480 [details] [diff] [review]
Consolidate startup code - v5

r=berend based on my previous comments.
Attachment #318480 - Flags: review?(Berend.Cornelius) → review+
(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.
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
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.

Attachment

General

Created:
Updated:
Size: