Bug 72875 – remove deprecated -f option of jsc shell

• Status: UNCONFIRMED
• Component: JavaScriptCore

[Andy Wingo]

If you run jsc --help, you will see that there is a -f option that is marked as deprecated.  It was marked as deprecated in 2008 and there are no uses in the current tree.  The patch to be attached removes this option.

[Andy Wingo]

Created attachment 116074 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 116074 [details]
Patch

The “unused” -f argument is passed to jsc by the Mozilla JS test harness used in run-javascriptcore-tests.

[Andy Wingo]

So... should we fix the harness, undeprecate the command-line argument, or add a comment and leave things as they are?

[Alexey Proskuryakov]

Also, this option is part of documented functionality on Mac OS X (jsc --help lists it).

[Andy Wingo]

Alexey, as I noted in the original report, it has been marked in --help as deprecated since 2008.  Surely that is sufficient warning, unless it was never intended to be removed.

[Andy Wingo]

Created attachment 116499 [details]
Patch

[Andy Wingo]

Comment on attachment 116499 [details]
Patch

How about this one, that also fixes jsDriver.pl ?

[Alexey Proskuryakov]

To reviewers: please don't r+ this unless you know that you have the authority to remove documented features from shipping Apple products.

[Andy Wingo]

(In reply to comment #8)
> To reviewers: please don't r+ this unless you know that you have the authority to remove documented features from shipping Apple products.

What is the next positive step to take here, Alexey?  Wrap this block in an #if PLATFORM(MAC)?  Does mrowe have this authority or do I need to find someone else?  Close as invalid?

[Alexey Proskuryakov]

Apple folks CC'ed on this bug can make the decision, possibly consulting someone else. Many of them are traveling this week, and may not have seen the patch yet.

Wrapping this in #if PLATFORM(MAC) doesn't seem great. Every platform specific code path has costs such as being unable to see if you patch breaks other platforms, more difficulty following the logic and such.

[Andy Wingo]

Can someone from Apple take a look at this small patch, please?

Thanks :-)

Andy

[Geoffrey Garen]

jsc is an internal testing tool, not Mac OS X API.

You can remove -f -- as long as you keep run-javascriptcore-tests working (probably by fixing jsDriver.pl).

You can add and remove features, as dictated by development needs.

If being present on the filesystem @ /System/Library/Frameworks/JavaScriptCore.framework/Versions/Current/Resources/ presents something as API, then neither jsc nor testapi.js should be present there -- that's a separate bug.

[Mark Rowe (bdash)]

Comment on attachment 116499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116499&action=review

> Source/JavaScriptCore/tests/mozilla/jsDriver.pl:17
> +# Copyright (C) 1997-1999, 2011 Netscape Communications Corporation. All

Unless you work for Netscape Communications Corporation, this change isn't appropriate.

[Mark Rowe (bdash)]

(In reply to comment #12)
> If being present on the filesystem @ /System/Library/Frameworks/JavaScriptCore.framework/Versions/Current/Resources/ presents something as API, then neither jsc nor testapi.js should be present there -- that's a separate bug.

testapi.js isn't, and should never have been, installed there. As you mention, jsc is installed there for testing purposes.

[Geoffrey Garen]

> testapi.js isn't, and should never have been, installed there. 

Right.

> As you mention, jsc is installed there for testing purposes.

Sort of. It's for our testing purposes, yes. But Alexey seems to think the location we've chosen accidentally presents our testing helper as API. If so, we should move it.

[Mark Rowe (bdash)]

It doesn't.

[Alexey Proskuryakov]

> Sort of. It's for our testing purposes, yes. But Alexey seems to think the location we've chosen accidentally presents our testing helper as API. If so, we should move it.

A quick Web search for "jsc javascriptcore" demonstrates that people already use it, and teach others how to use it.

I don't have an opinion on whether is OK to break those who use it by deleting it. Perhaps reading the found articles would be informative.

[Andy Wingo]

(In reply to comment #17)
> > Sort of. It's for our testing purposes, yes. But Alexey seems to think the location we've chosen accidentally presents our testing helper as API. If so, we should move it.
> 
> A quick Web search for "jsc javascriptcore" demonstrates that people already use it, and teach others how to use it.
> 
> I don't have an opinion on whether is OK to break those who use it by deleting it. Perhaps reading the found articles would be informative.

I read the first 20 articles, and none of them use -f.

[Andy Wingo]

Created attachment 122640 [details]
Patch

[Andy Wingo]

The newest patch fixes the copyright header.  Mark, r?

[Eric Seidel (no email)]

Comment on attachment 122640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122640&action=review

Based on comment 12 and comment 18, this patch seems to satisfy the requests of the reviewers.

> Source/JavaScriptCore/tests/mozilla/jsDriver.pl:152
> +    my $file_param = " ";

Why is this variable still needed if it's empty?  Just trying to change the harness as little as possible?