Closed
Bug 426746
Opened 16 years ago
Closed 16 years ago
Consolidate startup code
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
0.9
People
(Reporter: Fallen, Assigned: Fallen)
Details
Attachments
(1 file, 1 obsolete file)
39.46 KB,
patch
|
berend.cornelius09
:
review+
|
Details | Diff | 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 1•16 years ago
|
||
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•16 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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #318480 -
Flags: review?(daniel.boelzle) → review?(Berend.Cornelius)
Comment 6•16 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•16 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•16 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•16 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
You need to log in
before you can comment on or make changes to this bug.
Description
•