Closed Bug 1121471 Opened 7 years ago Closed 7 years ago

Remove use of expression closures (shorthand function syntax) from browser-ctrlTab.js

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: dao, Assigned: vikramcse.10, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1120159 +++

Expression closures (shorthand function syntax) are a nonstandard language extension we'd like to remove from SpiderMonkey.

In browser-ctrlTab.js, this affects the selected, isOpen, tabCount and tabPreviewCount getters:

https://hg.mozilla.org/mozilla-central/annotate/21edbaa22144/browser/base/content/browser-ctrlTab.js#l187

e.g.:

get selected () this._selectedIndex < 0 ?
                  document.activeElement :
                  this.previews.item(this._selectedIndex),

should be replaced with:

get selected () {
  return this._selectedIndex < 0 ?
           document.activeElement :
           this.previews.item(this._selectedIndex);
},
OS: Linux → All
Hardware: x86_64 → All
Hi, I am a first time beginner and I would like to work on this bug :) I will need some mentorship to get setup.
(In reply to Adrian C from comment #1)
> Hi, I am a first time beginner and I would like to work on this bug :) I
> will need some mentorship to get setup.

Hi. Have you read <https://developer.mozilla.org/en-US/docs/Introduction>? For any fundamental questions e.g. about how to build Firefox or how to generate a patch, it's probably best to ask in the #introduction chat room on irc.mozilla.org. If anything is unclear about this particular bug, I'm the best person to ask -- please don't hesitate.
Hi I am a beginner.
I would like to work on this bug.
I already build the firefox code.
Thanks.
As long as this bug isn't assigned to a particular person, feel free to upload a patch or ask questions.
Hi
I updated the code
so are there any tests so i can be sure about it?
Yes, there's browser/base/content/test/general/browser_ctrlTab.js
(In reply to Dão Gottwald [:dao] from comment #6)
> Yes, there's browser/base/content/test/general/browser_ctrlTab.js

You can run that this way:

./mach mochitest-browser browser/base/content/test/general/browser_ctrlTab.js
Here i uploaded the patch.
removed all the expression closures.
And the tests had no error.
Please let me know if there is any problem or wrong step.
Thanks
Attachment #8550268 - Flags: review?(dao)
Comment on attachment 8550268 [details] [diff] [review]
Bug1121471_Remove_use_of_expression_closures.diff

There are some tab stops in there messing up the indentation ... please use spaces instead. Looks good otherwise.
Attachment #8550268 - Flags: review?(dao) → review+
Is this looks good now?
If any query please let me know?
thanks.
Attachment #8550282 - Flags: review?(dao)
Comment on attachment 8550282 [details] [diff] [review]
Bug1121471_Remove_use_of_expression_closures.diff

>+    return this._selectedIndex < 0 ?
>+	    document.activeElement :
>+	    this.previews.item(this._selectedIndex);

Still two tabs left :)
The last two lines should be indented like I did in comment 0.
Attachment #8550282 - Flags: review?(dao)
Actually I updated the indentation of code but somehow my queue is not updated so i created new queue.
so is this sounds good?
Attachment #8550298 - Flags: review?(dao)
Comment on attachment 8550298 [details] [diff] [review]
Bug1121471_Remove_use_of_expression_closure_rev2.diff

This looks good, but in order to land this we'll need all changes in one patch.
Attachment #8550298 - Flags: review?(dao) → review+
Sorry but how to do that can you tell me!!
(In reply to Vikram Jadhav from comment #14)
> Sorry but how to do that can you tell me!!

I don't use mercurial queues myself, so I'm not sure what went wrong. It's probably best to ask in the #introduction channel on irc.mozilla.org.
Assignee: nobody → vikramcse.10
Vikram, were you able to get any help yet? You can easily join the #introduction channel here: https://chat.mibbit.com/?url=irc%3A%2F%2Firc.mozilla.org%2F%23introduction
All changes into one file.
Thanks.
Attachment #8551274 - Flags: review?(dao)
Comment on attachment 8551274 [details] [diff] [review]
Bug1121471_Remove_use_of_expression_closure_rev3.diff

>-	  get selected () {
>-	    return this._selectedIndex < 0 ?
>-		     document.activeElement :
>-		     this.previews.item(this._selectedIndex);
>-	  },
>-	  get isOpen () {
>-	    return this.panel.state == "open" || this.panel.state == "showing" || this._timer;
>-	  },
>-	  get tabCount () {
>-	    return this.tabList.length;	
>-	  },
>-	  get tabPreviewCount () {
>-	    return Math.min(this.maxTabPreviews, this.tabCount);	
>-	  },
>-	  get tabList () {
>-	    return this._recentlyUsedTabs;
>-	  },

Hmm, this patch says you removed lines with tab stops here, but I don't see these tab stops here: https://hg.mozilla.org/mozilla-central/file/tip/browser/base/content/browser-ctrlTab.js#l187

>+   get selected () {
>+     return this._selectedIndex < 0 ?
>+              document.activeElement :
>+	      this.previews.item(this._selectedIndex);
>+     },
>+     get isOpen () {
>+       return this.panel.state == "open" || this.panel.state == "showing" || this._timer;
>+     },
>+     get tabCount () {
>+       return this.tabList.length;	
>+     },
>+     get tabPreviewCount () {
>+       return Math.min(this.maxTabPreviews, this.tabCount);	
>+     },
>+     get tabList () {
>+       return this._recentlyUsedTabs;
>+     },

All lines but the first one are indented too far. Also, there are tab stops after "return this.tabList.length;" and "return Math.min(this.maxTabPreviews, this.tabCount);" that shouldn't be there.
Attachment #8551274 - Flags: review?(dao)
  get selected () {
    return this._selectedIndex < 0 ?
             document.activeElement :
             this.previews.item(this._selectedIndex);
  },
  get isOpen () {
    return this.panel.state == "open" || this.panel.state == "showing" || this._timer;
  },
  get tabCount () {
    rerurn this.tabList.length
  },
  get tabPreviewCount () {
    return Math.min(this.maxTabPreviews, this.tabCount);
  },

what about the indentation, is it right?
(In reply to Vikram Jadhav from comment #19)
>   get selected () {
>     return this._selectedIndex < 0 ?
>              document.activeElement :
>              this.previews.item(this._selectedIndex);
>   },
>   get isOpen () {
>     return this.panel.state == "open" || this.panel.state == "showing" ||
> this._timer;
>   },
>   get tabCount () {
>     rerurn this.tabList.length
>   },
>   get tabPreviewCount () {
>     return Math.min(this.maxTabPreviews, this.tabCount);
>   },
> 
> what about the indentation, is it right?

Yes, this looks perfect.
Sorry,
Actually i just messed out my hg queue that's why my code was not updationg.
i think this patch would work!!
Thanks.
Attachment #8551331 - Flags: review?(dao)
Comment on attachment 8551331 [details] [diff] [review]
Bug1121471_removed_indentation_problem

Looks good!
Attachment #8551331 - Flags: review?(dao) → review+
Attachment #8551331 - Attachment is patch: true
Attachment #8551274 - Attachment is obsolete: true
Attachment #8550268 - Attachment is obsolete: true
Attachment #8550298 - Attachment is obsolete: true
Attachment #8550282 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bac9e28a8d6c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.