[Presentation API, Android] Implement routes observers

[Presentation API, Android] Implement routes observers

BUG=517100
Committed: https://crrev.com/849d49aa4ecad9246680207c2fdc5791c07afcbc
Cr-Commit-Position: refs/heads/master@{#344231}

[whywhat, 2015-08-18 22:26:19]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2015-08-18 22:26:19]

PTaL

[whywhat, 2015-08-18 22:26:24]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 22:26:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291203003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291203003/1

[mlamouri (slow - plz ping), 2015-08-18 22:39:37]

lgtm with comments applied.

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.cc:177: if
(!routes_observers_.HasObserver(observer))
nit: I don't think this check is necessary.

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.cc:284: for (auto it =
active_routes_.begin(); it != active_routes_.end(); ++it)
nit: const auto& it

also, can you do: for (const auto& it : active_routes_) ?

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.cc:285:
routes.push_back(*it->second);
nit:
routes.push_back(*(it->second));

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.h:117: MediaRoutes
active_routes_;
FWIW, I'm not sure if it is really helping to have a HashMap. Every time you
modify the map (O(1)), you end up creating a vector from that map (O(n)). So,
basically, it boils down to a O(n) operation. I wonder if it wouldn't be better
to simply have a vector for active_routes_ so at least you save creating a new
vector on the stack when there are changes.

Obviously, I do not have the big picture so maybe this map might come handy with
future CLs.

[commit-bot: I haz the power, 2015-08-18 23:31:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 23:31:43]

Dry run: This issue passed the CQ dry run.

[whywhat, 2015-08-19 00:26:43]

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.cc:177: if
(!routes_observers_.HasObserver(observer))
On 2015/08/18 at 22:39:37, Mounir Lamouri wrote:
> nit: I don't think this check is necessary.

I copied it from the desktop implementation :) Not sure if that's there on
purpose or not.

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.cc:284: for (auto it =
active_routes_.begin(); it != active_routes_.end(); ++it)
On 2015/08/18 at 22:39:37, Mounir Lamouri wrote:
> nit: const auto& it

A const reference to iterator can't be ++'ed

> 
> also, can you do: for (const auto& it : active_routes_) ?

Perhaps, I haven't got used to this form yet :(

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1291203003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_android.h:117: MediaRoutes
active_routes_;
On 2015/08/18 at 22:39:37, Mounir Lamouri wrote:
> FWIW, I'm not sure if it is really helping to have a HashMap. Every time you
modify the map (O(1)), you end up creating a vector from that map (O(n)). So,
basically, it boils down to a O(n) operation. I wonder if it wouldn't be better
to simply have a vector for active_routes_ so at least you save creating a new
vector on the stack when there are changes.
> 
> Obviously, I do not have the big picture so maybe this map might come handy
with future CLs.

Agreed. I don't think a map is needed in the future - we need a map from route
id to session id which already exists on the Java side. (unless there's a hope
that MediaRoutesObserver and MediaRouter Mojo interfaces are improved and we
don't have to pass all the routes in a vector each time :) Perhaps it's easier
to pass this array from JS of the component extension.

[whywhat, 2015-08-19 13:54:26]

Rebase

[whywhat, 2015-08-19 13:55:14]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 13:55:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291203003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291203003/40001

[commit-bot: I haz the power, 2015-08-19 15:07:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-19 15:07:29]

Dry run: This issue passed the CQ dry run.

[whywhat, 2015-08-19 15:25:57]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-08-19 15:25:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1291203003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-19 15:26:35]

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

[commit-bot: I haz the power, 2015-08-19 15:32:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-19 15:33:39]

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