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)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox24 affected)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox24 | --- | affected |
People
(Reporter: AndreeaMatei, Assigned: mustafa_adam1990, Mentored)
References
Details
Attachments
(2 files, 16 obsolete files)
186.43 KB,
patch
|
Details | Diff | Splinter Review | |
70.36 KB,
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?
Reporter | ||
Updated•11 years ago
|
status-firefox24:
--- → affected
Whiteboard: [lib][enhancement][mentor=andreea.matei][lang=js][good first bug]
Comment 1•11 years ago
|
||
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]
Comment 2•11 years ago
|
||
Hi Andreea,
Can you please provide more details to start working on this bug?
Flags: needinfo?(andreea.matei)
Reporter | ||
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
(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
Reporter | ||
Comment 5•11 years ago
|
||
> * parameters types are indicated by {Type}, and arrays by {Type[]}
>
As an update here, {Type} is with lowercase, like: {function}, {object} and so on.
Updated•10 years ago
|
Mentor: andreea.matei
Whiteboard: [lib][mentor=andreea.matei][lang=js][good first bug] → [lib][lang=js][good first bug]
Reporter | ||
Comment 7•10 years ago
|
||
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?
Reporter | ||
Updated•10 years ago
|
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?
Reporter | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Hi Andreea, I have attached the patch_file.
Assignee | ||
Comment 12•10 years ago
|
||
Hello, I have updated the patch_file, please review and please let me know for corrections. Thanks
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8447360 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
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`.
Reporter | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8447627 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8447890 -
Attachment is obsolete: true
Reporter | ||
Comment 18•10 years ago
|
||
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-
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8447249 -
Attachment is obsolete: true
Attachment #8447917 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8448004 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8448005 -
Attachment is obsolete: true
Attachment #8448007 -
Flags: review?(andreea.matei)
Reporter | ||
Comment 22•10 years ago
|
||
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-
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8448007 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8448447 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8448449 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8448453 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Hi Andreea, please review patch after corrections.
Attachment #8448476 -
Attachment is obsolete: true
Attachment #8448487 -
Flags: review?(andreea.matei)
Reporter | ||
Comment 28•10 years ago
|
||
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-
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8448487 -
Attachment is obsolete: true
Attachment #8449161 -
Flags: review?(andreea.matei)
Attachment #8449161 -
Flags: review?(andreea.matei)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8449161 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
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)
Assignee | ||
Comment 32•10 years ago
|
||
Reporter | ||
Comment 33•10 years ago
|
||
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-
Comment 34•10 years ago
|
||
(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?
Reporter | ||
Comment 35•10 years ago
|
||
Oh right, as with comment 0 all have to be @param. Got confused with older code where we have @config.
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8449594 -
Attachment is obsolete: true
Attachment #8451100 -
Flags: review?(andreea.matei)
Assignee | ||
Comment 37•10 years ago
|
||
The patch replaces @config with @param.
Attachment #8451203 -
Flags: review?(andreea.matei)
Comment 38•10 years ago
|
||
Test run pass after the patch #3963 merge.
Comment 39•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8451100 -
Attachment is obsolete: true
Attachment #8451100 -
Flags: review?(andreea.matei)
Reporter | ||
Comment 40•10 years ago
|
||
Hi Mustafa, do you have any troubles in updating the patch? Please let me know as we're very close to fix it :)
Comment 41•9 years ago
|
||
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]
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•