Update marionette server .js modules for function definition style (space before opening bracket)

RESOLVED FIXED in Firefox 52

Status

Testing
Marionette
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: whimboo, Assigned: Thomas Charles, Mentored)

Tracking

({ateam-marionette-server, good-first-bug})

Version 3
mozilla53
ateam-marionette-server, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

7 months ago
Right now the .js modules for Marionette server define methods like that:

> foo = function(arg) {}

This can cause confusion because it could also mean that a function with the name "function" gets called here. To avoid that and to be conform with the Mozilla JS style guide we should change all those instances to:

> foo = function (arg) {}

Andreas, do you want to mentor this bug? Looks like a good-first-bug one.
Flags: needinfo?(ato)
Flags: needinfo?(ato)
(Reporter)

Updated

7 months ago
Mentor: ato@mozilla.com
Keywords: ateam-marionette-server, good-first-bug
(Assignee)

Comment 1

7 months ago
Created attachment 8816753 [details] [diff] [review]
Bug1316975.patch
Attachment #8816753 - Flags: feedback?(ato)
(Assignee)

Comment 2

7 months ago
Hi Andreas,

I'm still relatively new to fixing bugs. I have uploaded a patch of the changes I think are needed.

Could you let me know if this is the correct approach to this bug and if any further changes need to made?

If the patch is looking good, can I have this bug assigned to me.

Thanks.
Flags: needinfo?(ato)
Assignee: nobody → thomascharles94
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Comment on attachment 8816753 [details] [diff] [review]
Bug1316975.patch

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

This looks good!  Thanks.

I’m going to submit this to MozReview and run some relevant try jobs, and if they pass, submit it for integration.

For your next patch, you might want to consider submitting it to MozReview yourself.
Attachment #8816753 - Flags: feedback?(ato) → feedback+
Comment hidden (mozreview-request)

Comment 5

7 months ago
mozreview-review
Comment on attachment 8817057 [details]
Bug 1316975 - Correct function definition style in Marionette components;

https://reviewboard.mozilla.org/r/97492/#review97790
Attachment #8817057 - Flags: review?(ato) → review+
Comment hidden (mozreview-request)
I have requested integration of this now.  Sorry for taking so long.

Comment 8

6 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/250599a34188
Correct function definition style in Marionette components; r=ato

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/250599a34188
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 10

6 months ago
I propose we also get this test-only patch landed on aurora to lessen the work for upcoming uplifts of the 52esr releases.
status-firefox52: --- → affected
Whiteboard: [checkin-needed-aurora]
needs rebasing 

grafting 379302:250599a34188 "Bug 1316975 - Correct function definition style in Marionette components; r=ato"
merging testing/marionette/addon.js
merging testing/marionette/components/marionettecomponent.js and testing/marionette/components/marionette.js to testing/marionette/components/marionettecomponent.js
merging testing/marionette/driver.js
merging testing/marionette/error.js
merging testing/marionette/listener.js
merging testing/marionette/navigate.js
warning: conflicts while merging testing/marionette/addon.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(hskupin)
Whiteboard: [checkin-needed-aurora]
(Reporter)

Comment 12

6 months ago
It might be not worth the time then. I would leave the decision up to Andreas.
status-firefox52: affected → fix-optional
Flags: needinfo?(hskupin)
(Reporter)

Comment 13

6 months ago
Created attachment 8823230 [details] [diff] [review]
uplift.patch

Given that other patches have been uplifted the number of merge conflicts for this patch is only 1. So let go and uplift it to 52, because it would cause lots of conflicts for other upcoming patches we want to uplift.
(Reporter)

Comment 14

6 months ago
Please uplift the test-only patch as updated in attachment 8823230 [details] [diff] [review] to aurora. Thanks.
Whiteboard: [checkin-needed-aurora]

Comment 15

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee386af27b8b
status-firefox52: fix-optional → fixed
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.