Refactor tabs API handlers out of AccessibilityApiHandler.

Refactor tabs API handlers out of AccessibilityApiHandler.

Committed: https://crrev.com/31524c7c1b23c965aad9c340b57fc0175a89f4b3
Cr-Commit-Position: refs/heads/master@{#296013}

[David Tseng, 2014-09-16 18:17:09]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[David Tseng, 2014-09-18 01:55:10]

dtseng@chromium.org changed reviewers:
+ plundblad@chromium.org
- dmazzoni@chromium.org

[Peter Lundblad, 2014-09-18 07:47:59]

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js
(right):

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:7:
* spoken feedback for events that happen in the "Chrome of Chrome".
nit: remove 'spoken', we give three kinds of feedback;)

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:24:
* brailing.
nit: s/brailing/brailling/

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:25:
* @param {Object} earcons The earcons object to use for playing
s/Object/AbstractEarcons/

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:32:
this.msg = cvox.ChromeVox.msgs.getMsg.bind(cvox.ChromeVox.msgs);
Why aren't these private?

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:37:
}
I am not convinced that this idiom is worth the effort because it is
inconsistent
with most of the code base. Why not be explicit and put .bind(this) in the
assignments below?  Or is there a specific reason for doing it this way
that I am missing?
(Alternatively create a small utility function to do this if  we want to
start using it in many places.)

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:47:
onCreated: function(tab) {
jsdoc?  Same all over.

[David Tseng, 2014-09-18 22:48:34]

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js
(right):

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:7:
* spoken feedback for events that happen in the "Chrome of Chrome".
On 2014/09/18 07:47:58, Peter Lundblad wrote:
> nit: remove 'spoken', we give three kinds of feedback;)

Done.

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:24:
* brailing.
On 2014/09/18 07:47:59, Peter Lundblad wrote:
> nit: s/brailing/brailling/

Done.

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:25:
* @param {Object} earcons The earcons object to use for playing
On 2014/09/18 07:47:58, Peter Lundblad wrote:
> s/Object/AbstractEarcons/

Done.

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:32:
this.msg = cvox.ChromeVox.msgs.getMsg.bind(cvox.ChromeVox.msgs);
On 2014/09/18 07:47:59, Peter Lundblad wrote:
> Why aren't these private?

Done.

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:37:
}
On 2014/09/18 07:47:59, Peter Lundblad wrote:
> I am not convinced that this idiom is worth the effort because it is
> inconsistent
> with most of the code base. Why not be explicit and put .bind(this) in the
> assignments below?  Or is there a specific reason for doing it this way
> that I am missing?
> (Alternatively create a small utility function to do this if  we want to
> start using it in many places.)

A few reasons:
- I've found it's very easy to forget binding.
- it's impossible to remove the listener (i.e. var f = function() {}. f !==
f.bind(this)).
- each time you want to add a listener, you have to bind(this) again.

I can convert back if you feel strongly, but I think doing this is a good thing.
Happy to also move this to somewhere more common/util something.

https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:47:
onCreated: function(tab) {
On 2014/09/18 07:47:59, Peter Lundblad wrote:
> jsdoc?  Same all over.

Done.

[Peter Lundblad, 2014-09-19 07:47:42]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:37:
> }
> On 2014/09/18 07:47:59, Peter Lundblad wrote:
> > I am not convinced that this idiom is worth the effort because it is
> > inconsistent
> > with most of the code base. Why not be explicit and put .bind(this) in
> the
> > assignments below?  Or is there a specific reason for doing it this
> way
> > that I am missing?
> > (Alternatively create a small utility function to do this if  we want
> to
> > start using it in many places.)
> 
> A few reasons:
> - I've found it's very easy to forget binding.

Agred, but then we need to use this idiom very consistently. Otherwise, we'll
just
start assuming that we don't need to bind in places where you need to.
My issue is that it is not clear from the call site whether you need to
bind or not.

> - it's impossible to remove the listener (i.e. var f = function() {}. f
> !== f.bind(this)).
> - each time you want to add a listener, you have to bind(this) again.

Those could be addressed by storing the bound method under a different
name, but they are of course valid points.

> I can convert back if you feel strongly, but I think doing this is a
> good thing. Happy to also move this to somewhere more common/util
> something.

I think doing *something* like this is a good idea.  How would you feel about
creating new members such as boundX for a member X to make it explicit?
Then creating a utility that can be used wherever this is useful.

Thanks,
//Peter

>
https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:47:
> onCreated: function(tab) {
> On 2014/09/18 07:47:59, Peter Lundblad wrote:
> > jsdoc?  Same all over.
> 
> Done.
> 
> https://codereview.chromium.org/572373002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-09-19 19:04:17]

On Fri, Sep 19, 2014 at 12:47 AM, <plundblad@chromium.org> wrote:

> dtseng@chromium.org writes:
> >
> >
>
https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
> >
>
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:37:
> > }
> > On 2014/09/18 07:47:59, Peter Lundblad wrote:
> > > I am not convinced that this idiom is worth the effort because it is
> > > inconsistent
> > > with most of the code base. Why not be explicit and put .bind(this) in
> > the
> > > assignments below?  Or is there a specific reason for doing it this
> > way
> > > that I am missing?
> > > (Alternatively create a small utility function to do this if  we want
> > to
> > > start using it in many places.)
> >
> > A few reasons:
> > - I've found it's very easy to forget binding.
>
> Agred, but then we need to use this idiom very consistently. Otherwise,
> we'll just
> start assuming that we don't need to bind in places where you need to.
>

What makes you think that? It's pretty clear at least on the class level.


> My issue is that it is not clear from the call site whether you need to
> bind or not.
>
>

> > - it's impossible to remove the listener (i.e. var f = function() {}. f
> > !== f.bind(this)).
> > - each time you want to add a listener, you have to bind(this) again.
>
> Those could be addressed by storing the bound method under a different
> name, but they are of course valid points.
>
> > I can convert back if you feel strongly, but I think doing this is a
> > good thing. Happy to also move this to somewhere more common/util
> > something.
>
> I think doing *something* like this is a good idea.  How would you feel
> about
> creating new members such as boundX for a member X to make it explicit?
> Then creating a utility that can be used wherever this is useful.
>
> Nope; that won't work since dynamically assigning methods will result in
numerous compiler errors complaining about undefined functions.. .

The only alternative that I think will solve your point about ambiguity of
whether a func is bound or not is perhaps to introduce "bound" into the
class name. For example, BoundedTabsApiHandler or something like that.

Anyway, converted back in favor of making progress on this cl.

Thanks,
> //Peter
>
> >
>
https://codereview.chromium.org/572373002/diff/1/chrome/browser/resources/chr...
> >
>
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:47:
> > onCreated: function(tab) {
> > On 2014/09/18 07:47:59, Peter Lundblad wrote:
> > > jsdoc?  Same all over.
> >
> > Done.
> >
> > https://codereview.chromium.org/572373002/
>
> --
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2014-09-22 16:47:41]

lgtm

lgtm

Let's talk about a way of making the binding part less annoying overall
separately.

[David Tseng, 2014-09-22 17:40:20]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-09-22 17:41:00]

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

[commit-bot: I haz the power, 2014-09-22 18:18:23]

Committed patchset #3 (id:40001) as baa44b0f59ad35ac9ebc6cc695bfd60eb6b430a5

[commit-bot: I haz the power, 2014-09-22 18:19:01]

Patchset 3 (id:??) landed as
https://crrev.com/31524c7c1b23c965aad9c340b57fc0175a89f4b3
Cr-Commit-Position: refs/heads/master@{#296013}