Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends

RESOLVED FIXED in 6.2

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Lightning 6.2
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

This moves the logging/stack/assert/showError/getUUID/calTryWrappedJSObject functions to calUtils.jsm and isPropertyValueSame to calDataUtils. I've also made use of XPCOMUtils.defineLazyPreferenceGetter().

I'm leaving calGetString for a separate bug because that is used quite much and I'd also like to do some refactoring.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8952998 - Flags: review?(makemyday)
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8952998 - Attachment is obsolete: true
Attachment #8952998 - Flags: review?(makemyday)
Attachment #8953001 - Flags: review?(makemyday)
Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bf2037ffe4e067c007a0e4fe0dd6eb94c986c84&selectedJob=164056900


(note that try may look like it doesn't have all bugs, but there was a previous failed try run, so the newest one isn't showing all changesets. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ae663d89c9caf8d2f0917f5c83168d8540100ea1&selectedJob=163954434 for the previous run)

Comment 4

Last year
Comment on attachment 8953001 [details] [diff] [review]
Fix - v1

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

r+ with comments considered.

::: calendar/base/modules/calDataUtils.jsm
@@ +325,5 @@
> +     *
> +     * @param aObjects An Array of Objects to inspect
> +     * @param aProperty Name the name of the Property of which the value is compared
> +     */
> +    isPropertyValueSame: function(aObjects, aPropertyName) {

Can you shorten this a bit to samePropertyValue? Also, please extend the doc block with the return value.

::: calendar/base/modules/calUtils.jsm
@@ +78,5 @@
> +     *
> +     * @param aArg  either a string to log or an object whose entire set of
> +     *              properties should be logged.
> +     */
> +    LOG: _logger.bind(null, Components.interfaces.nsIScriptError.infoFlag, "debugLogEnabled", ""),

We probably should consider to change this name to DEBUG to make it similar to WARN and ERROR describing the log level - what do you think? Also, we maybe should prefix such messages with "Debug: ", so that people might get a reminder to turn it off again if not needed any more.

@@ +261,5 @@
>      },
>  
> +    /**
> +     * Make a UUID using the UUIDGenerator service available, we'll use that.
> +     */

Please add a return value to the doc block

@@ +423,5 @@
> +    scriptError.init(message, filename, frame.sourceLine, frame.lineNumber, frame.columnNumber,
> +                     flag, "component javascript");
> +
> +    dump(dumpPrefix + message + "\n");
> +    Services.console.logMessage(scriptError);

Does this bring back the abiliy to expand objects and arrays like it was possible befor switching to the new error console? If not, can we make it that way easily here to cover that regression (doing that in a separate separate bug would be fine, too)?
Attachment #8953001 - Flags: review?(makemyday) → review+
(In reply to [:MakeMyDay] from comment #4)
> > +    isPropertyValueSame: function(aObjects, aPropertyName) {
> 
> Can you shorten this a bit to samePropertyValue? Also, please extend the doc
> block with the return value.
I chose to remove this function without a migration, as it is the same as 

aObjects.every(obj => obj[aPropertyName] == aObjects[0][aPropertyName])

which is not much longer than before, especially when not using long argument names. This also works for empty arrays.


> We probably should consider to change this name to DEBUG to make it similar
> to WARN and ERROR describing the log level - what do you think? Also, we
> maybe should prefix such messages with "Debug: ", so that people might get a
> reminder to turn it off again if not needed any more.

Givem this is a fairly commonly used function and we are not changing the semantics, I don't think we should just be renaming it. I could imagine creating a new DEBUG function, that takes a function as its only argument, which it executes only if logging is enabled. This way you could do e.g. DEBUG(() => "this logs " + somethingPerformanceIntensive()) that isn't evaluated at all if logging is disabled. This is more of a task for a separate bug though.


> Does this bring back the abiliy to expand objects and arrays like it was
> possible befor switching to the new error console? If not, can we make it
> that way easily here to cover that regression (doing that in a separate
> separate bug would be fine, too)?

It does not. For that it would probably make sense to use a console api instance from Console.jsm
Posted patch Fix - v2 β€” β€” Splinter Review
Actually, console log wasn't that hard. Here we go, requesting review since there are some functional changes.
Attachment #8953001 - Attachment is obsolete: true
Attachment #8962322 - Flags: review?(makemyday)

Comment 7

Last year
Comment on attachment 8962322 [details] [diff] [review]
Fix - v2

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

Thanks, just one not blocking question left.

::: calendar/base/modules/calUtils.jsm
@@ +394,5 @@
>  
> +// Preferences
> +XPCOMUtils.defineLazyPreferenceGetter(cal, "debugLogEnabled", "calendar.debug.log", false, (pref, prev, value) => {
> +    gCalendarConsole.maxLogLevel = value ? "all" : "warn";
> +});

Sorry, I didn't mention this in the first review, but what's the point of introducing this getter? debugLogEnabled isn't not used elsewhere in the patch or the existing code base?

What would probably make sense to provide a hook for verbose logging that we have in the providers. But we can do this in a separate patch since we would also need to change the consumers for that.
Attachment #8962322 - Flags: review?(makemyday) → review+
(In reply to [:MakeMyDay] from comment #7)
> Sorry, I didn't mention this in the first review, but what's the point of
> introducing this getter? debugLogEnabled isn't not used elsewhere in the
> patch or the existing code base?
We could probably make this a local variable to the module, but the main reason I added it is so I can use defineLazyPreferenceGetter which handles the updates nicely in one line.


> What would probably make sense to provide a hook for verbose logging that we
> have in the providers. But we can do this in a separate patch since we would
> also need to change the consumers for that.
I have a similar getter for verbose logging somewhere later down the patch queue. What I'd really like to be able to do is cal.LOG(() => "this is put together" + when_it_is_called() +  "and not before") which would avoid the call to when_it_is_called() if logging is disabled. This would make it impossible to log functions without them being called though. I guess we could add a separate cal.lazyLOG() or something. Anyway, discussion to be had in a different bug.

Comment 9

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/92d3e1ffec49
Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

Last year
Target Milestone: --- → 6.3
Attachment #8962322 - Flags: approval-calendar-beta+
https://hg.mozilla.org/releases/comm-beta/c75db1860608
Bug 1440249 - Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends. r=MakeMyDay
Target Milestone: 6.3 → 6.2
https://hg.mozilla.org/releases/comm-beta/rev/c75db1860608
Bug 1440249 - Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends. r=MakeMyDay
You need to log in before you can comment on or make changes to this bug.