Closed Bug 1160105 Opened 5 years ago Closed 5 years ago

Fix strict mode warnings in protovis-r2.6-modded.js

Categories

(Thunderbird :: Search, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 41.0

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 1 obsolete file)

In faceted search results:

JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 59: SyntaxError: test for equality (==) mistyped as assignment (=)?
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 627: SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 1003: TypeError: variable t redeclares argument
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 1844: SyntaxError: test for equality (==) mistyped as assignment (=)?
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 3626: SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 3612: ReferenceError: reference to undefined property d.$panel
JavaScript strict warning: chrome://messenger/content/protovis-r2.6-modded.js, line 1858: ReferenceError: reference to undefined property this.events

Maybe Bug 517978 is an alternative.
Attached patch protoviswarnings.diff (obsolete) — Splinter Review
It's "modded" anyway...
Attachment #8599816 - Flags: review?(acelists)
Comment on attachment 8599816 [details] [diff] [review]
protoviswarnings.diff

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

::: mail/base/content/protovis-r2.6-modded.js
@@ +55,5 @@
>   */
>    pv.parse = function(js) { // hacky regex support
>      var re = new RegExp("function(\\s+\\w+)?\\([^)]*\\)\\s*", "mg"), m, i = 0;
>      var s = "";
> +    while ((m = re.exec(js))) {

I already remember this (()) hack works, but I do not know why :) Can it be rewritten in a more readable way? Another occurrence is also later on.

@@ +623,5 @@
>          return (new pv.Color.Hsl(h, s, l, a)).rgb();
>        }
>        case "rgba":
>        case "rgb": {
> +        let parse = c => { // either integer or percentage

For me "let name = function () { ... }" is more readable.

@@ +3624,5 @@
>          d.appendChild(c);
>        }
>  
>        /** Returns the computed style for the given element and property. */
> +      let css = (e, p) => {

let css = function (e, p) {... }
Attachment #8599816 - Flags: review?(acelists)
(In reply to :aceman from comment #2)
> Comment on attachment 8599816 [details] [diff] [review]
> > +    while ((m = re.exec(js))) {
> 
> I already remember this (()) hack works, but I do not know why :) Can it be
> rewritten in a more readable way? Another occurrence is also later on.

It works because the brackets make it explicit that the variable assignment is supposed to happen before the cast to boolean (that is needed to evaluate the condition), i.e. it is made clear that the = is not a typo for ==.

One could rewrite it as 

while (!!(m = re.exec(js)))

or move the assignment inside the loop, but neither of those two options make it more readable imho.
 
> @@ +623,5 @@
> >          return (new pv.Color.Hsl(h, s, l, a)).rgb();
> >        }
> >        case "rgba":
> >        case "rgb": {
> > +        let parse = c => { // either integer or percentage
> 
> For me "let name = function () { ... }" is more readable.

I made the change, but arrow functions are becoming quite common ;)
Attachment #8600275 - Flags: review?(acelists)
Attachment #8599816 - Attachment is obsolete: true
Comment on attachment 8600275 [details] [diff] [review]
protoviswarnings.diff v2

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

Thanks.
Attachment #8600275 - Flags: review?(acelists) → review+
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
https://hg.mozilla.org/comm-central/rev/35679800b300 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
You need to log in before you can comment on or make changes to this bug.