bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

move shared code from applicationUtil.js to calUtils.js

VERIFIED FIXED in 0.8

Status

Calendar
General
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Michael Büttner (no reviews TFN), Assigned: sipaq)

Tracking

unspecified

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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)

Updated

11 years ago
Assignee: nobody → bugzilla
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 2

11 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

11 years ago
Created attachment 291252 [details] [diff] [review]
Patch v1

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

11 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

11 years ago
Patch checked in on HEAD and MOZILLA_1_8_BRANCH with the changes applied that Daniel suggested.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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.
(Assignee)

Comment 7

11 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

11 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

11 years ago
Created attachment 292116 [details] [diff] [review]
Followup patch (see comment 7)

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)
Duplicate of this bug: 407671
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

11 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

11 years ago
Created attachment 293406 [details] [diff] [review]
Followup patch - v2

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+
(Assignee)

Updated

11 years ago
Depends on: 408607
(Assignee)

Comment 15

11 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

11 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
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.