970606 - Support the username parameter on setIdentityProvider

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Martin Thomson [:mt:]]

Bug 884573 doesn't support the username parameter of setIdentityProvider().  Mainly because the spec didn't support the feature.  It's not super important, but having the feature is pretty handing for some testing scenarios.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Adding username to setIdentityProvider

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8391416 [details] [diff] [review]
Adding username to setIdentityProvider

There are changes with respect to usernames coming in later bugs, and I'd like to get this relatively simple change out of the way first.

Comment 3

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-970606-Adding-username-to-setIdentityProvider.patch

Getting dependencies cleared up, rebasing, etc...

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8393835 [details] [diff] [review]
0001-Bug-970606-Adding-username-to-setIdentityProvider.patch

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

::: dom/media/tests/identity/idp-proxy.js
@@ +58,5 @@
>      case "SIGN":
> +      if (message.username) {
> +        if (message.username.indexOf("@") < 0) {
> +          this.username = message.username + "@" + this.domain;
> +        } else if (message.username.split("@", 2)[1] === this.domain) {

If username were "[e-mail]@              [e-mail]" it would get through here. Is that a problem?

Also, == is preferred to ===.

@@ +82,2 @@
>      case "VERIFY":
> +      var payload = JSON.parse(message.message);

Any reason we're not using let in this file?

Comment 5

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> Comment on attachment 8393835 [details] [diff] [review]
> 0001-Bug-970606-Adding-username-to-setIdentityProvider.patch
> 
> Review of attachment 8393835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/identity/idp-proxy.js
> @@ +58,5 @@
> >      case "SIGN":
> > +      if (message.username) {
> > +        if (message.username.indexOf("@") < 0) {
> > +          this.username = message.username + "@" + this.domain;
> > +        } else if (message.username.split("@", 2)[1] === this.domain) {
> 
> If username were "[e-mail]@              [e-mail]" it would
> get through here. Is that a problem?
> 
> Also, == is preferred to ===.

Hmm... Why? This is the opposite of Crockford's guidance in
Appendix B of JavaScript: the Good Parts. The argument he offers
for === over == seems pretty compelling to me.




> @@ +82,2 @@
> >      case "VERIFY":
> > +      var payload = JSON.parse(message.message);
> 
> Any reason we're not using let in this file?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Eric Rescorla (:ekr) from comment #5)
> > Also, == is preferred to ===.
> 
> Hmm... Why? This is the opposite of Crockford's guidance in
> Appendix B of JavaScript: the Good Parts. The argument he offers
> for === over == seems pretty compelling to me.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Operators says:
> In JavaScript, == is preferred to ===.

=== seems necessary only to distinguish between 0/false/undefined.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

and emptyish strings.

Comment 8

[Eric Rescorla (:ekr)]

Yes, I know what the style guide says, but the problem is that == does coercions if the objects are not  of the same type. It's not just limited to falsy values (again, this is all in Crockford).  Do you have a technical reason for preferring ==?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yes, this being JS, type-conversions are not abnormal and === is not a panacea, because it is sometimes wrong. For instance, I reviewed a js patch recently that did this:

  if(aInput !== 0) { doSomethingWith(aInput); }

Problem here is it doesn't catch aInput being undefined, which happens alot in js, and is generally expected to be handled (implicitly off not explicitly). If I recall correctly we changed it to:

  if(!aInput) return false;

which is the same as:

  if(aInput != 0) { doSomething(aInput); }

not ===. I don't think outlawing == would work.

Comment 10

[Eric Rescorla (:ekr)]

This seems like an argument for simply testing boolean
arguments directly, rather than for using == versus ===.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yes, and if(aInput) is equivalent to if(aInput != false) not if(aInput !== false), so it seems type-conversion is the norm in JS rather than the exception. I generally only use and see === and !== when detection of undefined as distinct from a value provided matters.

Comment 12

[Eric Rescorla (:ekr)]

The whole thrust of Crockford's argument is that type conversion is a menace,
(and incidentally that the large number of falsy values is a problem), so I'm
not sure this is responsive. In any case, I'll leave it to Martin to decide if
he wants to make this change.

Comment 13

[Martin Thomson [:mt:]]

I'm glad that you guys have run the course of the argument while I was struggling with children.  I'm with Crockford on this one.  Implicit coercion is bad.

Jan-Ivar's example is a case where you can and should use if (aInput) {...}; which is explicit boolean coercion, though maybe better still to use (typeof aInput === 'string') and avoid having to rely on coercion later.

I've seen too many actual bugs caused by lazy type inference to find relying on implicit type conversion sane.  Especially when it comes to numeric types.

'2' + 1 = '21'
'2' - 1 = 1

Comment 14

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> > +        } else if (message.username.split("@", 2)[1] === this.domain) {
> 
> If username were "[e-mail]@              [e-mail]" it would
> get through here. Is that a problem?

It won't.  That's what the 2 is for.

> Any reason we're not using let in this file?

This is content, not chrome.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #13)
> I'm glad that you guys have run the course of the argument while I was
> struggling with children.  I'm with Crockford on this one.  Implicit
> coercion is bad.
> 
> Jan-Ivar's example is a case where you can and should use if (aInput) {...};
> which is explicit boolean coercion,

No it is implicit. aInput is an int in a boolean expression. 

> though maybe better still to use (typeof aInput === 'string') and avoid having to rely on coercion later.

Ugh, I'm still new at JS, but ducktyping rules. Checking types at every corner is neither efficient nor best practice.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #14)
> > > +        if (message.username.indexOf("@") < 0) {
> > > +          this.username = message.username + "@" + this.domain;
> > > +        } else if (message.username.split("@", 2)[1] === this.domain) {
> > > +          this.username = message.username;
> >
> > If username were "[e-mail]@              [e-mail]" it would
> > get through here. Is that a problem?
> 
> It won't.  That's what the 2 is for.

It will, because the domain test succeeds, and then you use the whole string in the next line.

> > Any reason we're not using let in this file?
> 
> This is content, not chrome.

Yeah, but isn't it our content running exclusively in our browser?

Comment 17

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> (In reply to Martin Thomson [:mt] from comment #13)
> > I'm glad that you guys have run the course of the argument while I was
> > struggling with children.  I'm with Crockford on this one.  Implicit
> > coercion is bad.
> > 
> > Jan-Ivar's example is a case where you can and should use if (aInput) {...};
> > which is explicit boolean coercion,
> 
> No it is implicit. aInput is an int in a boolean expression. 

I consider this to be explicit.  if statements take booleans.
 
> > though maybe better still to use (typeof aInput === 'string') and avoid having to rely on coercion later.
> 
> Ugh, I'm still new at JS, but ducktyping rules. Checking types at every
> corner is neither efficient nor best practice.

I said "maybe" intentionally.  You have to be aware of every place that you allow ducktyping to occur.  Otherwise, you are operating with unknown preconditions.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> (In reply to Martin Thomson [:mt] from comment #14)
> > > > +        if (message.username.indexOf("@") < 0) {
> > > > +          this.username = message.username + "@" + this.domain;
> > > > +        } else if (message.username.split("@", 2)[1] === this.domain) {
> > > > +          this.username = message.username;
> > >
> > > If username were "[e-mail]@              [e-mail]" it would
> > > get through here. Is that a problem?
> > 
> > It won't.  That's what the 2 is for.
> 
> It will, because the domain test succeeds, and then you use the whole string
> in the next line.

I could have sworn that I tested this out.  But you are right.

It's not a problem, but I think that we can do better here.  It will make the code less clear, but correctness > clarity, sadly.

> > > Any reason we're not using let in this file?
> > 
> > This is content, not chrome.
> 
> Yeah, but isn't it our content running exclusively in our browser?

None of our mochitests use let.  The marginal value of let over var is so marginal I'd rather be consistent.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #17)
> I consider this to be explicit.  if statements take booleans.

What about if ('2' - 1)

> I said "maybe" intentionally.  You have to be aware of every place that you
> allow ducktyping to occur.  Otherwise, you are operating with unknown
> preconditions.

Preconditions are different from types. Rather than a liability, my understanding of ducktyping is to narrow preconditions to what's relevant to the task at hand. E.g. My code should (nay must) work the same whether I'm passed an array or an object that acts like an array. This is expressly avoiding type-dependence as a design principle and is "webby" I am told.

By inference, I claim == should generally be considered first, unless your code needs ===.

> None of our mochitests use let.  The marginal value of let over var is so
> marginal I'd rather be consistent.

Makes sense.

Comment 19

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18)
> (In reply to Martin Thomson [:mt] from comment #17)
> > I consider this to be explicit.  if statements take booleans.
> 
> What about if ('2' - 1)

Let's say that '2' is a string variable and you have something like this:

if (parseInt(stringVar, 10) - 1) {...}

That's OK.  The point being that relying on implicit coercion === bad.

> > I said "maybe" intentionally.  You have to be aware of every place that you
> > allow ducktyping to occur.  Otherwise, you are operating with unknown
> > preconditions.
> 
> Preconditions are different from types. Rather than a liability, my
> understanding of ducktyping is to narrow preconditions to what's relevant to
> the task at hand. E.g. My code should (nay must) work the same whether I'm
> passed an array or an object that acts like an array. This is expressly
> avoiding type-dependence as a design principle and is "webby" I am told.
> 
> By inference, I claim == should generally be considered first, unless your
> code needs ===.

Type checking is entirely valid if the code assumes a dependency on type.

As I said, it will depend on situation.  Array-like objects can be used in place of Array sometimes.  Until you try to use .map(), .filter(), etc... and find that they aren't there.

Comment 20

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-970606-Adding-username-to-setIdentityProvider.patch

I think that this addresses the comments, other than the ones on which we have a philosophical disagreement (==).

Comment 21

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18)
> By inference, I claim == should generally be considered first, unless your
> code needs ===.

I'm pretty sure you're off in the weeds here. I invite you to take this theory to #developers to see what the consensus view is there.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Adam Roach [:abr] from comment #21)
> I invite you to take this theory to #developers to see what the consensus view is there.

No need, I'm aligned with the official code-style.

Comment 23

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> (In reply to Adam Roach [:abr] from comment #21)
> > I invite you to take this theory to #developers to see what the consensus view is there.
> 
> No need, I'm aligned with the official code-style.

The reason I'm asking you to talk to people on #developers is because I've already been down this route. When I came across this part of the code guidelines, I found it somewhat perplexing -- so I asked around, and determined that, of the people I sampled on the project, all thought that the code guidelines were off-base.

At the time, I didn't have the cycles to determine what accepted process and norms around updating that page were; but I was am fairly certain that what is documented there matches neither practice nor intention of the bulk of Mozilla developers.

See, e.g., http://krijnhoetmer.nl/irc-logs/developers/20130712#l-1101 and http://krijnhoetmer.nl/irc-logs/developers/20130327#l-2264

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We should probably take this to irc and not continue in this bug. I would ask in #content.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a3429366fc

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85a3429366fc