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

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[[:MakeMyDay]]

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)?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

Actually, console log wasn't that hard. Here we go, requesting review since there are some functional changes.

Comment 7

[[:MakeMyDay]]

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.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92d3e1ffec49
Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends. r=MakeMyDay

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/releases/comm-beta/c75db1860608
Bug 1440249 - Move (almost) all remaining calUtils.js functions to calUtils.jsm and friends. r=MakeMyDay

Comment 11

[Jorg K (CEST = GMT+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