Remove the refcount from ScriptBadgeController.

Remove the refcount from ScriptBadgeController, which
required me to switch it from wrapping a ScriptExecutor to observing one.

Also write a simple test to demonstrate how to test ScriptBadgeController. This was the original point of the change. The associated refactoring turned out to be more useful, but was ultimately unnecessary for the test.

BUG=133139, 136301
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145994

[Jeffrey Yasskin, 2012-07-09 19:22:05]

If you see a better strategy for this, feel free to suggest it.

[not at google - send to devlin, 2012-07-10 01:28:15]

lgtm only nits

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_badge_controller.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_badge_controller.h:62: // Implementation of
ScriptExecutor::Observer.
nit: nittiest nit ever: make the implementation-comment terminology match the
one above :)

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_badge_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_badge_controller_unittest.cc:98: .Build();
so cool

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.cc (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.cc:76: page_id, error));
nit: surround multi-line condition bodies in {}

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.cc:100: content::WebContents*
web_contents)
nit: 1 line

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:56: // and destruction. 
*script_executor must outlive *this.
nit: 1 space not 2

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:57: explicit
Observer(ScriptExecutor* script_executor);
That lifetime thing is neat... I wonder why we don't do that more often in
Chromium.

There's also base/scoped_observer but it's awkward to use.

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:65: ScriptExecutor&
script_executor_;
Being passed into here as a pointer, why not hold on to it as a pointer?

(More a curiosity)

[Jeffrey Yasskin, 2012-07-10 18:34:58]

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_badge_controller.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_badge_controller.h:62: // Implementation of
ScriptExecutor::Observer.
On 2012/07/10 01:28:15, kalman wrote:
> nit: nittiest nit ever: make the implementation-comment terminology match the
> one above :)

How in the world did I miss that? Done.

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.cc (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.cc:76: page_id, error));
On 2012/07/10 01:28:15, kalman wrote:
> nit: surround multi-line condition bodies in {}

Oops, done.

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.cc:100: content::WebContents*
web_contents)
On 2012/07/10 01:28:15, kalman wrote:
> nit: 1 line

Oops again. Oh wait, this one was like that when I got here! ;-) Fixed.

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:56: // and destruction. 
*script_executor must outlive *this.
On 2012/07/10 01:28:15, kalman wrote:
> nit: 1 space not 2

Done. We're so inconsistent on this, but of course consistency within files
wins.

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:57: explicit
Observer(ScriptExecutor* script_executor);
On 2012/07/10 01:28:15, kalman wrote:
> That lifetime thing is neat... I wonder why we don't do that more often in
> Chromium.

Several observers do. I was imitating WebContentsObserver to some extent, which
goes even farther and observes a WebContentsImplDestroyed() event to allow the
WebContents to be destroyed before the observer. I decided that wasn't worth
doing in this case.

> There's also base/scoped_observer but it's awkward to use.

Oh hey, I was looking for that class. Let's see if it shortens things...

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:65: ScriptExecutor&
script_executor_;
On 2012/07/10 01:28:15, kalman wrote:
> Being passed into here as a pointer, why not hold on to it as a pointer?
> 
> (More a curiosity)

It can't be NULL, and it's constant through the life of the Observer, so that
naturally fits storing a reference as the data member. However, the constructor
shouldn't take it as a reference for two reasons: 1) it's non-const and 2) it
gets captured, which is better indicated by pointers than references. Shifting
the type in the constructor is a little uncommon, but makes the call and uses
both as clear as the types can make them.

[Jeffrey Yasskin, 2012-07-10 19:14:44]

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/11001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:57: explicit
Observer(ScriptExecutor* script_executor);
On 2012/07/10 18:34:58, Jeffrey Yasskin wrote:
> On 2012/07/10 01:28:15, kalman wrote:
> > There's also base/scoped_observer but it's awkward to use.
> 
> Oh hey, I was looking for that class. Let's see if it shortens things...

By 3 lines, which I think isn't worth it. If ScopedObserver had a destructor
taking a single source to observe, it would save 4 more lines (by making an
inline constructor sensible), which might tip the balance.

https://chromiumcodereview.appspot.com/10754004/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/script_executor.h (right):

https://chromiumcodereview.appspot.com/10754004/diff/21001/chrome/browser/ext...
chrome/browser/extensions/script_executor.h:59: ~Observer() {}
This explicit destructor is required by chromium's style checks.

[commit-bot: I haz the power, 2012-07-10 19:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10754004/21002

[commit-bot: I haz the power, 2012-07-10 20:55:40]

Try job failure for 10754004-21002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 20:56:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10754004/21002

[commit-bot: I haz the power, 2012-07-10 20:58:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10754004/21002

[commit-bot: I haz the power, 2012-07-10 22:19:25]

Try job failure for 10754004-21002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 22:27:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10754004/21002

[commit-bot: I haz the power, 2012-07-10 23:47:04]

Change committed as 145994