1017946 - Kill usage of hasOwnProperty global

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Patrick Cloke [:clokep]]

We should stop using hasOwnProperty and use "safer" constructs (i.e. Maps) instead.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: 1 - Remove XMPP usage [checked in]

Comment 2

[Patrick Cloke [:clokep]]

Attachment: 2 - Remove other usages of hasOwnProperty

Obviously the XMPP part has to land first since this does the actual removal from imXPCOMUtils.

Comment 3

[Patrick Cloke [:clokep]]

Right, and the XMPP part switches back to actually using obj.hasOwnProperty, I think this is OK since we get an object anyway, but it is an object received from the network...so...I'm not entirely sure if this is OK or not. (I.e. Twitter would be really stupid to name one of the fields in their JSON response "hasOwnProperty")

Comment 4

[aleth [:aleth]]

Comment on attachment 8431264 [details] [diff] [review]
1 - Remove XMPP usage [checked in]

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

I don't understand your previous comment on the XMPP part.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1105,5 @@
>      for each (let qe in aStanza.getChildren("query")) {
>        if (qe.uri != Stanza.NS.roster)
>          continue;
>  
> +      // Find all the rosters items in the new message.

Typo: roster items

@@ +1115,3 @@
>        }
> +      // If an item was in the old roster, but not in the new, forget it.
> +      let savedRoster = Object.keys(this._buddies);

this._buddies may also be worth converting, there's a hasOwnProperty call on it elsewhere in this file?

Comment 5

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #4)
> Comment on attachment 8431264 [details] [diff] [review]
> 1 - Remove XMPP usage
> 
> Review of attachment 8431264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand your previous comment on the XMPP part.
Do you mean comment #4? This should read "Twitter", not XMPP.

> @@ +1115,3 @@
> >        }
> > +      // If an item was in the old roster, but not in the new, forget it.
> > +      let savedRoster = Object.keys(this._buddies);
> 
> this._buddies may also be worth converting, there's a hasOwnProperty call on
> it elsewhere in this file?
There is a separate patch in my queue which converts _buddies, _conv and _mucs. It's unrelated to removing hasOwnProperty, however.

Comment 6

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #5)
> > I don't understand your previous comment on the XMPP part.
> Do you mean comment #4? This should read "Twitter", not XMPP.
OK.

> > this._buddies may also be worth converting, there's a hasOwnProperty call on
> > it elsewhere in this file?
> There is a separate patch in my queue which converts _buddies, _conv and
> _mucs. It's unrelated to removing hasOwnProperty, however.
Right, sorry, it doesn't use the global.

Comment 7

[aleth [:aleth]]

Comment on attachment 8431265 [details] [diff] [review]
2 - Remove other usages of hasOwnProperty

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

::: chat/protocols/twitter/twitter.js
@@ +1039,5 @@
>      };
>  
>      let tooltipInfo = [];
>      for (let field in kFields) {
> +      if (userInfo.hasOwnProperty(field) && userInfo[field]) {

Why don't we simply use Object.prototype.hasOwnProperty and not spend any time thinking about what twitter might do.

::: im/content/conversation.xml
@@ +1281,5 @@
>  
>        <!-- Array of lowercase nicks kept in the order in which they are
>             displayed in the participant list.
>             This array is for performance optimization only. To obtain the
> +           capitalized list of nicks use [b for (b of this.buddies.keys())]. -->

"...use the keys of this.buddies" is enough here I think.

::: mail/components/im/content/imconversation.xml
@@ +143,4 @@
>            // Return without doing anything.
>            return;
>          }
>  

Wow, this file is out of date :-/

Comment 8

[Patrick Cloke [:clokep]]

Attachment: 2 - Remove other usages of hasOwnProperty v2 [checked in]

Comment 9

[aleth [:aleth]]

Comment on attachment 8432233 [details] [diff] [review]
2 - Remove other usages of hasOwnProperty v2 [checked in]

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

Thanks!

Comment 10

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #3)
> it is an object
> received from the network...so...I'm not entirely sure if this is OK or not.

It's not OK to assume data from the network is correct.

Comment 11

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/80203d332229
https://hg.mozilla.org/comm-central/rev/1023b47fd0ff

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Other usages interdiff

I messed up and committed attachment 8431265 [details] [diff] [review] instead of attachment 8432233 [details] [diff] [review], so this is the interdiff between them (https://bugzilla.mozilla.org/attachment.cgi?oldid=8431265&action=interdiff&newid=8432233&headers=1) to fix the code to how I actually had it when getting r+. Oops.

Comment 13

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/2608fae8b13b