Closed Bug 389522 Opened 17 years ago Closed 17 years ago

move shared code from applicationUtil.js to calUtils.js

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: michael.buettner, Assigned: sipaq)

References

Details

Attachments

(2 files, 1 obsolete file)

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
You also need to move launchBrowser() that is used in the Event dialog.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
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
Attached patch Patch v1Splinter Review
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 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+
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
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.
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 → ---
Ok, bustage fix (first page of the first list item) checked in.
I'll provide a followup patch for the other items.
Attached patch Followup patch (see comment 7) (obsolete) — Splinter Review
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 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.
(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?
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 on attachment 293406 [details] [diff] [review]
Followup patch - v2

Looks good. r=ssitter
Attachment #293406 - Flags: review?(ssitter) → review+
Depends on: 408607
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
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 ago17 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.