Ensure that all functions have an internal name for Panorama

RESOLVED FIXED in Firefox 10

Status

defect
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

Trunk
Firefox 10

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Make all functions consistent to have internal name.
Posted patch v1 (obsolete) — Splinter Review
Attachment #556482 - Flags: review?(tim.taubert)
OS: Mac OS X → All
Hardware: x86 → All
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)
Attachment #556482 - Flags: review?(tim.taubert)
(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)
(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...
Posted patch v2 (obsolete) — Splinter Review
Attachment #556482 - Attachment is obsolete: true
Attachment #557116 - Flags: review?(tim.taubert)
This patch should not affect anything since it is adding names for anonymous functions.  Tim: could you review this please?
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 :)
Attachment #557116 - Flags: review?(ttaubert) → review-
Attachment #557116 - Attachment is obsolete: true
Attachment #556482 - Attachment is obsolete: false
Attachment #556482 - Flags: review+
Posted patch Patch for checkin (obsolete) — Splinter Review
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=8b5a76750887
Attachment #556482 - Attachment is obsolete: true
Attachment #565870 - Attachment is obsolete: true
(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!
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/121549dd2e34
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.