Bundle Hangouts Services extension with Chrome

Bundle Hangouts Services extension with Chrome

BUG=291271
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230426

[vrk (LEFT CHROMIUM), 2013-10-23 00:31:09]

Patrick, since you are in Munich-time and I'm in PST, please chat with Joi for
background. Thanks!

[Patrick Dubroy, 2013-10-23 12:07:51]

I don't think I'm the best person to review this. Maybe arv?

[Jói, 2013-10-23 12:17:56]

Manifest, logo and JavaScript code LGTM.

I chatted with Patrick who did not feel he was the right reviewer for this. Will
find another reviewer from chrome/browser/resources/OWNERS to approve.

Cheers,
Jói

[Nikita (slow), 2013-10-23 12:24:22]

lgtm

[commit-bot: I haz the power, 2013-10-23 12:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/35873003/1

[commit-bot: I haz the power, 2013-10-23 14:49:52]

Change committed as 230426

[arv (Not doing code reviews), 2013-10-23 16:32:26]

LGTM

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
File chrome/browser/resources/hangouts_services/thunk.js (right):

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
chrome/browser/resources/hangouts_services/thunk.js:14: if (method ==
'chooseDesktopMedia') {
This would have been cleaner as a switch (yes you can use switch with strings in
js)

[meacer, 2013-10-23 17:09:16]

Just a comment from a drive by review.

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
File chrome/browser/resources/hangouts_services/manifest.json (right):

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
chrome/browser/resources/hangouts_services/manifest.json:9: "matches":
["*://*.google.com/hangouts*"]
Should this be /hangouts/* rather than /hangouts* ?

Also, can we restrict this to https? In which cases is http used?

[vrk (LEFT CHROMIUM), 2013-10-23 19:10:01]

Thanks all. I'll write a follow-up CL to address the post-commit comments.

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
File chrome/browser/resources/hangouts_services/manifest.json (right):

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
chrome/browser/resources/hangouts_services/manifest.json:9: "matches":
["*://*.google.com/hangouts*"]
On 2013/10/23 17:09:17, Mustafa Emre Acer wrote:
> Should this be /hangouts/* rather than /hangouts* ?
Yes, I think so. I'll double-check to make sure.

> Also, can we restrict this to https? In which cases is http used?
We run local test instances of Hangouts over http. If it's important to restrict
this to https, we could do it -- it would just make testing local changes a bit
inconvenient from the Hangouts-side. Let  me know.

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
File chrome/browser/resources/hangouts_services/thunk.js (right):

https://codereview.chromium.org/35873003/diff/1/chrome/browser/resources/hang...
chrome/browser/resources/hangouts_services/thunk.js:14: if (method ==
'chooseDesktopMedia') {
On 2013/10/23 16:32:27, arv wrote:
> This would have been cleaner as a switch (yes you can use switch with strings
in
> js)

Thanks, will do in a follow-up CL.