Aura: Show dialog box asking user's permisssion for screen sharing for gtalk

Aura: Show dialog box asking user's permisssion for screen sharing for gtalk
video plugin.

BUG=111282
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127638

[Avi (use Gerrit), 2012-03-12 14:45:30]

https://chromiumcodereview.appspot.com/9683003/diff/2001/content/browser/rend...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/2001/content/browser/rend...
content/browser/renderer_host/pepper_message_filter.cc:661: string16
message(ASCIIToUTF16("Share your screen?"));
Do you have a better internationalization story than this?

https://chromiumcodereview.appspot.com/9683003/diff/2001/content/public/brows...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9683003/diff/2001/content/public/brows...
content/public/browser/content_browser_client.h:413: virtual void
ShowYesNoBox(const string16& title,
I'm not that familiar with ppapi; how much UI does it need to do? This seems a
little limiting in the general case.

[jam, 2012-03-12 16:09:54]

there are a few issues here
-a chrome-specific api added to ContentBrowserClient, which we avoid, i.e. as
you can see from the other callbacks they tend to be more of the form of
AllowX()
-pepper message filter (in content) has stuff about Talk, which is a chrome
feature

I don't see PpapiHostMsg_PPBTalk_GetPermission in cs, is it not landed yet? if
this is a private pepper interface, it should be in chrome so that the ipc is
sent from chrome, not content. you can see
chrome/renderer/chrome_ppb_pdf_impl.cc as an example for what we did for the pdf
plugin, which is also a chrome feature

[jam, 2012-03-12 16:10:54]

btw more info about the content/chrome split is here:
http://www.chromium.org/developers/content-module

[Josh Horwich, 2012-03-12 18:42:27]

On 2012/03/12 16:09:54, John Abd-El-Malek wrote:
> there are a few issues here
> -a chrome-specific api added to ContentBrowserClient, which we avoid, i.e. as
> you can see from the other callbacks they tend to be more of the form of
> AllowX()
> -pepper message filter (in content) has stuff about Talk, which is a chrome
> feature
> 
> I don't see PpapiHostMsg_PPBTalk_GetPermission in cs, is it not landed yet?

Correct, this has not landed yet. brettw is adding it (under private) here:
https://chromiumcodereview.appspot.com/9666020/

> if
> this is a private pepper interface, it should be in chrome so that the ipc is
> sent from chrome, not content. you can see
> chrome/renderer/chrome_ppb_pdf_impl.cc as an example for what we did for the
pdf
> plugin, which is also a chrome feature

[varunjain, 2012-03-14 18:17:55]

Hi John, Brett,
I have changed this CL according to what we talk about. PTAL. Also upload
another CL which is a complete version of Brett's skeleton CL for ppapi changes.

[Josh Horwich, 2012-03-14 18:41:45]

A drive-by question on localization

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:189: string16
message = ASCIIToUTF16("Share your screen?");
Should we be using l10n_util::GetStringUTF16 for these (along with resources in
chrome/app/generated_resources.grd, as per
http://www.chromium.org/developers/design-documents/ui-localization) ?

[varunjain, 2012-03-14 18:49:08]

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:189: string16
message = ASCIIToUTF16("Share your screen?");
On 2012/03/14 18:41:45, Josh Horwich wrote:
> Should we be using l10n_util::GetStringUTF16 for these (along with resources
in
> chrome/app/generated_resources.grd, as per
> http://www.chromium.org/developers/design-documents/ui-localization) ?

the talk code already has localized versions of this string. I am thinking if we
want to relocalize here or when we do the complete screen casting dialog, send
localized strings from the plugin?

[sky, 2012-03-14 20:56:28]

I suspect you don't really need 5 people reviewing this. I'm removing myself
from the list of reviewers.

[Avi (use Gerrit), 2012-03-14 21:02:59]

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:189: string16
message = ASCIIToUTF16("Share your screen?");
I'm not familiar with the design of this area, but you can't just use English
here as I earlier noted and Josh reasserted. If you have an idea how to
reasonably handle this, go for it.

[varunjain, 2012-03-15 00:44:52]

On 2012/03/14 21:02:59, Avi wrote:
>
https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
> File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9683003/diff/11001/chrome/browser/rend...
> chrome/browser/renderer_host/chrome_render_view_host_observer.cc:189: string16
> message = ASCIIToUTF16("Share your screen?");
> I'm not familiar with the design of this area, but you can't just use English
> here as I earlier noted and Josh reasserted. If you have an idea how to
> reasonably handle this, go for it.

Hi guys... I talked to saintlou about this and decided to localize in chrome.
Changed the CL accordingly. PTAL.

[jam, 2012-03-15 02:05:26]

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:198: Send(new
ChromeViewMsg_GetTalkPermissionACK(
what you're doing here is basically a sync IPC, so just make it one for
simplicity (it'll simplify http://codereview.chromium.org/9700028/ a lot).

normally we don't allow sync IPCs to the browser UI thread, but that's only for
messages that are also handled on windows (with npapi plugins). in this case,
you don't have to worry about that, so make it a sync IPC. see the comment in
browser_message_filter.cc for more background

[Avi (use Gerrit), 2012-03-15 04:33:03]

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/app/generate...
chrome/app/generated_resources.grd:15368: +        System dialog
Yuck. Don't we usually use the name of the app for dialog titles? Look at some
of the dialog callers in the code.

[brettw, 2012-03-15 17:44:32]

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:198: Send(new
ChromeViewMsg_GetTalkPermissionACK(
I talked to John and we can keep this async. I think it is better async.

[varunjain, 2012-03-15 17:54:15]

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/app/generate...
chrome/app/generated_resources.grd:15368: +        System dialog
On 2012/03/15 04:33:03, Avi wrote:
> Yuck. Don't we usually use the name of the app for dialog titles? Look at some
> of the dialog callers in the code.

Done.

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:198: Send(new
ChromeViewMsg_GetTalkPermissionACK(
On 2012/03/15 02:05:26, John Abd-El-Malek wrote:
> what you're doing here is basically a sync IPC, so just make it one for
> simplicity (it'll simplify http://codereview.chromium.org/9700028/ a lot).
> 
> normally we don't allow sync IPCs to the browser UI thread, but that's only
for
> messages that are also handled on windows (with npapi plugins). in this case,
> you don't have to worry about that, so make it a sync IPC. see the comment in
> browser_message_filter.cc for more background

Done.

https://chromiumcodereview.appspot.com/9683003/diff/12004/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:198: Send(new
ChromeViewMsg_GetTalkPermissionACK(
On 2012/03/15 17:44:32, brettw wrote:
> I talked to John and we can keep this async. I think it is better async.

reverted to async

[Avi (use Gerrit), 2012-03-15 18:04:01]

https://chromiumcodereview.appspot.com/9683003/diff/15009/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9683003/diff/15009/chrome/app/generate...
chrome/app/generated_resources.grd:15368: +        GTalk Screen Sharing Dialog
The user sees that it's a dialog. You don't need to say "dialog" in the title.

[Ben Goodger (Google), 2012-03-15 18:05:01]

kan/kuscher should comment on the strings here

[Kuscher, 2012-03-15 18:28:06]

Let's use a slight adaptation of what we do on other platforms (adjusted for the
fact that we offer no option to share specific windows only)

https://chromiumcodereview.appspot.com/9683003/diff/15009/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9683003/diff/15009/chrome/app/generate...
chrome/app/generated_resources.grd:15365: +        Share your screen?
Again, slight adaptation of GTalk on other platforms:

Please confirm that you want to start presenting your screen in this meeting.

https://chromiumcodereview.appspot.com/9683003/diff/15009/chrome/app/generate...
chrome/app/generated_resources.grd:15368: +        GTalk Screen Sharing Dialog
Let's make this consistent with this GTalk dialog on other platforms, use:

Google Talk Screen Sharing Request

[varunjain, 2012-03-16 10:28:45]

@avi,kuscher: Changed the message strings according to comments.

@John,Brett:
I have made the changes according to what we discussed. The CL is still slightly
incomplete but I wanted to get early feedback, especially on how I am getting
the renderer id from the renderer. I have divided the CLs as follows:
* Basic PPAPI changes are in http://codereview.chromium.org/9700028/ 

* This CL adds on to the one above. I have implemented browser side
functionality and also functionality to get the renderer id from the renderer
first, then calling the browser to show the dialog.

[Avi (use Gerrit), 2012-03-16 12:57:46]

The dialog strings LGTM.

[jam, 2012-03-16 21:49:45]

http://codereview.chromium.org/9683003/diff/16006/chrome/browser/pepper_gtalk...
File chrome/browser/pepper_gtalk_message_filter.cc (right):

http://codereview.chromium.org/9683003/diff/16006/chrome/browser/pepper_gtalk...
chrome/browser/pepper_gtalk_message_filter.cc:52: // TODO(varunjain): use the
renderer_id to get current renderer.
why is this a todo?

to get the current TabContents, you can do

WebContents* tab = tab_util::GetWebContentsByID(renderer_id, render_view_id)

then Browser::GetBrowserForController(tab, NULL)

http://codereview.chromium.org/9683003/diff/16006/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/9683003/diff/16006/content/public/browser/cont...
content/public/browser/content_browser_client.h:111: virtual void
BrowserChildProcessHostCreated(
please add a comment like the rest of this interface. also mention that it's
called on the IO thread

http://codereview.chromium.org/9683003/diff/16006/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9683003/diff/16006/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1594: return
render_view_->GetRoutingID();
nit: just "return routing_id();"

http://codereview.chromium.org/9683003/diff/16006/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_delegate.h (right):

http://codereview.chromium.org/9683003/diff/16006/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_delegate.h:594: virtual int GetRendererId() = 0;
nit: add a comment.

this should also be called GetRenderViewID

[varunjain, 2012-03-19 09:05:28]

John, Brett,
as discussed earlier, I am making the dialog system modal for now. PTAL at this
and the ppapi changes CL (https://chromiumcodereview.appspot.com/9700028/)

On 2012/03/16 21:49:45, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9683003/diff/16006/chrome/browser/pepper_gtalk...
> File chrome/browser/pepper_gtalk_message_filter.cc (right):
> 
>
http://codereview.chromium.org/9683003/diff/16006/chrome/browser/pepper_gtalk...
> chrome/browser/pepper_gtalk_message_filter.cc:52: // TODO(varunjain): use the
> renderer_id to get current renderer.
> why is this a todo?
> 
> to get the current TabContents, you can do
> 
> WebContents* tab = tab_util::GetWebContentsByID(renderer_id, render_view_id)
> 
> then Browser::GetBrowserForController(tab, NULL)
> 
>
http://codereview.chromium.org/9683003/diff/16006/content/public/browser/cont...
> File content/public/browser/content_browser_client.h (right):
> 
>
http://codereview.chromium.org/9683003/diff/16006/content/public/browser/cont...
> content/public/browser/content_browser_client.h:111: virtual void
> BrowserChildProcessHostCreated(
> please add a comment like the rest of this interface. also mention that it's
> called on the IO thread
> 
>
http://codereview.chromium.org/9683003/diff/16006/content/renderer/pepper/pep...
> File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):
> 
>
http://codereview.chromium.org/9683003/diff/16006/content/renderer/pepper/pep...
> content/renderer/pepper/pepper_plugin_delegate_impl.cc:1594: return
> render_view_->GetRoutingID();
> nit: just "return routing_id();"
> 
>
http://codereview.chromium.org/9683003/diff/16006/webkit/plugins/ppapi/plugin...
> File webkit/plugins/ppapi/plugin_delegate.h (right):
> 
>
http://codereview.chromium.org/9683003/diff/16006/webkit/plugins/ppapi/plugin...
> webkit/plugins/ppapi/plugin_delegate.h:594: virtual int GetRendererId() = 0;
> nit: add a comment.
> 
> this should also be called GetRenderViewID

[jam, 2012-03-19 17:50:07]

lgtm

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
File chrome/browser/pepper_gtalk_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
chrome/browser/pepper_gtalk_message_filter.cc:7: #if defined(USE_AURA)
nit: usually we put the #includes in ifdefs at the end, all in one section

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
chrome/browser/pepper_gtalk_message_filter.cc:14: #include
"ipc/ipc_message_macros.h"
nit: no need to put this

[varunjain, 2012-03-19 18:05:13]

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
File chrome/browser/pepper_gtalk_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
chrome/browser/pepper_gtalk_message_filter.cc:7: #if defined(USE_AURA)
On 2012/03/19 17:50:07, John Abd-El-Malek wrote:
> nit: usually we put the #includes in ifdefs at the end, all in one section

Done.

https://chromiumcodereview.appspot.com/9683003/diff/22001/chrome/browser/pepp...
chrome/browser/pepper_gtalk_message_filter.cc:14: #include
"ipc/ipc_message_macros.h"
On 2012/03/19 17:50:07, John Abd-El-Malek wrote:
> nit: no need to put this

Done.

[commit-bot: I haz the power, 2012-03-19 23:25:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/9683003/26001

[commit-bot: I haz the power, 2012-03-20 00:14:58]

Try job failure for 9683003-26001 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[varunjain, 2012-03-20 00:17:45]

Talked to Brett and added the one file include to chrome/browser/DEPS

On 2012/03/20 00:14:58, I haz the power (commit-bot) wrote:
> Try job failure for 9683003-26001 (retry) on linux_rel for step "check_deps".
> It's a second try, previously, step "check_deps" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-20 00:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/9683003/26010

[commit-bot: I haz the power, 2012-03-20 01:18:22]

Try job failure for 9683003-26010 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Josh Horwich, 2012-03-20 01:19:09]

FYI

http://codereview.chromium.org/9683003/diff/26010/chrome/browser/DEPS
File chrome/browser/DEPS (right):

http://codereview.chromium.org/9683003/diff/26010/chrome/browser/DEPS#newcode16
chrome/browser/DEPS:16: "+ppapi/shared_impl/api_id.h"
Please add comma after string, that makes check_deps happy again (I tried that
locally)

[varunjain, 2012-03-20 02:48:34]

http://codereview.chromium.org/9683003/diff/26010/chrome/browser/DEPS
File chrome/browser/DEPS (right):

http://codereview.chromium.org/9683003/diff/26010/chrome/browser/DEPS#newcode16
chrome/browser/DEPS:16: "+ppapi/shared_impl/api_id.h"
On 2012/03/20 01:19:09, Josh Horwich wrote:
> Please add comma after string, that makes check_deps happy again (I tried that
> locally)

oh weird it somehow passed locally for me :(
Changed.

[commit-bot: I haz the power, 2012-03-20 02:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/9683003/32001

[commit-bot: I haz the power, 2012-03-20 03:54:45]

Change committed as 127638