Closed Bug 882137 Opened 11 years ago Closed 9 years ago

Refactor the jsdoc for mozmill-tests repository

Categories

(Mozilla QA Graveyard :: Mozmill Tests, enhancement)

enhancement
Not set
normal

Tracking

(firefox24 affected)

RESOLVED WONTFIX
Tracking Status
firefox24 --- affected

People

(Reporter: AndreeaMatei, Assigned: mustafa_adam1990, Mentored)

References

Details

Attachments

(2 files, 16 obsolete files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
andrei
: review-
Details | Diff | Splinter Review
At the moment we're not completely consistent with our jsdoc, especially in the shared modules. We have to update it so that: * parameters description are in a new line and indented * optional parameters are indicated by [parameter=default_value] * parameters types are indicated by {Type}, and arrays by {Type[]} * constructors have the description and the "@constructor" tag, like: /** * Creates a new instance of.. * @constructor */ * parameters with multiple properties (like aSpec, with type, subtype, value properties) should have each of the properties under the @param propery + description Do we really want this or should leave it like is (intended under the main parameter)? Henrik, Dave, do you have any other suggestions?
Whiteboard: [lib][enhancement][mentor=andreea.matei][lang=js][good first bug]
Only that the @returns line should look like "{Type} Short description"
Severity: normal → enhancement
Whiteboard: [lib][enhancement][mentor=andreea.matei][lang=js][good first bug] → [lib][mentor=andreea.matei][lang=js][good first bug]
Hi Andreea, Can you please provide more details to start working on this bug?
Flags: needinfo?(andreea.matei)
It's regarding the js doc we have before each method in the libraries, which describes it. The changes needed are explained in comment 0 and 1. It's basically refactoring, we need to customize a bit those comments. Let me know if you have other questions.
Flags: needinfo?(andreea.matei)
(In reply to Andreea Matei [:AndreeaMatei] from comment #0) > * parameters with multiple properties (like aSpec, with type, subtype, value > properties) should have each of the properties under the @param propery + > description Hi Andreea, for this particular situation there is a new @config tag for documenting parameters with multiple properties. https://code.google.com/p/jsdoc-toolkit/wiki/DocExamples#Parameters
> * parameters types are indicated by {Type}, and arrays by {Type[]} > As an update here, {Type} is with lowercase, like: {function}, {object} and so on.
Mentor: andreea.matei
Whiteboard: [lib][mentor=andreea.matei][lang=js][good first bug] → [lib][lang=js][good first bug]
Hi, can I be assigned to this bug?
Mustafa, please install Mozmill and clone mozmill-tests repository first, like instructed here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests Then you need to open every library file in the lib folders and update the jsdoc (comments before methods) like mentioned in comment 0. Are you familiar with mercurial, hg, creating patches?
Assignee: nobody → mustafa_adam1990
Status: NEW → ASSIGNED
Hi Andreea, I am familiar, and have already started working on this.
Hi Andreea, I believe I have resolved this Bug, can you check to make sure that everything is done?
Well, please attach the patch and ask review from myself in the review field. To create you patch, you can see: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#The_review_process Basically you have to do: hg qnew Patch_name hg qdiff (to see your changes) hg qrefresh -m "Bug Number - description. r=amatei" hg export tip> patchname.patch That patch you attach here.
Attached patch Bug 882137 patch file. (obsolete) (deleted) — Splinter Review
Hi Andreea, I have attached the patch_file.
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Hello, I have updated the patch_file, please review and please let me know for corrections. Thanks
Attached patch 8447360-patch_update (obsolete) (deleted) — Splinter Review
Attachment #8447360 - Attachment is obsolete: true
Hi Andreea, Every time I made the change and try to create a new patch. I get the below error: >hg qnew pathc_update $') was unexpected at this time. transaction abort! rollback completed abort: pretxncommit.whitespace hook exited with status 1 I have attached the patch_update and patch_file using `hg diff > filename`, but cannot find the missing $') using `hg qnew patch_update`.
You will just have to do 'hg qrefresh' and export it to the same patch file after you do some more changes to the repo. The qnew command is only once, we work with one patch only. Please work on the whole repository and add the patch when all the files are done.
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8447627 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8447890 - Attachment is obsolete: true
Comment on attachment 8447917 [details] [diff] [review] patch_update Review of attachment 8447917 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for you patch Mustafa! A lot of work you've done here. Please obsolete the patches when you attach a new one and add my name in the review ? field (andreea matei and you'll get autosuggestions) so I can receive a notification. Overall, what we need further: All the descriptions to be aligned under the opening bracket from the element type: @param {type} aParameter Description The type of the parameters to be with lowercase: for example {string} All cases with subtypes/subparameters to have the @param tag to all of them, like here: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/toolbars.js#l154 ::: firefox/lib/addons.js @@ +68,5 @@ > /** > * Get the controller of the window > * > + * @returns {MozMillController} > + * Mozmill Controller No need to change anything at the @returns lines, just for the parameters and types. Please remove all the changes to @returns. @@ +1305,5 @@ > }; > > /** > + * @class > + * Class to handle the addons.mozilla.org webpage No need for this change, we don't use indentations like this. @@ +1396,3 @@ > * @constructor > + * @param {MozMillController} aBrowserController > + * Controller of the browser window Please align the description (the second line) to start from the "{" symbol. Same for every case modified, similar with this: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/toolbars.js#l21 @@ +1421,5 @@ > * Check if the URL contains the specified SRC attribute > * > + * @param {ElemBase} aElement > + * Element to check for the install source > + * @returns {String} The types must be with lowercase, so "elembase", "string" and so on. @@ +1484,5 @@ > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] > * Here we also want changes, to have them like this: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/toolbars.js#l154 @@ +1506,5 @@ > * [optional - default: ""] > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] Same here. @@ +1763,5 @@ > } > } > > /** > + * Handles the modal dialogue to install an add-on nit: "dialog" ::: firefox/lib/modal-dialog.js @@ +148,2 @@ > * > * @param {object} aWindow [optional - default: null] We want to change this [optional - default] cases with this form: [parameter=default_value] In this case would be [aWindow=null]. Please modify everywhere you find "optional - default", you can seach/grep for it. ::: firefox/lib/prefs.js @@ +163,5 @@ > * @param {object} spec > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property Same changes here as in: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/toolbars.js#l154
Attachment #8447917 - Flags: review-
Attached file patch_update (obsolete) (deleted) —
Attachment #8447249 - Attachment is obsolete: true
Attachment #8447917 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448004 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448005 - Attachment is obsolete: true
Attachment #8448007 - Flags: review?(andreea.matei)
Comment on attachment 8448007 [details] [diff] [review] patch_update Review of attachment 8448007 [details] [diff] [review]: ----------------------------------------------------------------- Did this partial review as most cases are similar. You want to find and replace all cases of elembase / mozmillcontroller with elemBase and mozmillController. Same for ote Please make the indentation to be aligned with the opening bracket '{'. The return lines don't need the description on a new line, please leave them as they were. And also look for [optional - default] and make sure to replace them as mentioned before. Thanks! ::: firefox/lib/addons.js @@ +67,5 @@ > > /** > * Get the controller of the window > * > + * @returns {mozmillcontroller} Mozmill Controller Please leave camelcase the second name: mozmillController @@ +76,5 @@ > > /** > * Get the instance of the Discovery Pane > * > + * @returns {discoverypane} Instance of the Discovery Pane class discoveryPane @@ +324,5 @@ > * Information on which add-on to operate on > * Elements: addon - Add-on element > * > * @returns True if the add-on is compatible > + * @type {elembase} elemBase @@ +873,5 @@ > /** > * Check if a search is active > * > + * @returns > + * State of the search This wasn't reverted. @@ +886,5 @@ > * Retrieve the currently selected search filter > * > + * @returns > + * Element which represents the currently selected search filter > + * @type {elembase} Same here @@ +1082,5 @@ > + * [optional - default: ""] > + * @config {string} [value] - Value of the attribute to filter > + * [optional - default: ""] > + * @config {element} [parent] - Parent of the to find element > + * [optional - default: document] Please remove the optional line, and update it as mentioned before: [parameter=defaultalue]. Here would be [parent=document], after {element} @@ +1104,5 @@ > + * [optional - default: ""] > + * @config {string} [value] - Value of the attribute to filter > + * [optional - default: ""] > + * @config {element} [parent] - Parent of the to find element > + * [optional - default: document] Same here and all cases. @@ +1366,5 @@ > * @constructor > + * @param {mozmillcontroller} aBrowserController > + * Controller of the browser window > + * @param {chromewindow} aWindow > + * Window of the embedded browser element. Indentation aligned to "{" please. @@ +1389,5 @@ > /** > * Check if the URL contains the specified SRC attribute > * > + * @param {elembase} aElement > + * Element to check for the install source Same here. @@ +1589,5 @@ > /** > * Disables an addon using back-end code > * > + * @param {string} aAddonId > + * Id for the addon to be disabled And here, please check all the cases. ::: firefox/lib/downloads.js @@ +73,5 @@ > /** > * Returns the number of currently active private downloads > * > + * @returns {Number} > + * Number of active downloads {number} and indentation. @@ +216,5 @@ > /** > * Get a list of normal or private downloads > * > + * @param {MozIStorageConnection} aDBConnection > + * Where downloads are stored Indentation @@ +384,5 @@ > + * @param {mozmillcontroller} controller > + * mozmillcontroller of the window to operate on > + * @param {DownloadState} state > + * Expected state of the download > + * @param {nmber} timeout nit: number. And indentation ::: firefox/lib/localization.js @@ +93,5 @@ > * Checks if the XUL boxObject has screen coordinates outside of > * the screen coordinates of its parent. If there's no parent, return. > * > + * @param {node} > + * child This doesn't need any change, it was fine before on the same line. @@ +169,5 @@ > * Filters out nodes which should not be tested because they are not in the > * current access key scope. > * > + * @param {node} > + * node Same here @@ +206,5 @@ > /** > * Filters out nodes which should not be tested because they are not visible > * > + * @param {node} > + * node And here @@ +275,5 @@ > * This function calls the screenshot.create method if there is at least one > * box. > * > + * @param {array of array of int} > + * boxes No need to change ::: firefox/lib/modal-dialog.js @@ +12,5 @@ > * Observer object to find the modal dialog spawned by a controller > * > * @constructor > + * @class > + * Observer used to find a modal dialog No need to change @@ +81,5 @@ > + * Not used. > + * @param {string} aTopic > + * Not used. > + * @param {string} aData > + * Not used. Indentation. @@ +143,5 @@ > * Creates a new instance of modalDialog. > * > * @constructor > + * @class > + * Handler for modal dialogs No need to change this. @@ +193,5 @@ > > /** > * Wait until the modal dialog has been processed. > * > + * @param {Number} aTimeout (timeout=TIMEOUT_MODAL_DIALOG) {number} please ::: firefox/lib/performance.js @@ +54,5 @@ > * resident {number} - resident memory allocated > * label {string} - label for result > * > + * @returns > + * Result string formatted for output No need to change ::: firefox/lib/screenshot.js @@ +14,5 @@ > * > + * @param {array of array of int} > + * boxes > + * @param {MozmillController} > + * controller No need to change on different lines. {mozmillController}
Attachment #8448007 - Flags: review?(andreea.matei) → review-
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448007 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448447 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448449 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448453 - Attachment is obsolete: true
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Hi Andreea, please review patch after corrections.
Attachment #8448476 - Attachment is obsolete: true
Attachment #8448487 - Flags: review?(andreea.matei)
Comment on attachment 8448487 [details] [diff] [review] patch_update Review of attachment 8448487 [details] [diff] [review]: ----------------------------------------------------------------- We're getting closer. This usually takes a bit longer since is such a big patch, we can't get everything perfect from the first round. I added comments to all the cases so we don't miss anything. Overall: indentation, optional parameters and descriptions that should start with uppercase. Thanks! In the end, please run testruns to check you haven't changed something which causes failures: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Run_via_the_automation_scripts ::: firefox/lib/addons.js @@ +1076,5 @@ > * @param {object} aSpec > * Information of the UI elements which should be retrieved > + * @config {string} type - Identifier of the element > + * @config {string} [subtype] - Attribute of the element to filter > + * [subtype = length] Please move all these optional strings into the first line, so we only need: * @config {string} [subtype=""] - Attribute of the element to filter The default value is the one mentioned after "default:", in [optional - default: ""] the default value is "". Please update everywhere, we don't need another line for this optional. @@ +1329,5 @@ > * @param {object} aSpec > + * Information of the UI elements which should be retrieved > + * @config {string} type - Identifier of the element > + * @config {element} [parent] - Parent of the to find element > + * [parent = document] Where did subtype and value params went? We need those documented too. @@ +1358,5 @@ > /** > * @class Class to handle the Discovery Pane > * @constructor > + * @param {mozmillController} aBrowserController Controller of the browser window > + * @param {chromeWindow} aWindow Window of the embedded browser element. The descriptions should go on a new line, for both. @@ +1380,5 @@ > > /** > * Check if the URL contains the specified SRC attribute > * > + * @param {elemBase} aElement Element to check for the install source Same here. ::: firefox/lib/downloads.js @@ +268,4 @@ > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property These also have to be with @config @@ +314,5 @@ > > /** > * Get the download filenames from the Download Panel > * > + * @param {mozmillController} aController mozmillController of the browser window Description on a new line @@ +340,5 @@ > > /** > * Open the Download Manager > * > + * @param {mozmillController} controller mozmillController of the window to operate on Same here. @@ +376,5 @@ > * Wait for the given download state > * > + * @param {mozmillController} controller > + * mozmillController of the window to operate on > + * @param {DownloadState} state downloadState ::: firefox/lib/modal-dialog.js @@ +139,5 @@ > * > * @constructor > * @class Handler for modal dialogs > * > + * @param {object} aWindow [aWindow=null] Remove the first aWindow and leave just [aWindow=null] ::: firefox/lib/search.js @@ +211,2 @@ > * @param {boolean} saveChanges > + * (optional) If true the OK button is clicked otherwise Cancel By default I see it's false in the code, so we can have [saveChanges=false] and remove (optional) @@ +645,5 @@ > * @param {object} spec > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property These should be with @config ::: firefox/lib/sessionstore.js @@ +73,5 @@ > * @param {object} spec > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property Same here. ::: firefox/lib/tabs.js @@ +494,5 @@ > /** > * Set the current state of the tabs on top setting > * > + * @param {Boolean} > + * True, if tabs should be on top Aligned with { please. @@ +515,5 @@ > > /** > * Close an open tab > * > * @param {String} [aEventType="menu"] string @@ +527,5 @@ > * <dd>A middle mouse click gets synthesized</dd> > * <dt>shortcut</dt> > * <dd>The keyboard shortcut is used</dd> > * </dl> > * @param {Number} [aIndex=selectedIndex] number @@ +591,5 @@ > * @param {object} spec > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property with @config too. @@ +737,5 @@ > * Open element (link) in a new tab > * > + * @param {elem} aTarget > + * Element to interact with > + * @param {String} [aEventType="middleClick" Type of event which triggers the action string. You missed to close the ] bracket and please move the description on a new line. @@ +796,5 @@ > /** > * Open a new tab > * > + * @param {string} [aEventType="menu"] > + * Type of event which triggers the action Indentation at {. ::: firefox/lib/tabview.js @@ +161,5 @@ > * Elements: filter - Type of filter to apply > * (active, title) > * [optional - default: ""] > * value - Value of the element > * [optional - default: ""] These are with @config. @@ +181,5 @@ > * Retrieve the group's title box > * > * @param {object} aSpec > * Information on which group to operate on > * Elements: group - Group element Same here. @@ +326,5 @@ > * Elements: filter - Type of filter to apply > * (active, title) > * [optional - default: ""] > * value - Value of the element > * [optional - default: ""] And here @@ +486,5 @@ > * [optional - default: ""] > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] Same here. @@ +508,5 @@ > * [optional - default: ""] > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] And here. ::: firefox/lib/tests/testAddonsBlocklist.js @@ +37,5 @@ > > tabs.closeAllTabs(controller); > } > > +//Function to test blocked list api. No need for these comments. ::: firefox/lib/tests/testAddonsManager.js @@ +15,5 @@ > name : "MemChaser", > id : "memchaser@quality.mozilla.org" > } > > +// Function to set up the module. Here too, you can remove it. ::: firefox/lib/toolbars.js @@ +268,5 @@ > * @param {object} spec > * Information of the UI element which should be retrieved > * type: General type information > * subtype: Specific element or property > * value: Value of the element or property These should be with @config. ::: firefox/lib/ui/addons_blocklist.js @@ +46,5 @@ > * [optional - default: ""] > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] @config please. @@ +68,5 @@ > * [optional - default: ""] > * value - Value of the attribute to filter > * [optional - default: ""] > * parent - Parent of the to find element > * [optional - default: document] Same here. ::: lib/assertions.js @@ +65,5 @@ > + * Expected string|date to strictly compare with. > + * @param {boolean} aCondition > + * Condition to test. > + * @returns {boolean} Result of the test. > + */ We only need actual and expected as parameters description and the returns line. The rest are not present in this method. @@ +88,2 @@ > // determined by having the same number of owned properties (as verified > + // with object.prototype.hasOwnProperty.call), the same set of keys Please leave these lines as they were. @@ +111,5 @@ > + * Condition to test. > + * @returns {boolean} Result of the test. > + * @throws {error} > + * > + * @returns {boolean} Result of the test. Same here, only need a and b and returns. @@ +125,3 @@ > } > > + //~~~I've managed to break object.keys through screwy arguments passing. Please leave these lines untouched, the first one will break as there's no tostring() method, it's toString(). @@ +172,5 @@ > + * @param {string} aResult.fileName > + * Name of the file in which the assertion failed. > + * @param {string} aResult.function > + * Function in which the assertion failed. > + * @param {number} aResult.linenumber It's aResult.lineNumber @@ +181,3 @@ > */ > Assert.prototype._logFail = function Assert__logFail(aResult) { > + throw new error(aResult.message, aResult.fileName, aResult.linenumber); Same here. @@ +192,5 @@ > + * @param {string} aResult.fileName > + * Name of the file in which the assertion failed. > + * @param {string} aResult.function > + * Function in which the assertion failed. > + * @param {number} aResult.linenumber And here. @@ +396,5 @@ > > /** > * Test if the regular expression matches the string. > * > + * @param {string} astring aString as it was. @@ +430,5 @@ > > /** > * Test if the regular expression does not match the string. > * > + * @param {string} astring Here too. @@ +464,5 @@ > > /** > * Test if the string contains the pattern. > * > + * @param {string} astring And here. @@ +477,2 @@ > */ > +Assert.prototype.contain = function Assert_contain(astring, aPattern, aMessage) { Please modify to aString everywhere as before, we use the a prefix to all parameters. @@ +506,5 @@ > * Test if a code block throws an exception. > * > + * @param {string} aBlock > + * Function to call to test for exception > + * @param {RegEx} aerror aError @@ +523,5 @@ > * Test if a code block does not throw an exception. > * > + * @param {string} aBlock > + * Function to call to test for exception > + * @param {RegEx} aerror And here. @@ +625,5 @@ > + * @param {number} aTimeout > + * Timeout in waiting for evaluation > + * @param {number} aInterval > + * Interval between evaluation attempts > + * @param {object} aThisobject aThisObject @@ +658,5 @@ > + * @param {string} aResult.fileName > + * Name of the file in which the assertion failed. > + * @param {string} aResult.function > + * Function in which the assertion failed. > + * @param {number} aResult.linenumber lineNumber @@ +678,5 @@ > + * @param {number} aTimeout > + * Timeout in waiting for evaluation > + * @param {number} aInterval > + * Interval between evaluation attempts > + * @param {object} aThisobject aThisObject ::: lib/dom-utils.js @@ +102,5 @@ > * for waitFor to wait > * before starting the walk. > * [optional - default: no waiting] > * windowHandler - Window instance > * [only needed for some tests] @config @@ +112,3 @@ > * The function used as an argument for waitFor to > * wait before starting the walk. > * [optional - default: no waiting] These have default options so we use [param=default_value]. Also please leave waitFunction, we only use lowercase on the first letter, not the middle words. @@ +188,5 @@ > * for waitFor to wait > * before starting the walk. > * [optional - default: no waiting] > * windowHandler - Window instance > * [only needed for some tests] @config here too. @@ +229,5 @@ > * for waitFor to wait > * before starting the walk. > * [optional - default: no waiting] > * windowHandler - Window instance > * [only needed for some tests] And here. @@ +364,5 @@ > if (windowHandler.paneId != idSet.id) { > windowHandler.paneId = idSet.id; > > // Wait for the pane's content to load and to be fully displayed > + assert.waitFor(function() { No need for the indentation. ::: lib/forms.js @@ +108,5 @@ > * Entries last accessed after or at this time > * @param {Date} [aSpec.lastUsedEnd] > * Entries last accessed before or at this time > * > + * @returns {Number} Number of results available All these types have to be with lowercase : date, number. ::: lib/graphics.js @@ +217,5 @@ > > +/** > + * @param {Function} [ahexValueToString] > + * Return hex value to string. > + */ The parameter here is value. ::: lib/places.js @@ +54,5 @@ > + * @param ph > + * Store plug-in status. > + * @param tags > + * Plug-in instance object. > + * There are no parameters to this method, please remove. ::: lib/stack.js @@ +14,5 @@ > * as the start frame has been found. The next file in the stack will be the > * frame to use for logging the result. > * > * @memberOf stack > + * @param {Object} [aStartFrame=Components.stack] object @@ +46,5 @@ > /** > * Strip away unwanted stack information > * > + * @param {Object} aStack > + * Stack to process. Same here. ::: lib/tests/testFormData.js @@ +24,5 @@ > function teardownModule(aModule) { > forms.clear(); > } > > +// Function to test form data. No need for these comments in the tests. ::: metrofirefox/lib/popups.js @@ +59,4 @@ > * General type information > * @param {Object} [aSpec.subtype] > * Specific element or property > * @param {Object} [aSpec.value=0] object ::: metrofirefox/lib/ui/flyoutPanel.js @@ +46,2 @@ > * The name of the panel to be closed > * @param {Object} [aSpec] object ::: metrofirefox/lib/ui/toolbars.js @@ +321,5 @@ > /** > * Type a search term into the find bar > * > * @param {string} aSearchTerm > + * string term to search for in the find bar The descriptions can start with uppercase. @@ +484,2 @@ > * Type of event which triggers the action > * @param {Boolean} [aSpec.aForce=false] boolean.
Attachment #8448487 - Flags: review?(andreea.matei) → review-
Attached patch patch_update (obsolete) (deleted) — Splinter Review
Attachment #8448487 - Attachment is obsolete: true
Attachment #8449161 - Flags: review?(andreea.matei)
Attachment #8449161 - Flags: review?(andreea.matei)
Attached patch pfile.patch (deleted) — Splinter Review
Attachment #8449161 - Attachment is obsolete: true
Comment on attachment 8449161 [details] [diff] [review] patch_update Hi Andreea, Created a patch file(attach: pfile) using hg qnew by removing the hooks from the config file. Although I am not able to test run because of the error(attach: error.txt) please review the patch. Thanks!
Attachment #8449161 - Flags: review?(andreea.matei)
Attached file error.txt (obsolete) (deleted) —
Comment on attachment 8449161 [details] [diff] [review] patch_update Review of attachment 8449161 [details] [diff] [review]: ----------------------------------------------------------------- Please go again through the previous review which had all the things you need to change, I see some have not been applied. You can open the "Review" link from the attachment and see better the comments and the whole code. ::: firefox/lib/addons.js @@ +465,5 @@ > * Information about the filter to apply > * Elements: attribute - DOM attribute of the wanted addon > * [optional - default: ""] > * value - Value of the DOM attribute > * [optional - default: ""] As mentioned in my last review, please update all these cases with @config. These are subparameters too, so search for "Elements" in all files and update all instances. @@ +1080,5 @@ > + * [subtype = length] > + * @config {string} [value] - Value of the attribute to filter > + * [value = undefined] > + * @config {element} [parent] - Parent of the to find elemen > + * [parent = document] You understood how we want to have the optional default value, but here's where we want it: Instead of the code below, we want: @config {element} [parent=document] - Parent of the element to find Same everywhere, we can remove the second line, just include the default value in the first one. @@ +1303,5 @@ > > /** > * Retrieve an UI element based on the given specification > + * * @config {string} [value] - Value of the attribute to filter > + * [value = undefined] This is already below, you can remove it from here. @@ +1440,5 @@ > * @param {object} aSpec > * Information of the UI elements which should be retrieved > + * > + * @config {element} [parent] - Parent of the to find element > + * [parent = document] Where are the rest of the parameters here? We still miss type, subtype and value.
Attachment #8449161 - Flags: review?(andreea.matei) → review-
(In reply to Andreea Matei [:AndreeaMatei] from comment #33) > > + * @config {element} [parent] - Parent of the to find elemen > > + * [parent = document] > > You understood how we want to have the optional default value, but here's > where we want it: > Instead of the code below, we want: > > @config {element} [parent=document] - Parent of the element to find No, we don't want that. I don't see that @config is a valid identifier. Where is that change coming from?
Oh right, as with comment 0 all have to be @param. Got confused with older code where we have @config.
Attached patch 3962.patch (obsolete) (deleted) — Splinter Review
Attachment #8449594 - Attachment is obsolete: true
Attachment #8451100 - Flags: review?(andreea.matei)
Attached patch 3963.patch (deleted) — Splinter Review
The patch replaces @config with @param.
Attachment #8451203 - Flags: review?(andreea.matei)
Test run pass after the patch #3963 merge.
Comment on attachment 8451203 [details] [diff] [review] 3963.patch Review of attachment 8451203 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't apply at all. It doesn't appear to be because of bitrot. Not sure why. mustafa, please do the following: > hg qpop > hg pull -u > hg qpush Fix all the conflicts you get, then do: > hg qrefresh ::: firefox/lib/addons.js @@ +110,5 @@ > * Open the Add-ons Manager > * > * @param {object} aSpec > * Information how to open the Add-ons Manager > + * @param {type} [type - default: menu] `type` is a property of `aSpec` This should be marked as following: > @param {string} [aSpec.type="menu"] The type of the element is between {} //string in this case If the element is optional the name is wrapped between [] Please update all elements accordingly.
Attachment #8451203 - Flags: review?(andreea.matei) → review-
Attachment #8451100 - Attachment is obsolete: true
Attachment #8451100 - Flags: review?(andreea.matei)
Hi Mustafa, do you have any troubles in updating the patch? Please let me know as we're very close to fix it :)
mozmill-tests are not used anymore. We switched to Marionette and firefox-ui-tests. So closing out this bug.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Whiteboard: [lib][lang=js][good first bug]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: