720744 - [Mozmill] EventUtils.js: fix some strict warnings and nits

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from bug 720095 comment #7)
> This needs to be fixed in the mozmill repository first. iirc the correct
> component is Testing:Mozmill.

I manage to find out where the mozmill repository is.
But I am not going to bother interacting with it directly ftb.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can you please give more context in what has to be fixed?

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) EventUtils.js: Fix some strict warnings and nits

Ports changes I made to SimpleTest file in bug 720095.

NB:
This patch is based on Thunderbird file.
I am not sure who to request review from.

Comment 3

[Jeff Hammel]

Comment on attachment 591153 [details] [diff] [review]
(Av1) EventUtils.js: Fix some strict warnings and nits

+    if (("type" in aEvent) && aEvent.type) {

Not sure why this is necessary; if not, won't aEvent.type be undefined, which is a false value anyway?  Likewise for subsequent comments

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> +    if (("type" in aEvent) && aEvent.type) {
> 
> Not sure why this is necessary; if not, won't aEvent.type be undefined,
> which is a false value anyway?  Likewise for subsequent comments

(In SimpleTest) It is needed to avoid triggering a javascript strict warning.
I know little about Mozmill, but I assume that applies there too.

Comment 5

[Jeff Hammel]

Well, that's simply awful, but i guess we should do it :(

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jeff Hammel [:jhammel] from comment #3) 
> +    if (("type" in aEvent) && aEvent.type) {
> 
> Not sure why this is necessary; if not, won't aEvent.type be undefined,
> which is a false value anyway?  Likewise for subsequent comments

Yes this is necessary because otherwise (as Serge pointed out) a strict JS warning gets thrown. Serge, have you checked the original version of that file in mozilla-central if it is also affected? We should fix it there too.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from bug 720095 comment #7)
> This needs to be fixed in the mozmill repository first.

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Serge, have you checked the original version of that
> file in mozilla-central if it is also affected? We should fix it there too.

Iiuc Mark's comment, the original file is the Mozmill one,
then the (3) copies in m-c should be updated (in a follow-up bug(s)) after Mozmill source has been fixed...

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, we don't have the original version of that file. This is really maintained on mozilla-central by devs. We simply forked it and haven't updated the file for a long time. Could be that most of those issues have already been fixed and new stuff has been added which is not part of our version yet.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> This is really maintained on mozilla-central by devs.

Then I am lost. Which is the original file in m-c precisely?
http://mxr.mozilla.org/comm-central/find?text=&string=%2FEventUtils.js

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It's the one for Mochitest:

http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/tests/SimpleTest/EventUtils.js

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> Ports changes I made to SimpleTest file in bug 720095.

(In reply to Henrik Skupin (:whimboo) from comment #10)
> It's the one for Mochitest:

Woh, going into circle here: let me summarize.

I fixed SimpleTest in bug 720095,
the 3 m-c "copies" have other differences too.
My patch here is to port my changes to Mozmill,
so someone else can later update the 3 m-c copies from it, as Mark suggested.

Wrt the other existing differences, someone else can deal with them, as need be.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jeff, can you please take care of the checkin? This patch sits around for a while now and I'm not that clear about the current state. Thanks.

Comment 13

[Jeff Hammel]

What branches is this needed on?  You also have checkin priveleges, :whimboo

Comment 14

[cmtalbert]

Landed on mozmill upstream:
* master: https://github.com/mozautomation/mozmill/commit/2f462fd36e5dc9097e3170b5fda86980fae7668d
* hotfix-2.0: https://github.com/mozautomation/mozmill/commit/61af56a4f3e187238e2b7d433f420561ce30fb7a
* hotfix-1.5: https://github.com/mozautomation/mozmill/commit/9214700036efc8e6633e5f739581c391eea66517

I noticed the original patch is against comm-central. Does this need to land in comm-central as well?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that the patch has landed across branches we should set the appropriate whiteboard entries.

Comment 16

[Mark Banner (:standard8)]

(In reply to Clint Talbert ( :ctalbert ) from comment #14)
> I noticed the original patch is against comm-central. Does this need to land
> in comm-central as well?

No, we can pick it up next time we upgrade mozmill in comm-central.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So what is left to do here? Seems like that everything has been landed.

Comment 18

[cmtalbert]

Based on comment 16, we're done.