Last Comment Bug 682791 - Ensure that all functions have an internal name for Panorama
: Ensure that all functions have an internal name for Panorama
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-28 23:44 PDT by Raymond Lee [:raymondlee]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (15.27 KB, patch)
2011-08-28 23:46 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
v2 (136.54 KB, patch)
2011-08-31 04:30 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Splinter Review
Patch for checkin (11.76 KB, patch)
2011-10-09 23:53 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Patch for checkin (13.27 KB, patch)
2011-10-10 00:06 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Raymond Lee [:raymondlee] 2011-08-28 23:44:31 PDT
Make all functions consistent to have internal name.
Comment 1 Raymond Lee [:raymondlee] 2011-08-28 23:46:10 PDT
Created attachment 556482 [details] [diff] [review]
v1
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-08-29 01:40:18 PDT
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] 2011-08-29 03:18:50 PDT
(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] (on PTO, back Aug 29th) 2011-08-29 08:07:30 PDT
(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] 2011-08-31 04:30:59 PDT
Created attachment 557116 [details] [diff] [review]
v2
Comment 6 Raymond Lee [:raymondlee] 2011-09-27 20:13:14 PDT
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] (on PTO, back Aug 29th) 2011-10-08 12:24:52 PDT
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] 2011-10-09 23:53:36 PDT
Created attachment 565870 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=8b5a76750887
Comment 9 Raymond Lee [:raymondlee] 2011-10-10 00:06:15 PDT
Created attachment 565871 [details] [diff] [review]
Patch for checkin
Comment 10 Raymond Lee [:raymondlee] 2011-10-10 04:45:17 PDT
(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 12 Marco Bonardo [::mak] 2011-10-11 02:26:40 PDT
https://hg.mozilla.org/mozilla-central/rev/121549dd2e34

Note You need to log in before you can comment on or make changes to this bug.