489889 - Remove checks for statement state in StatementJSHelper

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I noticed they were not tested in lcov reports, but really, we don't even need to do the checks.  All of our public API functions, which this code use, do these checks anyway, so we can save a virtual method call.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 374359 [details] [diff] [review]
v1.0

Switching to bent for reviewer to make asuth's life a bit easier.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 374359 [details] [diff] [review]
v1.0

Why not rework these to be assertions? That way you can ensure that future code changes don't break you.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> (From update of attachment 374359 [details] [diff] [review])
> Why not rework these to be assertions? That way you can ensure that future code
> changes don't break you.
I could indeed turn these into debug assertions, but I'd also wager that if I ever made the public API not check this, I'd be causing lots of other problems too.  If you want these to be debug asserts, let me know.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'd do it, yeah. Defense in depth.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

addresses comments

Comment 6

[Shawn Wilsher :sdwilsh]

Of course, the downside to v1.1 is that we now fail xpcshell tests because I have tests for this :(

I can't really land v1.1 because of this, so what do you want to do?

Comment 7

[Shawn Wilsher :sdwilsh]

...and I forgot to qrefresh after adding semicolons after NS_ASSERTION calls.  It's in my local version.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

In my haste to get an updated patch yesterday, I did something stupid.  After looking closer at the test, I realized that the assertion was just backwards.  This passes tests, with the assertions, as the original review asked.

Comment 9

[Shawn Wilsher :sdwilsh]

Managed to forget to update the commit message, but this is pushed:
http://hg.mozilla.org/mozilla-central/rev/90ac3a21ea49