Ensure that all functions have an internal name for Panorama

RESOLVED FIXED in Firefox 10

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

Trunk
Firefox 10

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Make all functions consistent to have internal name.
(Assignee)

Comment 1

6 years ago
Created attachment 556482 [details] [diff] [review]
v1
Attachment #556482 - Flags: review?(tim.taubert)
(Assignee)

Updated

6 years ago
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)
(Assignee)

Comment 3

6 years ago
(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...
(Assignee)

Comment 5

6 years ago
Created attachment 557116 [details] [diff] [review]
v2
Attachment #556482 - Attachment is obsolete: true
Attachment #557116 - Flags: review?(tim.taubert)
(Assignee)

Comment 6

6 years ago
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+
(Assignee)

Comment 8

6 years ago
Created attachment 565870 [details] [diff] [review]
Patch for checkin

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

Updated

6 years ago
Attachment #556482 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 565871 [details] [diff] [review]
Patch for checkin
Attachment #565870 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/121549dd2e34
Target Milestone: --- → Firefox 10

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/121549dd2e34
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.