Closed
Bug 389522
Opened 17 years ago
Closed 17 years ago
move shared code from applicationUtil.js to calUtils.js
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: michael.buettner, Assigned: sipaq)
References
Details
Attachments
(2 files, 1 obsolete file)
21.95 KB,
patch
|
dbo
:
review+
|
Details | Diff | Splinter Review |
4.46 KB,
patch
|
ssitter
:
review+
|
Details | Diff | Splinter Review |
This is a follow-up bug from bug #371917 and is intended to move the code that is shared between Sunbird and Lightning from applicationUtil.js to calUtils.js. Specifically, the following list of functions are affected. - openAboutDialog() - openReleaseNotes() - openRegionURL() - openFormattedRegionURL() - getFormattedRegionURL() The prototype event dialog includes applicationUtil.js which shouldn't be the case as soon as these functions have been moved. This should also be reflected by removing the appropriate line from base/jar.mn
Comment 1•17 years ago
|
||
You also need to move launchBrowser() that is used in the Event dialog.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → bugzilla
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 2•17 years ago
|
||
Please don't bloat calUtils.js further. I strongly vote for base/src/calApplicationUtils.js instead. Moreover IMO we should start to modularize the current stuff of calUtils.js into different files.
Version: Trunk → unspecified
Assignee | ||
Comment 3•17 years ago
|
||
Daniel, this is a patch attempt without bloating calUtils.js any further. Summary of changes: - replaced applicationUtil.js with calApplicationUtil.js in - jar.mn - calendar-summary-dialog.xul - sun-calendar-event-dialog.xul - Added calApplicationUtil.js to Sunbird's calendar-scripts.inc - Copied the relevant fucntions from applicationUtil.js to calApplicationUtil.js - Removed unneeded parts of the launchBrowser function
Attachment #291252 -
Flags: review?(daniel.boelzle)
Comment 4•17 years ago
|
||
Comment on attachment 291252 [details] [diff] [review] Patch v1 Looks good except for two small nits: >Index: calendar/base/src/calApplicationUtil.js please name it calApplicationUtils.js (trailing s) similar to calUtils.js. >+const nsIWindowMediator = Components.interfaces.nsIWindowMediator; please resolve that const from the file and fully qualify where used. r=dbo
Attachment #291252 -
Flags: review?(daniel.boelzle) → review+
Assignee | ||
Comment 5•17 years ago
|
||
Patch checked in on HEAD and MOZILLA_1_8_BRANCH with the changes applied that Daniel suggested.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Comment 6•17 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.11pre) Gecko/20071207 Calendar/0.8pre Seems this regressed several issues. For example the Error Console and Add-ons Manager don't open and selecting the Help menu and Check for updates don't work: Error: toJavaScriptConsole is not defined Source File: chrome://calendar/content/calendar.xul Line: 1 Error: goOpenAddons is not defined Source File: chrome://calendar/content/calendar.xul Line: 1 Error: sbUpdateItem is not defined Source File: chrome://calendar/content/calendar.xul Line: 1 Error: sbCheckForUpdates is not defined Source File: chrome://calendar/content/calendar.xul Line: 1 There are several more error messages following this scheme.
Assignee | ||
Comment 7•17 years ago
|
||
Ok, the following adjustments need to be made here: - applicationUtil.js needs to be added to calendar/sunbird/base/jar.mn That's the reason for the error messages. While we're at it, we should move applicationUtil.js to calendar/sunbird/base/content - We currently have the function 'toOpenWindowByType' in both applicationUtil.js and calApplicationUtil.js. We should remove the instance in applicationUtil.js - The function 'toBrowser' in applicationUtil.js is unused and should be removed I'll provide a new patch in a few hours. Stefan, if you want to step in before me, then please build a patch and assign the review to me.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•17 years ago
|
||
Ok, bustage fix (first page of the first list item) checked in. I'll provide a followup patch for the other items.
Assignee | ||
Comment 9•17 years ago
|
||
This covers the second and third item from comment 7. I'll open up a new bug for the cvs move of applicationUtil.js to calendar/sunbird/base/content once this patch gets approved.
Attachment #292116 -
Flags: review?(ssitter)
Comment 11•17 years ago
|
||
Comment on attachment 292116 [details] [diff] [review] Followup patch (see comment 7) (In reply to comment #7) > We currently have the function 'toOpenWindowByType' in both > applicationUtil.js and calApplicationUtil.js. We should remove > the instance in applicationUtil.js Should it be removed from calApplicationUtil.js instead? It's already defined in /mail/base/content/mailCore.js so probably only need to be shipped in Sunbird but not in Lightning. In addition it's still called in applicationUtil.js.
Assignee | ||
Comment 12•17 years ago
|
||
(In reply to comment #11) >> We currently have the function 'toOpenWindowByType' in both >> applicationUtil.js and calApplicationUtil.js. We should remove the instance >> in applicationUtil.js > > Should it be removed from calApplicationUtil.js instead? It's already defined > in /mail/base/content/mailCore.js so probably only need to be shipped in > Sunbird but not in Lightning. In addition it's still called in > applicationUtil.js. No problem. I can of course remove it in calApplicationUtil.js instead. Do you need a new patch for that?
Assignee | ||
Comment 13•17 years ago
|
||
This patch removes the 'toOpenWindowByType' function from calApplicationUtils.js instead of applicationUtil.js. It also removes the call to this function from Lightning code and replaces that call with the corresponding function from mailCore.js
Attachment #292116 -
Attachment is obsolete: true
Attachment #293406 -
Flags: review?(ssitter)
Attachment #292116 -
Flags: review?(ssitter)
Comment 14•17 years ago
|
||
Comment on attachment 293406 [details] [diff] [review] Followup patch - v2 Looks good. r=ssitter
Attachment #293406 -
Flags: review?(ssitter) → review+
Assignee | ||
Comment 15•17 years ago
|
||
Patch checked into HEAD and MOZILLA_1_8_BRANCH. I also filed bug 408607 for the cvs copy of applicationUtil.js. I will cvs remove the file from the old location and adjust the calendar/sunbird/base/jar.mn file once the cvs copy is done. Leaving bug open for now.
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 16•17 years ago
|
||
cvs copy was done and calendar/sunbird/base/jar.mn was adjusted accordingly. --> FIXED on HEAD and MOZILLA_1_8_BRANCH
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•