Last Comment Bug 750872 - Add yet more GCLI tests, this time for developer toolbar
: Add yet more GCLI tests, this time for developer toolbar
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 15
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on:
Blocks: 745773
  Show dependency treegraph
 
Reported: 2012-05-01 13:35 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-05-16 13:26 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Upload 1 (7.28 KB, patch)
2012-05-01 13:58 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
rcampbell: review+
Details | Diff | Review
Upload 2 (5.96 KB, patch)
2012-05-11 11:33 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-01 13:35:05 PDT

    
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-01 13:58:58 PDT
Created attachment 620054 [details] [diff] [review]
Upload 1

I took this out of the mega-patch earlier to make it less mega.
Please could you take a particularly close look at oneTimeObserve().
Thanks.
Comment 2 Rob Campbell [:rc] (:robcee) 2012-05-03 03:37:37 PDT
Comment on attachment 620054 [details] [diff] [review]
Upload 1

+
+  // Christmas tree!

! :)

+function oneTimeObserve(name, callback) {
+  var func = function() {
+    Services.obs.removeObserver(func, name, false);

Don't need the third argument on removeObserver (it takes 2).

+    console.log('OTO end ' + name);

probably unneeded now?

+    callback();

I might use callback.call() just to be a pedant, but it's your call. (see what I did there?)
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-03 06:51:31 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> Comment on attachment 620054 [details] [diff] [review]

> +function oneTimeObserve(name, callback) {
> +  var func = function() {
> +    Services.obs.removeObserver(func, name, false);
> 
> Don't need the third argument on removeObserver (it takes 2).

Ok, thanks. I smell some confusion with addEventListener.

> +    console.log('OTO end ' + name);
> 
> probably unneeded now?

Yes.

> +    callback();
> 
> I might use callback.call() just to be a pedant, but it's your call. (see
> what I did there?)

Is there any technical difference between the 2? - I'm happy to change it, but adding chars seems like it needs a justification!

Thanks,

Joe.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-11 11:33:27 PDT
Created attachment 623231 [details] [diff] [review]
Upload 2

Fixes Rob's comments
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-11 13:33:46 PDT
Relevant tries:
https://tbpl.mozilla.org/?tree=Try&rev=77b47c8bbc35
https://tbpl.mozilla.org/?tree=Try&rev=1ce36097dc58
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-15 05:09:53 PDT
Try before land:
https://tbpl.mozilla.org/?tree=Try&rev=eba127ab7962

Fx-Team:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=0f08ab4dfcac
Comment 7 Rob Campbell [:rc] (:robcee) 2012-05-16 13:26:21 PDT
https://hg.mozilla.org/mozilla-central/rev/0f08ab4dfcac

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