Last Comment Bug 875297 - Move the contents of the initialize method into the constructor of the search-textbox binding
: Move the contents of the initialize method into the constructor of the search...
Status: RESOLVED FIXED
[good first bug][mentor=mconley][lang...
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 24
Assigned To: maofl
:
Mentors:
Depends on: 875042
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-23 06:48 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2013-06-04 17:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 875297 (9.21 KB, patch)
2013-06-03 11:48 PDT, maofl
mconley: review-
Details | Diff | Splinter Review
Patch for bug 875297 (replaces attachment 757529) (9.56 KB, patch)
2013-06-04 10:41 PDT, maofl
mconley: review+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2013-05-23 06:48:06 PDT
In bug 875042, we got rid of the setTimeout that was delaying the initialization of the search-toolbox. Gavin rightly pointed out that we could further simplify things if we just got rid of the initialize method and just dump all of its code directly into the constructor.

This is a super trivial piece of clean-up work, and an excellent first bug if somebody wants to get their feet wet in Firefox code.
Comment 1 maofl 2013-06-03 11:48:38 PDT
Created attachment 757529 [details] [diff] [review]
Patch for bug 875297

Put all code from the initialize method into the constructor, as suggested.
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2013-06-03 12:03:48 PDT
Comment on attachment 757529 [details] [diff] [review]
Patch for bug 875297

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

This is the right idea, but the indentation is a bit off. Take a look around the rest of the file - I think we're using 4 space indentation in there. Please conform to this.

Thanks!
Comment 3 maofl 2013-06-04 10:41:34 PDT
Created attachment 758035 [details] [diff] [review]
Patch for bug 875297 (replaces attachment 757529 [details] [diff] [review])

Now the identation should be correct (2 space identation).
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2013-06-04 11:59:24 PDT
Comment on attachment 758035 [details] [diff] [review]
Patch for bug 875297 (replaces attachment 757529 [details] [diff] [review])

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

Looks good to me! Thanks Markus!
Comment 5 Ryan VanderMeulen [:RyanVM] 2013-06-04 12:45:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/950ab2f55a1a
Comment 6 Ryan VanderMeulen [:RyanVM] 2013-06-04 17:51:09 PDT
https://hg.mozilla.org/mozilla-central/rev/950ab2f55a1a

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