Closed Bug 491200 Opened 15 years ago Closed 10 years ago

Mozmill should not use anonymous function names

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: gilles.leblanc)

References

Details

(Whiteboard: [mozmill-2.1][mentor=whimboo][lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090427 Shiretoko/3.5b5pre ID:20090427031112 and Mozmill 1.1

During a debugging session on Friday it was impossible for us to identify all the functions in Venkman because all of them are anonymous. To have a better way in debugging Mozmill all the functions should be changes like this:

From: MozMillController.prototype.click = function(el {
To:   MozMillController.prototype.click = function mc_Click(el) {
Doesn't stop us from creating tests but will give us a much better way of identifying problems in Mozmill itself. Setting as P3.
Priority: -- → P3
This is pretty old, is this still relevant?
I'd be interested in fixing this. 

Do you want it done only in controller.js or every .js file?
Also by all functions are we talking all MozMillController functions, every possible functions, etc?
Hi Gilles. Thank you for your interest in working on this bug. Your help would be really welcome! As it looks like you already found the source yourself. That's great. So no further hints seem to be necessary. I will assign this bug to you then.

Regarding your last questions, we should do this for all the js files, yes. We want to have a clean way of those definitions. Otherwise people are confused and we will most likely not get a coding style setup for that. And this does not only include simple functions but also methods bind to classes or objects.
Assignee: nobody → gilles.leblanc
Status: NEW → ASSIGNED
Priority: P3 → --
Whiteboard: [mentor=whimboo][lang=js][good first bug]
Hi, I have started refactoring the code. 

I have a question, do you also want function names in cases where you are only creating an object to later bind methods unto it?

ie: 
var Assert = function () {}
Hi Gilles, it might have been a bit too early for the refactoring. We still haven't landed the patch on bug 927397, which you really might want to wait for. The merge should happen really soon to the default branch. So this bug should be unblocked soon. Please CC yourself on bug 927397 to get the status updates. Thanks.

(In reply to Gilles Leblanc from comment #5)
> var Assert = function () {}

This case is a constructor and would also have to be converted, yes.
Depends on: 927397
I'll wait until the refactoring for bug 927397 and then merge these changes with my current local changes and submit a patch. 

In the meantime I'll look for another bug.

Thanks for the head's up.
Hi, just a question. I'm looking at the MozMill repository on GitHub and I'm not seeing the commit for bug 927397.
(In reply to Gilles Leblanc from comment #8)
> Hi, just a question. I'm looking at the MozMill repository on GitHub and I'm
> not seeing the commit for bug 927397.

Oh my! You are totally right.  I completely mixed-up the components and thought this bug is for the mozmill-tests repository. Thank you for the enlightenment! And sorry for holding up this bug from being worked on. If you want, you can get started. Correct.
This patch adds function names to most functions. 

It does not add function names where the code seems to be copied verbatim from another project and another license was aposed. 

Some tests functions where I deemed it would not be necessary or confusing were also skipped. 

But overall, I'd say over 90% of all anonymous functions have been refactored.

var name = function() -> have been converted to function name().

Adding functions to prototypes has mostly been done with a prefix like in the original comment/description of the bug.

Other cases were either converted with; 
- an abbreviation prefix 
- with a leading underscore if no abbreviation seemed fitting
- a new method name if one could be found that added information in the current context. 

I have tried to follow the local coding style to each file (or part of a file) as well as the preceding rules.
Sorry for the long delay. I got sidetracked with other things.
Comment on attachment 8378005 [details] [diff] [review]
0001-Bug-491200-Mozmill-should-not-use-anonymous-function.patch

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

Thanks Gilles! I assume the patch is ready to be reviewed? I will set the review flag appropriately, and will try to do it in the next couple of days.
Attachment #8378005 - Flags: review?(hskupin)
Comment on attachment 8378005 [details] [diff] [review]
0001-Bug-491200-Mozmill-should-not-use-anonymous-function.patch

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

Thanks a lot Gilles and sorry for the bit of delay. I hope that you are still interested to come up with the follow-up changes I will mention in my review comments, which you can find inline.

It's really a ton of work! So thanks again! But as you might see we don't want to have names in every case. I called those situation out only once. Please note that they will also apply to other files. That means a lot of changes would have to be reverted.

Let me know if you have questions and if you want to proceed with this bug. Thanks!

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +28,4 @@
>  // For Mozmill 1.5 backward compatibility
>  var windowMap = windows.map;
>  
> +waitForEvents = function _waitForEvents() {

Here we unset a method. I think that this should stay as 'function ()'.

@@ +45,4 @@
>      this.registry = {};
>  
>      for each (var e in events) {
> +      var listener = function eventTypeFired(event) {

Given that we have to keep this function as a variable to be able to pass it as callback, we should keep the code as we had before.

@@ +60,4 @@
>     */
>    wait: function waitForEvents_wait(timeout, interval) {
>      for (var e in this.registry) {
> +      assert.waitFor(function hasBeenFired() {

Sorry, I should have mentioned that the changes as requested here will not apply to callback functions, but only to lines with var declarations. Can you please revert all of those changes? 

Reason is that for callbacks we really need no names set because those are directly passed over and don't have to be pre-defined.

@@ +370,4 @@
>   *
>   * @param {DOMWindow} [aWindow=this.window] Window object to check for loaded state
>   */
> +MozMillController.prototype.isLoaded = function mc_IsLoaded(aWindow) {

nit: mc_isLoaded.

@@ +388,4 @@
>   * Wrapper function to create a new instance of a menu
>   * @see Menu
>   */
> +MozMillController.prototype.getMenu = function mc_GetMenu(menuSelector, document) {

nit: getMenu

@@ +400,4 @@
>    logDeprecated('controller.menus', 'Use controller.mainMenu instead');
>  });
>  
> +MozMillController.prototype.waitForImage = function mc_WaitForImage(aElement, timeout,

nit: mc_waitForImage.

Please adjust the following instances of this type too.

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +143,4 @@
>  
>    this.selector = selector;
>  
> +  this.getNodeForDocument = function getNodeForDocument(s) {

Please add a class prefix to the method similar to the controller class.

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +253,4 @@
>    frame.timers.push(this);
>  }
>  
> +timer.prototype.start = function timerStart(name) {

I would name it 'timer_start'. Similar for the other methods.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +53,4 @@
>    // ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
>    // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>  
> +  _deepEqual: function _equal(actual, expected) {

we should use a prefix like assert__equal. Please note the double underscore given the private method.

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +113,4 @@
>    globalListeners : []
>  }
>  
> +events.setState = function e_SetState(v) {

I would use 'events' as prefix.

::: mozmill/mozmill/extension/resource/stdlib/EventUtils.js
@@ +598,4 @@
>  
>    var type = (aExpectedEvent.charAt(0) == "!") ?
>               aExpectedEvent.substring(1) : aExpectedEvent;
> +  var eventHandler = function isPassed(event) {

No need to modify this file because its an import from mozilla-central.

::: mozmill/mozmill/extension/resource/stdlib/httpd.js
@@ +71,4 @@
>      dumpn("###!!! Stack follows:");
>  
>      var stack = new Error().stack.split(/\n/);
> +    dumpn(stack.map(function printVal(val) { return "###!!!   " + val; }).join("\n"));

No need to update this file too, because we also import it from mozilla-central.
Attachment #8378005 - Flags: review?(hskupin) → review-
Thanks for the review. 

I have something in progress on another open-source project. Once it's done I'll work to fix this patch.
I appreciate the update Gilles. Good luck with the other code first!
I have a question, do I add the function names in such a case:

var event = {
  notify: function () {

?
(In reply to Gilles Leblanc from comment #16)
> var event = {
>   notify: function () {

Best is to also add the internal function name and a reference to the class/object it lives in. So something like that is fine:

> var event = {
>   notify: function event_notify() {

If the class/object name is too long you can find abbreviations, e.g. BrowserWindow -> BW_notify().
Attached patch bug491200.patch (obsolete) — Splinter Review
I submitted a new patch following the discussions in the previous review.
Attachment #8378005 - Attachment is obsolete: true
Attachment #8397467 - Flags: review?(hskupin)
Comment on attachment 8397467 [details] [diff] [review]
bug491200.patch

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

Looks way better. Thanks Gilles! Can you please fix the remaining lines as pointed out inline? Once this is done we can get your patch landed.

::: jsbridge/jsbridge/extension/components/jsbridge.js
@@ +58,4 @@
>  
>          var self = this;
>          var startCallback = {
> +          notify: function observe_notify(timer) {

nit: sc_notify.

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +261,4 @@
>    this.timers[name].startTime = (new Date).getTime();
>  }
>  
> +timer.prototype.stop = function time_stop(name) {

nit: timer_

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +67,4 @@
>    // Use a timer to trigger the application restart, which will allow us to
>    // send an ACK packet via jsbridge if the method has been called via Python.
>    var event = {
> +    notify: function restartApplication(timer) {

Similar as what you used in other modules, we should have 'event_notify' here. Please check similar code in this module.

@@ +456,4 @@
>    this.testing = [];
>  }
>  
> +Collector.prototype.addHttpResource = function c_addHttpResource(aDirectory, aPath) {

I would use 'collector_' for all those methods.
Attachment #8397467 - Flags: review?(hskupin) → review-
Attached patch bug491200b.patchSplinter Review
Attachment #8397467 - Attachment is obsolete: true
Attachment #8398833 - Flags: review?(hskupin)
Thanks for all the work/time you put in to review my patches! I appreciate it.
Comment on attachment 8398833 [details] [diff] [review]
bug491200b.patch

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

Looks good to me. Thanks a lot!
Attachment #8398833 - Flags: review?(hskupin) → review+
Landed on master for Mozmill 2.1:
https://github.com/mozilla/mozmill/commit/15c0dc97fde8f44ae3bd4cbf2ef576239b156ded

Thanks to you too! I hope it was a good first exercise for you, and we can continue in learning.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [mentor=whimboo][lang=js][good first bug] → [mozmill-2.1][mentor=whimboo][lang=js][good first bug]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: