682791 - Ensure that all functions have an internal name for Panorama

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Raymond Lee [:raymondlee]]

Make all functions consistent to have internal name.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 556482 [details] [diff] [review]
v1

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

Yeah, that's indeed a good idea and will help us with debugging. But while we're on it, we should do it right and name every anonymous function out there. I marked some occurrences below but there should be some more...

::: browser/base/content/browser-tabview.js
@@ +230,5 @@
>      if (this.isVisible())
>        return;
>  
>      let self = this;
>      this._initFrame(function() {

(anonymous function with out a name)

@@ +284,5 @@
>      while (popup.firstChild && popup.firstChild != separator)
>        popup.removeChild(popup.firstChild);
>  
>      let self = this;
>      this._initFrame(function() {

(anonymous function with out a name)

::: browser/base/content/tabview/groupitems.js
@@ +970,4 @@
>      let self = this;
>  
>      if (!this._undoButtonTimeoutId) {
>        this._undoButtonTimeoutId = setTimeout(function() { 

(anonymous function with out a name)

::: browser/base/content/tabview/search.js
@@ +333,3 @@
>      let self = this;
>      iQ("#search").hide();
>      iQ("#searchshade").hide().click(function(event) {

(anonymous function with out a name)

@@ +336,5 @@
>        if ( event.target.id != "searchbox")
>          hideSearch();
>      });
>      
>      iQ("#searchbox").keyup(function() {

(anonymous function with out a name)

@@ +421,4 @@
>      let self = this;
>      if (this.currentHandler)
>        iQ(window).unbind("keydown", this.currentHandler);
>      this.currentHandler = function(event) self.beforeSearchKeyHandler(event);

(anonymous function with out a name)

@@ +433,4 @@
>      let self = this;
>      if (this.currentHandler)
>        iQ(window).unbind("keydown", this.currentHandler);
>      this.currentHandler = function(event) self.inSearchKeyHandler(event);

(anonymous function with out a name)

@@ +470,5 @@
>      // it is always a <TabItem>. Also note that index is offset
>      // by the number of matches within the window.
>      let item = iQ("<div/>")
>        .addClass("inlineMatch")
>        .click(function(event){

(anonymous function with out a name)

Comment 3

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> ::: browser/base/content/browser-tabview.js
> @@ +230,5 @@
> >      if (this.isVisible())
> >        return;
> >  
> >      let self = this;
> >      this._initFrame(function() {
> 
> (anonymous function with out a name)

I think we should decide the format of the name for those anonymous. For the above case, shall we use

this._initFrame(function initFrame() { 

OR

// since this line is inside function TabView_show(), shall we prefix that function name?
this._initFrame(function TabView_show_initFrame () { 

Furthermore, I don't think we need to add internal name for loops e.g. Array.forEach(function() {})



> 
> @@ +284,5 @@
> >      while (popup.firstChild && popup.firstChild != separator)
> >        popup.removeChild(popup.firstChild);
> >  
> >      let self = this;
> >      this._initFrame(function() {
> 
> (anonymous function with out a name)
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +970,4 @@
> >      let self = this;
> >  
> >      if (!this._undoButtonTimeoutId) {
> >        this._undoButtonTimeoutId = setTimeout(function() { 
> 
> (anonymous function with out a name)
> 
> ::: browser/base/content/tabview/search.js
> @@ +333,3 @@
> >      let self = this;
> >      iQ("#search").hide();
> >      iQ("#searchshade").hide().click(function(event) {
> 
> (anonymous function with out a name)
> 
> @@ +336,5 @@
> >        if ( event.target.id != "searchbox")
> >          hideSearch();
> >      });
> >      
> >      iQ("#searchbox").keyup(function() {
> 
> (anonymous function with out a name)
> 
> @@ +421,4 @@
> >      let self = this;
> >      if (this.currentHandler)
> >        iQ(window).unbind("keydown", this.currentHandler);
> >      this.currentHandler = function(event) self.beforeSearchKeyHandler(event);
> 
> (anonymous function with out a name)
> 
> @@ +433,4 @@
> >      let self = this;
> >      if (this.currentHandler)
> >        iQ(window).unbind("keydown", this.currentHandler);
> >      this.currentHandler = function(event) self.inSearchKeyHandler(event);
> 
> (anonymous function with out a name)
> 
> @@ +470,5 @@
> >      // it is always a <TabItem>. Also note that index is offset
> >      // by the number of matches within the window.
> >      let item = iQ("<div/>")
> >        .addClass("inlineMatch")
> >        .click(function(event){
> 
> (anonymous function with out a name)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #3)
> > >      this._initFrame(function() {
> > 
> > (anonymous function with out a name)
> 
> I think we should decide the format of the name for those anonymous. For the
> above case, shall we use
> 
> this._initFrame(function initFrame() { 
> 
> OR
> 
> // since this line is inside function TabView_show(), shall we prefix that
> function name?
> this._initFrame(function TabView_show_initFrame () {

I think we should use the latter here. We could have a hard time figuring out which anonymous function is on the stack if they don't have absolute names.

> Furthermore, I don't think we need to add internal name for loops e.g.
> Array.forEach(function() {})

This might be a lot of "naming overhead" but those names come without a price so we should tend to name anonymous functions whenever we use them. Array.forEach() can contain failing code as well and life's much easier if we exactly know which function fails...

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 6

[Raymond Lee [:raymondlee]]

This patch should not affect anything since it is adding names for anonymous functions.  Tim: could you review this please?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 557116 [details] [diff] [review]
v2

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

Raymond, can we please take your first patch? I know that was a lot of work naming those anonymous functions but I feel like this adds too much clutter and doesn't yields enough benefit. Additionally we "lose" a lot of blame information when touching all those lines. Patch v1 is totally fine... sorry about that :|

::: browser/base/content/tabview/modules/utils.jsm
@@ +339,5 @@
>    // ----------
>    // Function: overlaps
>    // Whether the <Range> overlaps with the given <Range> or value or not.
>    //
>    // Paramaters

Please fix this typo while you're at it :)

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=8b5a76750887

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 10

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> Created attachment 565870 [details] [diff] [review] [diff] [details] [review]
> Patch for checkin
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=8b5a76750887

Passed Try!

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/121549dd2e34

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/121549dd2e34