less strict debug builds
John J. Barton
javascript.options.strict on all chrome js. Since Firebug is not
'strict', it emits huge numbers of warnings, making the performance of
Firebug on debug builds horrific.
Now I know there are folks who believe strongly in 'strict'. I would
love to have a debate about it, but without derailing my suggest/request
here.
I propose that
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp
be changed to add
javascript.options.strict.debug
which defaults to true. The runtime value of this pref would control the
block of code which forces the strict setting:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1251
In non-debug builds, there is one additional pref, never used.
In debug builds, the default causes the current behavior.
In debug builds, setting strict.debug false would skip the force block.
Would such a change be acceptable?
jjb
Alex Vincent
> I propose that
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp
> be changed to add
> javascript.options.strict.debug
> which defaults to true. The runtime value of this pref would control the
> block of code which forces the strict setting:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1251
For everyone's information, this has been filed as bug 508562.
Personally, I'm not sure how I feel about this. I hope people are
filing bugs on Firebug to clean up these strict warnings...
Also, since it's a debug build, wouldn't it be practical to simply
comment out the section that's bugging you and rebuild? Developer
flexibility is good - but if you're doing a debug build you pretty much
already have that.
My two cents. After inflation.
Alex
John J. Barton
> John J. Barton wrote:
>> I propose that
>> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp
>> be changed to add
>> javascript.options.strict.debug
>> which defaults to true. The runtime value of this pref would control
>> the block of code which forces the strict setting:
>> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1251
> Also, since it's a debug build, wouldn't it be practical to simply
> comment out the section that's bugging you and rebuild? Developer
> flexibility is good - but if you're doing a debug build you pretty much
> already have that.
Thanks for the suggestion, but my goal is to encourage routine, simple
use of Firebug and Chromebug by mozilla developers. Discovering the
requirement to comment out the section, editing the code, and rebuilding
defeats this goal. (I use symbols-only builds which is why I did not
realize this was an issue).
jjb
Gijs Kruitbosch
> Personally, I'm not sure how I feel about this. I hope people are
> filing bugs on Firebug to clean up these strict warnings...
I tried, back in 2006, when it was part of my SoC project. I don't think those
changes were taken. I remember talking to others at the time that did patches in
that regard that were also not taken. From:
John J. Barton wrote:
> Now I know there are folks who believe strongly in 'strict'.
I presume that interest in fixing these things in Firebug itself is still low.
~ Gijs
PS: that being said, Mozilla web property doesn't really do a better job in
terms of strict warnings, so there is definitely room to lead by example.
John J. Barton
> Alex Vincent wrote:
>> Personally, I'm not sure how I feel about this. I hope people are
>> filing bugs on Firebug to clean up these strict warnings...
>
> I tried, back in 2006, when it was part of my SoC project. I don't think
> those changes were taken. I remember talking to others at the time that
> did patches in that regard that were also not taken. From:
I wished we had discussed this before you made the efforts.
>
> John J. Barton wrote:
> > Now I know there are folks who believe strongly in 'strict'.
>
> I presume that interest in fixing these things in Firebug itself is
> still low.
Joe Hewitt and I do not agree with the coding style enforced by
'strict'. To quote Joe: "JavaScript is not Java". (Or, I would add, C++).
I obviously believe in good tools for development. I don't think
'strict' is one of them.
Any analysis of a tool's benefit has to include costs, benefits, and
alternatives. I did that for strict on Firebug. We chose not to adopt it
thoughtfully, not out of carelessness.
It's possible that I am not aware of all the benefits of 'strict'. I
have not been able to find much documentation of its value and I have
experienced its problems and the unnecessary code contortion it requires.
One improvement would help: allowing strict to be controlled at a more
granular level. Currently if users have Firebug running and they turn on
strict, they get all of Firebug's warnings as well as there own.
jjb
Gijs Kruitbosch
> Gijs Kruitbosch wrote:
>> Alex Vincent wrote:
>>> Personally, I'm not sure how I feel about this. I hope people are
>>> filing bugs on Firebug to clean up these strict warnings...
>>
>> I tried, back in 2006, when it was part of my SoC project. I don't
>> think those changes were taken. I remember talking to others at the
>> time that did patches in that regard that were also not taken. From:
>
> I wished we had discussed this before you made the efforts.
I did not mean to come across (and am not) resentful; don't worry.
>> John J. Barton wrote:
>> > Now I know there are folks who believe strongly in 'strict'.
>>
>> I presume that interest in fixing these things in Firebug itself is
>> still low.
>
> Joe Hewitt and I do not agree with the coding style enforced by
> 'strict'. To quote Joe: "JavaScript is not Java". (Or, I would add, C++).
>
> I obviously believe in good tools for development. I don't think
> 'strict' is one of them.
>
> Any analysis of a tool's benefit has to include costs, benefits, and
> alternatives. I did that for strict on Firebug. We chose not to adopt it
> thoughtfully, not out of carelessness.
>
> It's possible that I am not aware of all the benefits of 'strict'. I
> have not been able to find much documentation of its value and I have
> experienced its problems and the unnecessary code contortion it requires.
> <snip>
OK. So, example:
if (foo = bar)
produces a strict warning.
if ((foo = bar))
does not.
I think this warning is useful, because it prevents me from writing code that
does not do what I want by accident (but lets me if I explicitly tell it that I
know what I'm doing). I don't think this is "code contortion". Similarly,
warnings for undefined members and variables help save me from typos or code
problems caused by unwittingly setting globals I didn't mean to set (assigning
to something not declared with var sets (attempts to set) the property with that
name on |window|), and help detect where I'm using a dictionary which doesn't
actually have all the keys I expect it to have, etc. And I don't think adding
'var'/'let's to your code is "code contortion" either.
In other words, I think that there are many cases where benefits > costs. In
fact, I can't think of cases where this is not the case. While I could
potentially believe that in some cases the balance tips one way or the other
solely because of circumstances, at this point I would prefer to believe that
either the warnings are useful, or they are not. In the former case, Firebug
should fix its code if these rules are violated. In the latter, the warnings
should be removed because they are not useful. If there is a clear case for
"normally not useful, except in case X" and we feel case X is valuable enough,
there should be a specific pref or way to toggle that warning on/off (or we
should make the warning detect case X if possible).
Regardless, if you think that none (or few?) of these warnings have the desired
cost/benefit, I would welcome a discussion of them and why you think they are
so. If you want, you could even file a bug (search for WONTFIXED ones before,
perhaps?) about removing them, and have the discussion there (please do let us
know here, though :-) ).
The worst for all of us would be if we end up having a group (maybe even the
majority!) of people believe some functionality in Firefox is useless, but keep
silent about it / try to live with it / work around it, because well, it's not
going to change! This project is all about participation. If you think something
is broken, tell us why.
Cheers,
Gijs
Boris Zbarsky
> Also, since it's a debug build, wouldn't it be practical to simply
> comment out the section that's bugging you and rebuild? Developer
> flexibility is good - but if you're doing a debug build you pretty much
> already have that.
How do you envision this working, exactly?
Here's how I see it working, per bug I work on, more or less:
1) I clone a new tree.
2) I create an mq.
3) I qnew and make the change above.
4) I qnew and work on my patches.
That really doesn't scale, as far as I can see. I'll typically just
forget item 3.
-Boris
Boris Zbarsky
> OK. So, example:
>
> if (foo = bar)
>
> produces a strict warning.
>
> if ((foo = bar))
>
> does not.
I think John's main issue is that foo.bar triggers a strict warning if
!("bar" in foo).
And worse yet, foo[bar.baz] triggers a strict warning if !(bar.baz in foo).
While the former is useful from a typo-detection point of view, the
latter just seems weird to me....
-Boris
Gijs Kruitbosch
Why is that weird? I must be missing a trick here, for which I apologise.
The only thing I can come up with is that in this case:
var quux = foo[bar.baz];
doStuff(quux.blah);
You do want a warning and in this case:
if (foo[bar.baz])
doStuff(foo[bar.baz].blah)
you don't? I presume this could be detected, and maybe we should? As it is, I'm
still not entirely clear if this is what you mean or not. Could you spell it out
for me, so even I get it? :-)
Cheers,
Gijs
John J. Barton
For example from Firebug:
JavaScript strict warning: chrome://firebug/content/firebug.js, line
1304: reference to undefined property persistedState.panelState[panel.name]
The code:
getPanelState: function(panel)
{
var persistedState = panel.context.persistedState;
if (!persistedState || !persistedState.panelState)
return null;
return persistedState.panelState[panel.name];
}
This case is particularly interesting because 'strict' does *not* find
the bug here.
Instead strict focuses our attention on the return statement. That
statement unambiguous and clear: it returns a property. The warning
message is a lie: the property referenced is not undefined. It may not
be defined, in which case the return value is 'undefined', exactly the
meaning we want.
So do you see the bug? It is the "return null;". This forces the caller
to test null vs undefined, something that one should not do in Firebug
style code. That caused me a lot of grief.
Had I used strict I would just waste time trying to pass the silly test.
Let's see. Would this work?
return (persistedState?
(persistedState.panelState?
("name" in panel ? persistedState.panelState[panel.name]
: undefined )
: undefined )
: undefined );
How it this better? Why do I have to think about this rather than the
real problem?
jjb
Boris Zbarsky
> var quux = foo[bar.baz];
> doStuff(quux.blah);
>
> You do want a warning
On which? I don't particularly want a warning on foo not having a
property having the name bar.baz.
> if (foo[bar.baz])
> doStuff(foo[bar.baz].blah)
Nor in this case.
> you don't? I presume this could be detected, and maybe we should? As it
> is, I'm still not entirely clear if this is what you mean or not. Could
> you spell it out for me, so even I get it? :-)
Warning on foo.bar if there is no such prop on foo can be useful because
someone might have typoed "baz" as "bar".
Warning on foo[bar.baz] is less useful, since for this to screw up the
value of bar.baz would have to be wrong, no?
-Boris
Gijs Kruitbosch
OK. Doesn't it only warn if the value of bar.baz *is* wrong (by some
definition), as in the case cited in John's post? In which case, don't you want
to fix this?
~ Gijs
Boris Zbarsky
> OK. Doesn't it only warn if the value of bar.baz *is* wrong (by some
> definition), as in the case cited in John's post? In which case, don't
> you want to fix this?
That depends on whether the contract for that object is that the
property is optional or not.
It's much more likely that the contract is that the prop is optional
than that someone typoed the value of bar.baz, imo.
-Boris
John J. Barton
> John J. Barton wrote:
>> Gijs Kruitbosch wrote:
>
> OK. So, example:
>
> if (foo = bar)
>
> produces a strict warning.
>
> if ((foo = bar))
>
> does not.
>
> I think this warning is useful, because it prevents me from writing code
If I could get this warning from strict without the others I would run
with it all the time.
> In other words, I think that there are many cases where benefits >
> costs. In fact, I can't think of cases where this is not the case. While
I can show you a few hundred, just run strict on Firebug ;-)
> I could potentially believe that in some cases the balance tips one way
> or the other solely because of circumstances, at this point I would
> prefer to believe that either the warnings are useful, or they are not.
Some are, some are not.
...
> The worst for all of us would be if we end up having a group (maybe even
> the majority!) of people believe some functionality in Firefox is
> useless, but keep silent about it / try to live with it / work around
> it, because well, it's not going to change! This project is all about
> participation. If you think something is broken, tell us why.
I don't think we will agree on all cases of what is broken and what is
not, because many of the issues here depend upon the coding style one
adopts. I'm not asking for strict to be abolished; I am only asking to
be able to optionally turn it off.
jjb
John J. Barton
> On strictness -- the IDE I use (Komodo) does green squigglies for strict
> warnings, and I'm always annoyed that it complains about:
>
> foo = {bar: 'baz',
> tom: 'cat',
> }
>
> specifically, the trailing comma. What's the rationale for complaining
> about that? I find that the trailing comma makes patches cleaner, and
> is less error prone (the cost of forgetting a comma is potentially large).
+1!
jjb
Gijs Kruitbosch
If the contract is optional, shouldn't the code check that foo actually has a
useful bar.baz? Otherwise, the code may blow up later (as in the first example I
gave).
Is this only the case for "compound" (is there a proper term for this?) ids used
for dictionary access, or do you think:
foo[bar]
should also not warn? Could we fix the warning code to check for this
"compoundness" or whatever you call it? I would presume so, and if that is the
issue, and there are no objections to it, then I think we should do that.
(I'm not entirely sure all those conditions hold - I'm just saying, if we're
warning for things which are (almost) always useless, then we should stop doing
that. :-) )
~ Gijs
Gijs Kruitbosch
> <snip>
> The code:
>
> getPanelState: function(panel)
> {
> var persistedState = panel.context.persistedState;
> if (!persistedState || !persistedState.panelState)
> return null;
>
> return persistedState.panelState[panel.name];
> }
>
> This case is particularly interesting because 'strict' does *not* find
> the bug here.
>
> Instead strict focuses our attention on the return statement. That
> statement unambiguous and clear: it returns a property. The warning
> message is a lie: the property referenced is not undefined. It may not
> be defined, in which case the return value is 'undefined', exactly the
> meaning we want.
How would anything but a human (having looked for and understood the uses of
getPanelState) know "what you want"? There are normally many more cases where
undefined is NOT an OK return value.
> So do you see the bug? It is the "return null;". This forces the caller
> to test null vs undefined, something that one should not do in Firebug
> style code. That caused me a lot of grief.
This is only a 'bug' because you want to distinguish those two (do you?).
If you don't, just do return; or return undefined; ? Nobody forces you to return
null as such...
> Had I used strict I would just waste time trying to pass the silly test.
> Let's see. Would this work?
> return (persistedState?
> (persistedState.panelState?
> ("name" in panel ? persistedState.panelState[panel.name]
> : undefined )
> : undefined )
> : undefined );
>
> How it this better? Why do I have to think about this rather than the
> real problem?
>
> jjb
That's a strawman in code. Clearly you could just add || !panel.name
or || !("name" in panel) to the if statement earlier, which is perfectly neat in
my book...
(although, as a note of caution, that first option will also succeed if
panel.name === "", which may or may not be what you want - it'd depend on the
rest of your code, which I don't know!)
~ Gijs
Gijs Kruitbosch
> If I could get this warning from strict without the others I would run
> with it all the time.
Great. I'm trying to identify the categories/patterns of warnings that you do
not think is useful.
>> In other words, I think that there are many cases where benefits >
>> costs. In fact, I can't think of cases where this is not the case. While
>
> I can show you a few hundred, just run strict on Firebug ;-)
I do not have the time to do that *and* figure out which of them are and are not
useful, why, and what it'd cost to fix them. It is your code, you know how it
works and where it gives false positives. I'm asking you to tell us where you
think it does, so that we can fix that.
>> I could potentially believe that in some cases the balance tips one
>> way or the other solely because of circumstances, at this point I
>> would prefer to believe that either the warnings are useful, or they
>> are not.
>
> Some are, some are not.
So let's identify the latter.
> I don't think we will agree on all cases of what is broken and what is
> not, because many of the issues here depend upon the coding style one
> adopts. I'm not asking for strict to be abolished; I am only asking to
> be able to optionally turn it off.
You already can on an ordinary build. What is disconcerting to me is that the
warnings are useless to you, which they are not meant to be. If we can agree on
which cases are or are not broken remains to be seen, and is what I'm trying to
figure out. Certainly, it may well be possible to determine cases which are
always useless (Boris and David make good points!), and we can hopefully fix
those so that the warnings become more useful. That'd be a positive thing, and
it would be great if you could help with that.
Gijs
Gijs Kruitbosch
> On strictness -- the IDE I use (Komodo) does green squigglies for strict
> warnings, and I'm always annoyed that it complains about:
>
> foo = {bar: 'baz',
> tom: 'cat',
> }
>
> specifically, the trailing comma. What's the rationale for complaining
> about that?
unearth their origins!) that it is about neglecting to add more properties? The
checkin comment (
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsparse.c&rev=3.168#3884
) seems to suggest it's an ECMA spec thing. Brendan and Mike (Shaver) would know
more about what went on 9 years ago than I do. :-)
> I find that the trailing comma makes patches cleaner,
OK, I could kind of buy that...
> and
> is less error prone (the cost of forgetting a comma is potentially large).
>
> --david
I don't buy this so much. IIRC forgetting one (and adding another member after
it) will produce an ordinary syntax error, and possibly stop parsing the script.
Is that what you mean by "potentially large"? If so, I need to tweak my cost
scale. :-)
~ Gijs
David Ascher
>
>> and is less error prone (the cost of forgetting a comma is
>> potentially large).
>>
>> --david
>
> I don't buy this so much. IIRC forgetting one (and adding another
> member after it) will produce an ordinary syntax error, and possibly
> stop parsing the script. Is that what you mean by "potentially large"?
> If so, I need to tweak my cost scale. :-)
I hate to admit how often it happens, but it does happen to me a lot.
And every time, that's anywhere from 10 seconds to a couple of minutes,
depending on my build setup. That adds up.
--david
Gijs Kruitbosch
Doesn't Komodo do red error squigglies for that? :-)
~ Gijs
Mike Shaver
Ascher<das...@mozillamessaging.com> wrote:
> On strictness -- the IDE I use (Komodo) does green squigglies for strict
> warnings, and I'm always annoyed that it complains about:
>
> foo = {bar: 'baz',
> tom: 'cat',
> }
>
> specifically, the trailing comma. What's the rationale for complaining
> less error prone (the cost of forgetting a comma is potentially large).
ECMA's syntax doesn't permit a trailing comma in object initializers
(perhaps changed in ES5, I can't recall) and I believe some versions
of IE choked on having one.
I agree that permitted trailing commas is superior, and useful to
developers in some environments (it simplifies code generation for
these patterns, and can make patches cleaner -- though in the latter
case you want the initial "bar: 'baz'" to be on its own line as well!)
Mike
Brendan Eich
> On Wed, Aug 5, 2009 at 2:10 PM, David Ascher<dasc...@mozillamessaging.com> wrote:
> > On strictness -- the IDE I use (Komodo) does green squigglies for strict
> > warnings, and I'm always annoyed that it complains about:
>
> > foo = {bar: 'baz',
> > tom: 'cat',
> > }
>
> > specifically, the trailing comma. What's the rationale for complaining
> > about that? I find that the trailing comma makes patches cleaner, and is
> > less error prone (the cost of forgetting a comma is potentially large).
>
> ECMA's syntax doesn't permit a trailing comma in object initializers
> (perhaps changed in ES5, I can't recall)
ES5 indeed gets rid of this obnoxious MSFT contribution to ES3. :-)
Someone please file a bug to get rid of this strict warning.
/be
David Ascher
>>> > > foo = {bar: 'baz',
>>> > > tom: 'cat',
>>> > > }
>>>
>> >
>>
>>> > > specifically, the trailing comma. What's the rationale for complaining
>>> > > about that? I find that the trailing comma makes patches cleaner, and is
>>> > > less error prone (the cost of forgetting a comma is potentially large).
>>>
>> >
>> > ECMA's syntax doesn't permit a trailing comma in object initializers
>> > (perhaps changed in ES5, I can't recall)
>>
> ES5 indeed gets rid of this obnoxious MSFT contribution to ES3.:-)
>
> Someone please file a bug to get rid of this strict warning.
John J Barton
...
> You already can on an ordinary build. What is disconcerting to me is
> that the warnings are useless to you, which they are not meant to be. If
> we can agree on which cases are or are not broken remains to be seen,
> and is what I'm trying to figure out. Certainly, it may well be possible
> to determine cases which are always useless (Boris and David make good
> points!), and we can hopefully fix those so that the warnings become
> more useful. That'd be a positive thing, and it would be great if you
> could help with that.
If your goal is a better tool for static analysis of Javascript, then I
suggest starting with an evaluation of JSLint. It is a much more
comprehensive tool, has lots of control options, is heavily documented
to justify its rules, well supported, and open source. By investing in
integration of JSLint we could end up with a better result with less
total effort.
JSLint shares the general outlook of 'strict':
JSLint defines a professional subset of JavaScript,...
(http://www.jslint.com/lint.html)
This outlook I don't agree with. In my opinion, the problems Javscript
developers face cannot be solved by changes to the language.
Maybe this is not the kind of help you had in mind, but do consider it.
jjb
Gijs Kruitbosch
I am not sure how I gave you the idea I was looking for a static analysis tool
(which, to the best of my knowledge, JSLint is not, anyway).
I will try to reformulate this one last time, because I don't seem to be making
myself clear:
Strict warnings are intended to be a feature, not a bug. If a feature is
misbehaving so that it becomes a bug, rather than throwing up our arms in
despair and trying to ignore it best we can, the feature should be fixed, or
removed.
~ Gijs
John J Barton
>
> I will try to reformulate this one last time, because I don't seem to be
> making myself clear:
>
> Strict warnings are intended to be a feature, not a bug. If a feature is
> misbehaving so that it becomes a bug, rather than throwing up our arms
> in despair and trying to ignore it best we can, the feature should be
> fixed, or removed.
I do believe I understand what you want. Rather than throwing up our
arms in despair I am proposing that the 'strict' feature be fixed.
Specifically the fix I am proposing is replacing it with JSLint. I
believe it provides the features you want from 'strict' for a much lower
cost. That means either more time can go into making the new 'strict'
excellent or more time can go in other important directions.
If you don't like the suggestion, that's fine. I'd still be up for
complaining about strict if that will help you make it better.
jjb
John J Barton
...
>
> Strict warnings are intended to be a feature, not a bug. If a feature is
> misbehaving so that it becomes a bug, rather than throwing up our arms
> in despair and trying to ignore it best we can, the feature should be
> fixed, or removed.
While finishing the work for bug 508562 I looked at the errors from
'strict' for Firebug 1.5 running on FF 3.6. In 267 errors, 24 unique
errors where reported. 23 of those were 'reference to undefined
property', the kind of message I don't think makes sense in Javascript.
One error was reported "assignment to undeclared variable" in a file in
our test harness. It was a missing 'var' that I fixed.
If the 23 bogus cases were omitted I would feel that at least 'strict'
was not doing damage. The one case is caught was a 'nice to have', but
perhaps with more experience, especially in new code, I would find more
cases to like.
jjb
Jim Blandy
> While finishing the work for bug 508562 I looked at the errors from
> 'strict' for Firebug 1.5 running on FF 3.6. In 267 errors, 24 unique
> errors where reported. 23 of those were 'reference to undefined
> property', the kind of message I don't think makes sense in Javascript.
> One error was reported "assignment to undeclared variable" in a file in
> our test harness. It was a missing 'var' that I fixed.
>
> If the 23 bogus cases were omitted I would feel that at least 'strict'
> was not doing damage. The one case is caught was a 'nice to have', but
> perhaps with more experience, especially in new code, I would find more
> cases to like.
We're going to be reconciling the conditions "strict" complains about
with those forbidden in strict mode code by ES5. Once that's done,
referring to a nonexistent property of an object will not generate a
complaint.
I still get tripped up by the standard, so I'm going to show my work:
- Assume obj is some object with no property named "prop".
- 11.2.1 says 'obj.prop' evaluates to a Reference whose base is obj,
whose referenced name is "prop", and whose strict mode flag is set from
the context.
- Since we're not assigning to the property, we must be calling GetValue
on it, described by 8.7.1. Assuming obj is actually an object,
IsPropertyReference(r) is true, so we all obj.[[Get]]("prop").
- An Object's [[Get]] method is described in 8.12.3. The
[[GetProperty]] internal method will return undefined after searching
the prototype chain, which causes us to return undefined.
So there'll be no strict-mode warning for references to non-existent
properties, which I think is what you want.
Robert Kaiser
> 23 of those were 'reference to undefined
> property', the kind of message I don't think makes sense in Javascript.
I found that exactly that message helped me in development of my code a
lot as in most cases, typos or something similar were the cause of the
warning.
Robert Kaiser
John J. Barton
Can you give an example? robcee found one in our code:
if(Firebug.Activation.allPagesActivation == "on")
should have been
if(Firebug.allPagesActivation == "on")
Here the equality test fails since the left hand side is "undefined". In
my opinion the bug in this case is *not* the undefined property. Rather
the bug was in the bigger picture:
if (Firebug.allPagesActivation == "on")
{
var label = $STR("enablement.on");
tooltip += "\n"+label+" "+$STR("enablement.for all pages");
}
if (Firebug.allPagesActivation == "off")
{
var label = $STR("enablement.off");
tooltip += "\n"+label+" "+$STR("enablement.for all pages");
}
// else allPagesActivation == "none" we don't show it.
--------^^^^^^^^^^^^^^^^^^^^^^^^^^^
I should have tested "none" and asserted on the final "else" where the
property is undefined *or* any value we don't support.
jjb
Robert Kaiser
> Robert Kaiser wrote:
>> John J Barton wrote:
>>> 23 of those were 'reference to undefined
>>> property', the kind of message I don't think makes sense in Javascript.
>>
>> I found that exactly that message helped me in development of my code
>> a lot as in most cases, typos or something similar were the cause of
>> the warning.
>>
>> Robert Kaiser
>
> Can you give an example? robcee found one in our code:
> if(Firebug.Activation.allPagesActivation == "on")
> should have been
> if(Firebug.allPagesActivation == "on")
>
> Here the equality test fails since the left hand side is "undefined". In
> my opinion the bug in this case is *not* the undefined property.
Oh, it is. You are using a property that doesn't exist though you
thought it would, and that rightly causes a warning. It's not disallowed
(in JS) - in that cases it would be an error and stop execution of the
script, but it's right to warn you that something is happening that you
probably wouldn't expect.
And it definitely points to a real bug in your code. I guess a number of
the other such warnings do as well, or at least point to code that omits
some checks for unexpected conditions, i.e. to code I would call dirty.
Though some of your statements sometimes seem to imply that you embrace
dirty hacks and dislike clean code, and if that's true, you might not
see those warnings as useful, of course. :P
Robert Kaiser
John J. Barton
> John J. Barton wrote:
>> Robert Kaiser wrote:
>>> John J Barton wrote:
>>>> 23 of those were 'reference to undefined
>>>> property', the kind of message I don't think makes sense in Javascript.
>>>
>>> I found that exactly that message helped me in development of my code
>>> a lot as in most cases, typos or something similar were the cause of
>>> the warning.
>>>
>>> Robert Kaiser
>>
>> Can you give an example? robcee found one in our code:
>> if(Firebug.Activation.allPagesActivation == "on")
>> should have been
>> if(Firebug.allPagesActivation == "on")
>>
>> Here the equality test fails since the left hand side is "undefined". In
>> my opinion the bug in this case is *not* the undefined property.
>
> Oh, it is. You are using a property that doesn't exist though you
> thought it would, and that rightly causes a warning. It's not disallowed
> (in JS) - in that cases it would be an error and stop execution of the
> script, but it's right to warn you that something is happening that you
> probably wouldn't expect.
Hopefully you read the rest of my post....
> And it definitely points to a real bug in your code. I guess a number of
> the other such warnings do as well, or at least point to code that omits
> some checks for unexpected conditions, i.e. to code I would call dirty.
...because I was pointing out that the unexpected condition test I
failed to add was a bug in my code.
> Though some of your statements sometimes seem to imply that you embrace
> dirty hacks and dislike clean code, and if that's true, you might not
> see those warnings as useful, of course. :P
I embrace Javascript and dislike efforts to pretend it is Java/C++/ObjC etc.
If we want to improve 'strict', then we have to give up on the idea that
we should put hacky workarounds in our code to make its incorrect
warnings go away. We need to understand which cases fail, both because
the warning is incorrect (in 23/24 cases for my example) or because it
fails to warn when it should (as in the case, above after I corrected
the bug).
jjb
Rob Campbell
> Alex Vincent wrote:
> > Personally, I'm not sure how I feel about this. I hope people are
> > filing bugs on Firebug to clean up these strict warnings...
>
> I tried, back in 2006, when it was part of my SoC project. I don't think those
> changes were taken. I remember talking to others at the time that did patches in
> that regard that were also not taken. From:
>
> John J. Barton wrote:
>
> > Now I know there are folks who believe strongly in 'strict'.
>
> I presume that interest in fixing these things in Firebug itself is still low.
no, I submitted a few fixes yesterday for some of the busiest
messages. I'll keep fixing more as they crop up from my experiments
with debug builds. I think we can get rid of most of them over time
and it's an interesting exercise in understanding the ebb and flow of
object creation in Firebug itself.