Closed Bug 1347149 Opened 7 years ago Closed 7 years ago

Use cal prefix for all calUtils functions

Categories

(Calendar :: General, enhancement, P1)

Product:
Calendar
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file, 6 obsolete files)

Updated and Combined patch, v2
458.31 KB, patch
mschroeder
: review+
Details | Diff | Splinter Review 
The upcoming patch will make sure that all current calUtils functions use the cal. prefix. While we may eventually decide differently based on bug 905097 comment 1, I think it would be a good idea to stay consistent and always use the cal. prefix (besides, I already had most of it written).
Attached patch Use cal prefix - v1 (obsolete) — — Splinter Review 

        Attachment #8847100 -
        Flags: review?(makemyday)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Use cal prefix - v1 (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed commit message

        Attachment #8847100 -
        Attachment is obsolete: true

        Attachment #8847100 -
        Flags: review?(makemyday)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Remove calUtils.js include - v1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8847103 -
        Flags: review?(makemyday)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Use cal prefix - v2 (obsolete)
        — 
          — Splinter Review
      

    


  
And now without accidentally adding calendar-event-dialog back in

        Attachment #8847102 -
        Attachment is obsolete: true

        Attachment #8847166 -
        Flags: review?(makemyday)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Use cal prefix - v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8847166 -
        Attachment is obsolete: true

        Attachment #8847166 -
        Flags: review?(makemyday)

        Attachment #8847178 -
        Flags: review?(makemyday)

    
    

    
  
Comment on attachment 8847178 [details] [diff] [review]
Use cal prefix - v3

Review of attachment 8847178 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the nits fixed.

As we now have several cal.calSomething names (like cal.calGetString), can we please consolidate the names when moving over from calUtils to calUtils.jsm to cal.something?

::: calendar/base/content/calendar-base-view.xml
@@ +245,2 @@
>          let alarmService = Components.classes["@mozilla.org/calendar/alarm-service;1"]
>                             .getService(Components.interfaces.calIAlarmService);

Can you enlarge the indentation here to align the dots?

@@ +552,5 @@
>        <method name="deleteItemsFromCalendar">
>          <parameter name="aCalendar"/>
>          <body><![CDATA[
>            /* This method must be implemented in subclasses. */
> +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;

Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)

::: calendar/base/content/calendar-statusbar.js
@@ +103,1 @@
>                                                   [this.mCurIndex, this.mCalendarCount]);

indentation.

        Attachment #8847178 -
        Flags: review?(makemyday) → review+

    
  

        Attachment #8847103 -
        Flags: review?(makemyday) → review+

    
    

    
  
(In reply to [:MakeMyDay] from comment #6)
> Comment on attachment 8847178 [details] [diff] [review]
> Use cal prefix - v3
> 
> Review of attachment 8847178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the nits fixed.
> 
> As we now have several cal.calSomething names (like cal.calGetString), can
> we please consolidate the names when moving over from calUtils to
> calUtils.jsm to cal.something?
Yes, this will happen in another patch.


> > +          throw Components.results.NS_cal.ERROR_NOT_IMPLEMENTED;
> 
> Let's leave it with NS_ERROR_NOT_IMPLEMENTED ;-)
Ha! Great catch :)

Will fix these issues before checkin!
Priority: -- → P1

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Updated and Combined patch (obsolete)
        — 
          — Splinter Review
      

    


  
After talking to Philipp, I updated and combined the patches to a check-in ready version.

Philipp, please set the checkin-needed keyword if you think it can land.

        Attachment #8847103 -
        Attachment is obsolete: true

        Attachment #8847178 -
        Attachment is obsolete: true
Flags: needinfo?(philipp)

        Attachment #8874551 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Updated and Combined patch, v2
        — 
          — Splinter Review
      

    


  
Updated the patch after some try-server runs

        Attachment #8874551 -
        Attachment is obsolete: true

        Attachment #8875458 -
        Flags: review+

    
    

    
  
After talking to Philipp over IRC, I set the checkin-needed keyword myself.
Flags: needinfo?(philipp)
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/comm-central/rev/4bafb6c2ddaf96fce9afed6ad76793224927207e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.7

    
    

    
  
Add-on Lightning Calendar Tabs is now broken:
ReferenceError: getPrefSafe is not defined       multiweek_tabs.js:39:3
ReferenceError: getDateFormatter is not defined  month_tabs.js:44:7

Looks like they need to prepend cal.getDateFormatter, but getPrefSafe seems to have disappeared.

    
    

    
  
BTW, I fixed the add-on by adding "cal." at month_tabs.js. Multiweek tabs still don't work since getPrefSafe was removed ages ago, I didn't look where, but it's mentioned in bug 1032172.

    
    

    
  
Bug 923026 marked getPrefSafe() as deprecated in Lighting 3.2 about 3 years ago. Please report errors to the extension author.

    
    

    
  
I did ;-). As I said in comment #14, I've fixed the function I need myself. Sorry about the noise.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: