signin: Abort previous 'channelConnected' handling on retry

signin: Abort previous 'channelConnected' handling on retry

- Bind previous channel's 'channelConnected' to a no-op when
  retry a new connection;
- Fix wrong member var declaration in gaia_auth classes;
- Re-enabled the flaky AddContentScriptsWithNewWindowAPI test;

BUG=497977
Committed: https://crrev.com/7121c3290e50fef87ef886f2c1defad2a9bedd0b
Cr-Commit-Position: refs/heads/master@{#333872}

[xiyuan, 2015-06-10 03:14:38]

xiyuan@chromium.org changed reviewers:
+ dzhioev@chromium.org

[xiyuan, 2015-06-10 05:40:18]

Please hold off review.

https://codereview.chromium.org/1179433002/diff/1/chrome/browser/resources/ga...
File chrome/browser/resources/gaia_auth/main.js (right):

https://codereview.chromium.org/1179433002/diff/1/chrome/browser/resources/ga...
chrome/browser/resources/gaia_auth/main.js:228:
supportChannel.registerMessage('channelConnected', function() {});
This does not work. Old message handler is called somehow. :(

[xiyuan, 2015-06-10 18:01:04]

Okay. Now it's ready for review.

PS1 reveals a bug that was there a long time ago. I was not reading JS doc
carefully and declared Object ({}, []) member in .prototype and it turns out to
be shared with all instances. That makes the rebinding 'channelConencted'
message not working. I have moved those members into the constructors to make
them per-instance now.

PTAL. Thanks.

[dzhioev (left Google), 2015-06-10 20:04:19]

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (left):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:37: bridges_: {},
Can you replace this with 'bridges_: null' instead of removing?

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:151: passwordStore_: {},
ditto

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/channel.js (left):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/channel.js:28: internalRequestCallbacks_: {},
Same here

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/channel.js (right):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/channel.js:16: this.nextInternalRequestId_ =
0;
This initialized with primitive type, so you can left it where it was before.

[xiyuan, 2015-06-10 20:21:12]

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (left):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:37: bridges_: {},
On 2015/06/10 20:04:18, dzhioev wrote:
> Can you replace this with 'bridges_: null' instead of removing?

It is not deleted. It is moved to BackgroundBridgeManager constructor so that we
have a per-instance copy of it.

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:151: passwordStore_: {},
On 2015/06/10 20:04:18, dzhioev wrote:
> ditto

Same here. It is moved to BackgroundBridge constructor.

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/channel.js (right):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/channel.js:16: this.nextInternalRequestId_ =
0;
On 2015/06/10 20:04:19, dzhioev wrote:
> This initialized with primitive type, so you can left it where it was before.

Yes, we can leave it where it was. But do you think it would be easier to read
to keep all member vars together?

[dzhioev (left Google), 2015-06-10 21:20:46]

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (left):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:37: bridges_: {},
On 2015/06/10 20:21:12, xiyuan wrote:
> On 2015/06/10 20:04:18, dzhioev wrote:
> > Can you replace this with 'bridges_: null' instead of removing?
> 
> It is not deleted. It is moved to BackgroundBridgeManager constructor so that
we
> have a per-instance copy of it.

I understand this. That's a matter of opinion, but I find it handy when all
properties that are used by "class" are "declared" in the beginning of
prototype. Some of them could be initialized in a constructor. Something like
that:

function FooBar() {
  this.foo_ = new Foo();
}

FooBar.prototype = {
  /**
   * Comment about |foo_|.
   */
  foo_: null,

  /**
   * Comment about |bar_|.
  bar_: 1
};

[xiyuan, 2015-06-10 21:32:45]

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/gaia_auth/background.js (left):

https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
chrome/browser/resources/gaia_auth/background.js:37: bridges_: {},
On 2015/06/10 21:20:46, dzhioev wrote:
> On 2015/06/10 20:21:12, xiyuan wrote:
> > On 2015/06/10 20:04:18, dzhioev wrote:
> > > Can you replace this with 'bridges_: null' instead of removing?
> > 
> > It is not deleted. It is moved to BackgroundBridgeManager constructor so
that
> we
> > have a per-instance copy of it.
> 
> I understand this. That's a matter of opinion, but I find it handy when all
> properties that are used by "class" are "declared" in the beginning of
> prototype. Some of them could be initialized in a constructor. Something like
> that:
> 
> function FooBar() {
>   this.foo_ = new Foo();
> }
> 
> FooBar.prototype = {
>   /**
>    * Comment about |foo_|.
>    */
>   foo_: null,
> 
>   /**
>    * Comment about |bar_|.
>   bar_: 1
> };

I see what you mean now. Done here and other places.

[dzhioev (left Google), 2015-06-10 22:14:17]

On 2015/06/10 21:32:45, xiyuan wrote:
>
https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/gaia_auth/background.js (left):
> 
>
https://codereview.chromium.org/1179433002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/gaia_auth/background.js:37: bridges_: {},
> On 2015/06/10 21:20:46, dzhioev wrote:
> > On 2015/06/10 20:21:12, xiyuan wrote:
> > > On 2015/06/10 20:04:18, dzhioev wrote:
> > > > Can you replace this with 'bridges_: null' instead of removing?
> > > 
> > > It is not deleted. It is moved to BackgroundBridgeManager constructor so
> that
> > we
> > > have a per-instance copy of it.
> > 
> > I understand this. That's a matter of opinion, but I find it handy when all
> > properties that are used by "class" are "declared" in the beginning of
> > prototype. Some of them could be initialized in a constructor. Something
like
> > that:
> > 
> > function FooBar() {
> >   this.foo_ = new Foo();
> > }
> > 
> > FooBar.prototype = {
> >   /**
> >    * Comment about |foo_|.
> >    */
> >   foo_: null,
> > 
> >   /**
> >    * Comment about |bar_|.
> >   bar_: 1
> > };
> 
> I see what you mean now. Done here and other places.

LGTM

[xiyuan, 2015-06-11 00:56:44]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2015-06-11 00:57:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179433002/60001

[commit-bot: I haz the power, 2015-06-11 01:44:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-11 01:44:55]

Patchset 4 (id:??) landed as
https://crrev.com/7121c3290e50fef87ef886f2c1defad2a9bedd0b
Cr-Commit-Position: refs/heads/master@{#333872}