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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

+++ 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);
},

Comment 1

[Adrian C]

Hi, I am a first time beginner and I would like to work on this bug :) I will need some mentorship to get setup.

Comment 2

[Dão Gottwald [:dao]]

(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.

Comment 3

[Vikram Jadhav]

Hi I am a beginner.
I would like to work on this bug.
I already build the firefox code.
Thanks.

Comment 4

[Dão Gottwald [:dao]]

As long as this bug isn't assigned to a particular person, feel free to upload a patch or ask questions.

Comment 5

[Vikram Jadhav]

Hi
I updated the code
so are there any tests so i can be sure about it?

Comment 6

[Dão Gottwald [:dao]]

Yes, there's browser/base/content/test/general/browser_ctrlTab.js

Comment 7

[Dão Gottwald [:dao]]

(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

Comment 8

[Vikram Jadhav]

Attachment: Bug1121471_Remove_use_of_expression_closures.diff

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

Comment 9

[Dão Gottwald [: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.

Comment 10

[Vikram Jadhav]

Attachment: Bug1121471_Remove_use_of_expression_closures.diff

Is this looks good now?
If any query please let me know?
thanks.

Comment 11

[Dão Gottwald [: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.

Comment 12

[Vikram Jadhav]

Attachment: Bug1121471_Remove_use_of_expression_closure_rev2.diff

Actually I updated the indentation of code but somehow my queue is not updated so i created new queue.
so is this sounds good?

Comment 13

[Dão Gottwald [: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.

Comment 14

[Vikram Jadhav]

Sorry but how to do that can you tell me!!

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Dão Gottwald [:dao]]

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

Comment 17

[Vikram Jadhav]

Attachment: Bug1121471_Remove_use_of_expression_closure_rev3.diff

All changes into one file.
Thanks.

Comment 18

[Dão Gottwald [: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.

Comment 19

[Vikram Jadhav]

  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?

Comment 20

[Dão Gottwald [:dao]]

(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.

Comment 21

[Vikram Jadhav]

Attachment: Bug1121471_removed_indentation_problem

Sorry,
Actually i just messed out my hg queue that's why my code was not updationg.
i think this patch would work!!
Thanks.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8551331 [details] [diff] [review]
Bug1121471_removed_indentation_problem

Looks good!

Comment 23

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/bac9e28a8d6c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bac9e28a8d6c