Till Schneidereit [:till] Reporter
	Description • 10 years ago

Should be straight-forward to implement, as I don't think we need to change all the String.prototype methods' implementations to work in terms of this. I'm not entirely sure what it's supposed to be used for, but there seems to be consensus about it being useful.

	Till Schneidereit [:till] Reporter
	Comment 1 • 9 years ago

This was renamed.

Arai, perhaps you'd be interested in working on this, given that you're working on several patches in the same general area right now?

	Tooru Fujisawa [:arai] Assignee
	Comment 2 • 9 years ago

Yeah, it's interesting :)
I'll work on this after current RegExp bugs are fixed.

	Tooru Fujisawa [:arai] Assignee
	Comment 3 • 9 years ago

Prepared 4 patches.

Part 0 renames existing IsRegExp function to IsRegExpObject, to avoid name confliction and confusion.

Part 1 adds Symbol.match.

Part 2 implements IsRegExp native function (currently IsRegExp is not used in Self-hosted script).

Part 3 changes IsObjectWithClass call in String.prototype.{startsWith,endsWith} to IsRegExp, and adds IsRegExp to String.prototype.contains.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83eab3bf55d9


Then, IsRegExp is also used in RegExp constructor, with some more related changes, I'll work on it in separated bug.
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp-pattern-flags

	Till Schneidereit [:till] Reporter
	Comment 7 • 9 years ago

Comment on attachment 8581899 [details] [diff] [review]
Part 0: Rename existing IsRegExp to IsRegExpObject.

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

Thank you for splitting this out into its own patch.

	Till Schneidereit [:till] Reporter
	Comment 8 • 9 years ago

Comment on attachment 8581900 [details] [diff] [review]
Part 1: Add Symbol.match.

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

As always, excellent test code!

::: js/src/jsapi.h
@@ +4287,5 @@
>  GetSymbolDescription(HandleSymbol symbol);
>  
>  /* Well-known symbols. */
>  enum class SymbolCode : uint32_t {
> +    iterator,                       // well-knowns

It's a bit weird that this comment is now for multiple lines. Perhaps just add `// Symbol.foo` comments to each line and a comment above all the well-knowns `// Well-known symbols.`?

	Till Schneidereit [:till] Reporter
	Comment 9 • 9 years ago

Comment on attachment 8581902 [details] [diff] [review]
Part 3: Use IsRegExp in String.prototype.{contains,startsWith,endsWith}.

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

And again, excellent tests, and very nice thorough updating of the spec steps; thank you!

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 9 years ago

Thank you for reviewing :)
can I get a review for Part 2 as well?

	Till Schneidereit [:till] Reporter
	Comment 11 • 9 years ago

Comment on attachment 8581901 [details] [diff] [review]
Part 2: Implement IsRegExp.

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

Huh, I had reviewed this, but apparently something went wrong when I sent it off. My apologies for the delay that caused!

	Tooru Fujisawa [:arai] Assignee
	Comment 12 • 9 years ago

Thank you again!
I'll post patches to bug 1147817 shortly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/708f251fadc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9336398400c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb1d70016c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fc40b10421

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/708f251fadc5
https://hg.mozilla.org/mozilla-central/rev/9336398400c5
https://hg.mozilla.org/mozilla-central/rev/2fb1d70016c0
https://hg.mozilla.org/mozilla-central/rev/e2fc40b10421

	Tooru Fujisawa [:arai] Assignee
	Comment 14 • 9 years ago

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla

	Florian Scholz (Open Web Docs)
	Comment 15 • 9 years ago

Thanks, :arai!

I also added https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/match
It could use a review. I didn't come up with a real use case for this as I am not sure I get what it's useful for.

Also, 
> RegExp.prototype[Symbol.match] === function
> false

Is this filed somewhere?

	Tooru Fujisawa [:arai] Assignee
	Comment 16 • 9 years ago

(In reply to Florian Scholz [:fscholz] (MDN) from comment #15)
> Thanks, :arai!
> 
> I also added
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Symbol/match
> It could use a review. I didn't come up with a real use case for this as I
> am not sure I get what it's useful for.
> 
> Also, 
> > RegExp.prototype[Symbol.match] === function
> > false
> 
> Is this filed somewhere?

Thanks, I found bug 887016 for it and all other new RegExp.prototype methods.
I have almost-working patches for @@match, @@search, and @@replace, but they depends on bug 1108382, so it won't come shortly.

For now, it's almost useless, but once all of those methods are implemented, custom RegExp-like object could be made, I guess.