Closed Bug 771086 Opened 8 years ago Closed 8 years ago

4 strict warnings in toolkit/mozapps/extensions/content/extensions.js

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: sgautherie, Assigned: avp)

References

()

Details

(Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js])

Attachments

(1 file, 2 obsolete files)

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1341473203.1341476269.12844.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/07/05 00:26:43
{
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 1481: test for equality (==) mistyped as assignment (=)?
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 1650: anonymous function does not always return a value
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 2302: anonymous function does not always return a value
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 2456: anonymous function does not always return a value
}
Those anonymous functions have since been named:


JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 1481: test for equality (==) mistyped as assignment (=)?

Would rather have this loop converted to a normal for-of loop.



JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 1650: function search_onCommand does not always return a value

I don't its useful for this function to ever return anything.



JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 2302: function gSearchView_getListItemForID does not always return a value
JavaScript strict warning: chrome://mozapps/content/extensions/extensions.js, line 2456: function gListView_getListItemForID does not always return a value

And these should be returning null if a matching list item isn't found.
Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js]
Hi Blair,
I am interested on working on this bug. Could you please guide me on getting started with this bug.

Thanks.
Hi Abhishek!

Do you have a copy of the mozilla-central repository, and a working build yet?
If not, have a read through https://developer.mozilla.org/En/Simple_Firefox_build


Once you've got that, have a look at extensions.js for the functions mentioned in comment 1, it's located at toolkit/mozapps/extensions/content/extensions.js in your repository.
Assignee: nobody → abhishekp.bugzilla
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
Hi Blair,

I have edited for the extensions.js for the last two warnings. I am not getting how to convert the while into a for...of loop. Will it be something like this :

  for (node of node.nextSibling) {
      var nodePriority = parseInt(node.getAttribute("priority"));
      // If the new type's priority is higher than this one then this is the
      // insertion point
      if (aPriority < nodePriority)
        break;
      // If the new type's priority is lower than this one then this is isn't
      // the insertion point
      if (aPriority > nodePriority)
        continue;
      // If the priorities are equal and the new type's name is earlier
      // alphabetically then this is the insertion point
      if (String.localeCompare(aName, node.getAttribute("name")) < 0)
        break;
    }

Sorry but I have never used for...of loop before but I am eager to learn to use it. I have gone through this https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of , I am not being able to figure out the object to be used in the loop. Please help with this.

Also I did not understand what was to be done for the second warning, please help me out with this as well.


Thanks.
Attachment #653262 - Flags: feedback?(bmcbride)
(In reply to Abhishek Potnis [:abhishekp] from comment #4)
>   for (node of node.nextSibling) {

Ah, yes - for...of loops are *very* new :) Sorry, I should have mentioned that. The part after "of" is expected to be an array (or something that you can iterate over).

The line before this sets |node| to the first child of |this.node| - so you want to rewrite both those lines so it's iterating over the children of |this.node| (using its .children property, |this.node.children|, which is basically an array).



> Also I did not understand what was to be done for the second warning, please
> help me out with this as well.

That function doesn't need to return any value (any return value isn't used anywhere), so just need to make it so it never returns a value (ie, a simple "return;").
Comment on attachment 653262 [details] [diff] [review]
Patch v1.0

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2305,5 @@
>        if (listitem.getAttribute("status") == "installed" && listitem.mAddon.id == aId)
>          return listitem;
>        listitem = listitem.nextSibling;
>      }
> +    return null;	 	

Be careful with accidentally adding whitespace at the end of lines - looks like there's 2 spaces and a tab at the end of this one.
Attachment #653262 - Flags: feedback?(bmcbride) → feedback+
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #653262 - Attachment is obsolete: true
Attachment #653344 - Flags: feedback?(bmcbride)
Comment on attachment 653344 [details] [diff] [review]
Patch v2.0

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

Just a couple of fix-ups :)

::: toolkit/mozapps/extensions/content/extensions.js
@@ +1477,5 @@
>      category.setAttribute("priority", aPriority);
>      category.setAttribute("hidden", aStartHidden);
>  
> +    var node;
> +    for (node of this.node.children) {

Instead of declaring |node| outside of the loop, declare it as part of the loop. You can do that using "let" instead of "var", which makes the variable only exist within the loop (var makes it exist for the whole function). ie:
  for (let node of this.node.children) {

See https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/let for some documentation on let.

@@ +1654,3 @@
>  
>        gViewController.loadView("addons://search/" + encodeURIComponent(query));
> +      return;

Don't need this last return here - functions automatically do this when they reach the end.
Attachment #653344 - Flags: feedback?(bmcbride) → feedback+
Attached patch Patch v3.0Splinter Review
Made the suggested changes !
Attachment #653344 - Attachment is obsolete: true
Attachment #653363 - Flags: feedback?(bmcbride)
Comment on attachment 653363 [details] [diff] [review]
Patch v3.0

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

Perfect - thanks :)
Attachment #653363 - Flags: feedback?(bmcbride) → review+
(In reply to Blair McBride (:Unfocused) from comment #8)
> > +    var node;
> > +    for (node of this.node.children) {

Oops, this was correct, not what I suggested - since node is used outside of the loop. Sorry! (This is what I get for doing reviews at 2am)

I'll land the patch with that one change reverted to what you had.
Landed on the fx-team branch, which will get merged into mozilla-central within a day or so:
https://hg.mozilla.org/integration/fx-team/rev/94b79a2eb30d
https://hg.mozilla.org/mozilla-central/rev/94b79a2eb30d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.