Closed
Bug 491200
Opened 15 years ago
Closed 10 years ago
Mozmill should not use anonymous function names
Categories
(Testing Graveyard :: Mozmill, defect)
Testing Graveyard
Mozmill
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)
63.00 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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) {
Reporter | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
This is pretty old, is this still relevant?
Assignee | ||
Comment 3•11 years ago
|
||
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?
Reporter | ||
Comment 4•11 years ago
|
||
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]
Assignee | ||
Comment 5•11 years ago
|
||
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 () {}
Reporter | ||
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
Hi, just a question. I'm looking at the MozMill repository on GitHub and I'm not seeing the commit for bug 927397.
Reporter | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Sorry for the long delay. I got sidetracked with other things.
Reporter | ||
Comment 12•10 years ago
|
||
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)
Reporter | ||
Comment 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
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.
Reporter | ||
Comment 15•10 years ago
|
||
I appreciate the update Gilles. Good luck with the other code first!
Assignee | ||
Comment 16•10 years ago
|
||
I have a question, do I add the function names in such a case: var event = { notify: function () { ?
Reporter | ||
Comment 17•10 years ago
|
||
(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().
Assignee | ||
Comment 18•10 years ago
|
||
I submitted a new patch following the discussions in the previous review.
Attachment #8378005 -
Attachment is obsolete: true
Attachment #8397467 -
Flags: review?(hskupin)
Reporter | ||
Comment 19•10 years ago
|
||
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-
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8397467 -
Attachment is obsolete: true
Attachment #8398833 -
Flags: review?(hskupin)
Assignee | ||
Comment 21•10 years ago
|
||
Thanks for all the work/time you put in to review my patches! I appreciate it.
Reporter | ||
Comment 22•10 years ago
|
||
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+
Reporter | ||
Comment 23•10 years ago
|
||
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]
Updated•8 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•