Generate and connect a Pepper identifier for Chrome OS

Generate and connect a Pepper identifier for Chrome OS

This change wires up GetDeviceID and passes in the BrowserContext into
PepperMessageFilter rather than the extracted ResourceContext.

The pepper drm identifier is a value provided for use by flash. It may
be reset or disabled by the user and cannot function in OffTheRecord
embodiments.

In Guest mode, the identifier is never generated.  When in incognito,
the ppapi plugin will be informed of the the profile state and not make
the call. Later, this will be enforced in the message filter as well.

Note, the preference is currently defaulted to true and does not have a
UI connection.  The UI is being wired up as part of crbug.com/125899.
At which point, we can make the setting syncable and chose the preferred
default.

This change bounces the GetDeviceID call through the renderer back to
the browser so that we have the path context to read the file from.
This same approach can be used to allow OTR checking and preference
reading if that is preferable to a file.  If we need to do anything on
the UI thread, we should do everything there (OTR+Pref).

Since the ID is regenerated at every pref init/toggle, we can move the
value around without impacting users in the future.

Please let me know if I've totally botched the wiring for this.  I'm not
sure the best way to fully test it.

TEST=built and tested on x86-alex target:
- ID is generated for normal sign-in in /home/chronos/user and is not generated for Guest.
- Very lightweight ppapi test was run too.
  (Still need pointers on better flash api wiring testing to make sure it is seeing right string)
- Pepper flash still worked normally
Also built full x86-alex system image:
- booted it
- checked the file existence in both modes
- checked pepflash and talk video for normal functionality
TRYBOT=http://build.chromium.org/p/tryserver.chromium/builders/cros_x86/builds/353
BUG=chromium-os:30378
Change-Id: Ibfbc484918d94147ad4fdc522a6415c71731068b

R=brettw,sky,piman,viettrungluu


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

[battre, 2012-05-03 09:57:42]

You guys are probably more qualified to do the review than me, so I am just
posting some comments and one error I found.

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_message_filter.cc (left):

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Nit: You accidentally deleted this header

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:62: const char
kDRMIdentifierFile[] = "Pepper DRM ID.0";
nit: insert space because the following line is not mirrored?

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:64: const int
kDRMIdentifierSize = 64;
so why is this 64?

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:75:
browser_context->GetResourceContext(): NULL),
nit: +2 spaces at beginning of line
+1 space infront of :

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:667: *id = "";
return;

[drewry, 2012-05-03 13:57:18]

Thanks!

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_message_filter.cc (left):

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/05/03 09:57:43, battre wrote:
> Nit: You accidentally deleted this header

Done.

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:62: const char
kDRMIdentifierFile[] = "Pepper DRM ID.0";
On 2012/05/03 09:57:43, battre wrote:
> nit: insert space because the following line is not mirrored?

I swapped the ordering to avoid confusion

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:64: const int
kDRMIdentifierSize = 64;
On 2012/05/03 09:57:43, battre wrote:
> so why is this 64?

I commented slightly better and uses a constant expression: 256/8*2

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:75:
browser_context->GetResourceContext(): NULL),
On 2012/05/03 09:57:43, battre wrote:
> nit: +2 spaces at beginning of line
> +1 space infront of :

Done.

https://chromiumcodereview.appspot.com/10342013/diff/1/content/browser/render...
content/browser/renderer_host/pepper_message_filter.cc:667: *id = "";
On 2012/05/03 09:57:43, battre wrote:
> return;

Done.

[brettw, 2012-05-03 20:34:20]

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
File chrome/browser/chromeos/preferences.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
chrome/browser/chromeos/preferences.cc:577: system::drm_settings::ToggleDrm(
This should fit on one line.

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
File chrome/browser/chromeos/system/drm_settings.h (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
chrome/browser/chromeos/system/drm_settings.h:11: namespace drm_settings {
I wouldn't use this extra namespace. Base namespaces off dirs not files, not
dirs, and this is only one function.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:93: browser_path_("") {
Don't use "" in here, that just makes it do extra work over the default
initializer.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:107: } else if
(message.type() == PpapiHostMsg_PPBFlash_GetDeviceID::ID) {
You need to update this for your message.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:651: *id = "";
id->clear(); is probably better.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/renderer/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/renderer/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:262: virtual void
GetDeviceID(std::string* id) OVERRIDE;
I don't understand why we have two of these functions in this file.

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1147: *id = "";
id->clear()

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1149: PP_Var id_var =
enter.functions()->GetFlashAPI()->GetDeviceID(instance);
This leaks the string. You should probably change the message to just pass a
var, which will handle all the string stuff for you.

See OnHostMsgReadClipboardData which returns a var.

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1151: if (id_str) {
No {} here.

https://chromiumcodereview.appspot.com/10342013/diff/4024/webkit/plugins/ppap...
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/webkit/plugins/ppap...
webkit/plugins/ppapi/mock_plugin_delegate.cc:444: return "";
return std::string()

[Will Drewry, 2012-05-03 21:58:48]

Pushing up the new changes.  Testing in progress (full x86-alex + loading
libppapi_tests.so via test_case.html seems to be exercising the right pieces).

Thanks!

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
File chrome/browser/chromeos/preferences.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
chrome/browser/chromeos/preferences.cc:577: system::drm_settings::ToggleDrm(
On 2012/05/03 20:34:21, brettw wrote:
> This should fit on one line.

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
File chrome/browser/chromeos/system/drm_settings.h (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/chrome/browser/chro...
chrome/browser/chromeos/system/drm_settings.h:11: namespace drm_settings {
On 2012/05/03 20:34:21, brettw wrote:
> I wouldn't use this extra namespace. Base namespaces off dirs not files, not
> dirs, and this is only one function.

WFM. I modeled this off of a neighboring file and didn't roll back.

Thanks!

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:93: browser_path_("") {
On 2012/05/03 20:34:21, brettw wrote:
> Don't use "" in here, that just makes it do extra work over the default
> initializer.

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:93: browser_path_("") {
On 2012/05/03 20:34:21, brettw wrote:
> Don't use "" in here, that just makes it do extra work over the default
> initializer.

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:107: } else if
(message.type() == PpapiHostMsg_PPBFlash_GetDeviceID::ID) {
On 2012/05/03 20:34:21, brettw wrote:
> You need to update this for your message.

Done.

Awesomely, the IO thread hooks in ReadFile caught this!

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/browser/ren...
content/browser/renderer_host/pepper_message_filter.cc:651: *id = "";
On 2012/05/03 20:34:21, brettw wrote:
> id->clear(); is probably better.

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/renderer/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/content/renderer/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.h:262: virtual void
GetDeviceID(std::string* id) OVERRIDE;
On 2012/05/03 20:34:21, brettw wrote:
> I don't understand why we have two of these functions in this file.

Done.

Me either.

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1147: *id = "";
On 2012/05/03 20:34:21, brettw wrote:
> id->clear()

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1149: PP_Var id_var =
enter.functions()->GetFlashAPI()->GetDeviceID(instance);
On 2012/05/03 20:34:21, brettw wrote:
> This leaks the string. You should probably change the message to just pass a
> var, which will handle all the string stuff for you.
> 
> See OnHostMsgReadClipboardData which returns a var.

Done - I think :) Initial tests all look good, but I'm not sure if I missed any
spots that need attention still.

https://chromiumcodereview.appspot.com/10342013/diff/4024/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:1151: if (id_str) {
On 2012/05/03 20:34:21, brettw wrote:
> No {} here.

Done.

https://chromiumcodereview.appspot.com/10342013/diff/4024/webkit/plugins/ppap...
File webkit/plugins/ppapi/mock_plugin_delegate.cc (right):

https://chromiumcodereview.appspot.com/10342013/diff/4024/webkit/plugins/ppap...
webkit/plugins/ppapi/mock_plugin_delegate.cc:444: return "";
On 2012/05/03 20:34:21, brettw wrote:
> return std::string()

Done.

[brettw, 2012-05-03 22:02:26]

lgtm

[Will Drewry, 2012-05-03 22:43:18]

On 2012/05/03 22:02:26, brettw wrote:
> lgtm

Thanks!

Retested with a full x86-alex build.
Ran libppapi_tests.so in a normal session and a guest session
Removed/tweaked the normal session file and checked the logs to see that the
behavior was expected (--v=1).

The ppapi test doesn't check the contents, but any failure along the path should
return undefined and not a string. So I think we're good.

Should I wait for other OWNERS before trying to bounce this into the CQ?

[commit-bot: I haz the power, 2012-05-03 22:53:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/drewry@google.com/10342013/10001

[commit-bot: I haz the power, 2012-05-04 00:39:20]

Change committed as 135255

[jam, 2012-06-01 22:21:05]

http://codereview.chromium.org/10342013/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/10342013/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/pepper_message_filter.cc:65: //  
chrome/browser/chromeos/system/drm_settings.cc
this is a layering violation. this code, in content, is basically reaching out
to chrome profile code through the file system. it needs to go through the
Content API instead.

[jam, 2012-06-01 22:32:39]

http://codereview.chromium.org/10342013/diff/10001/content/browser/renderer_h...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/10342013/diff/10001/content/browser/renderer_h...
content/browser/renderer_host/pepper_message_filter.cc:65: //  
chrome/browser/chromeos/system/drm_settings.cc
On 2012/06/01 22:21:06, John Abd-El-Malek wrote:
> this is a layering violation. this code, in content, is basically reaching out
> to chrome profile code through the file system. it needs to go through the
> Content API instead.

and just to be clear, there should be a getter, most likely on BrowserContext,
that gets the unique id. I thought I saw a change that had that before.

[brettw, 2012-06-01 22:41:39]

Let's add a getter for this on BrowserContext. Since I don't think that's
threadsafe, let's remove the thread override for this message and call it on the
IO thread. That function should just take a callback.

Then we can implement that asynchronously on the Chrome FILE thread to read the
file and execute the callback.

[jam, 2012-06-01 22:45:50]

On 2012/06/01 22:41:39, brettw wrote:
> Let's add a getter for this on BrowserContext. Since I don't think that's
> threadsafe, let's remove the thread override for this message and call it on
the
> IO thread. That function should just take a callback.

BrowserContext is only to be used on the UI thread, so you'll still want to keep
the thread override, but switch to UI. the callback that you give to it can use
currying to hold the reply message.

> 
> Then we can implement that asynchronously on the Chrome FILE thread to read
the
> file and execute the callback.

[jam, 2012-06-18 15:04:57]

ping? Will: will you be able to get to this soon?

[drewry, 2012-06-18 15:11:34]

On 2012/06/18 15:04:57, John Abd-El-Malek wrote:
> ping? Will: will you be able to get to this soon?

Yep, but I'm not sure if I'll make branch or not yet.

Now, some continuation of my cluelessness questions :)

If I put a getter on the BrowserContext and run this on the UI context, should I
just have it store the device id in a pref value? I can do the generation in
settings on the file thread and have it post back to UI for storage.  On the
message filter side, it'll come down to getting that value either from a getter
broker on the BrowserContext (or are prefs fair game on the UI thread?).

Thanks!

[brettw, 2012-06-18 17:12:25]

On Mon, Jun 18, 2012 at 8:11 AM,  <drewry@google.com> wrote:
> On 2012/06/18 15:04:57, John Abd-El-Malek wrote:
>>
>> ping? Will: will you be able to get to this soon?
>
>
> Yep, but I'm not sure if I'll make branch or not yet.
>
> Now, some continuation of my cluelessness questions :)
>
> If I put a getter on the BrowserContext and run this on the UI context,
> should I
> just have it store the device id in a pref value? I can do the generation in
> settings on the file thread and have it post back to UI for storage.  On the
> message filter side, it'll come down to getting that value either from a
> getter
> broker on the BrowserContext (or are prefs fair game on the UI thread?).

I'm not sure I totally follow what you want to do with prefs. Are you
thinking of reading the file into a pref and storing it there? When
would you read it out of the file? That seems more complicated than
just reading the file every time. We just need to be sure to do it
from Chrome code.

Brett

[drewry, 2012-06-18 18:42:25]

On Mon, Jun 18, 2012 at 12:12 PM, Brett Wilson <brettw@chromium.org> wrote:
> On Mon, Jun 18, 2012 at 8:11 AM,  <drewry@google.com> wrote:
>> On 2012/06/18 15:04:57, John Abd-El-Malek wrote:
>>>
>>> ping? Will: will you be able to get to this soon?
>>
>>
>> Yep, but I'm not sure if I'll make branch or not yet.
>>
>> Now, some continuation of my cluelessness questions :)
>>
>> If I put a getter on the BrowserContext and run this on the UI context,
>> should I
>> just have it store the device id in a pref value? I can do the generation in
>> settings on the file thread and have it post back to UI for storage.  On the
>> message filter side, it'll come down to getting that value either from a
>> getter
>> broker on the BrowserContext (or are prefs fair game on the UI thread?).
>
> I'm not sure I totally follow what you want to do with prefs. Are you
> thinking of reading the file into a pref and storing it there?

I was thinking of skipping file creation (which Chrome does now) and
just poke it into prefs instead of writing out the file.

> When would you read it out of the file? That seems more complicated than
> just reading the file every time. We just need to be sure to do it
> from Chrome code.

Instead we'd create the file (in Chrome) during sign-in, then read it
when pulling it into the browser context, I guess?

[brettw, 2012-06-18 19:01:09]

On Mon, Jun 18, 2012 at 11:42 AM, Will Drewry™ <drewry@google.com> wrote:
> On Mon, Jun 18, 2012 at 12:12 PM, Brett Wilson <brettw@chromium.org> wrote:
>> On Mon, Jun 18, 2012 at 8:11 AM,  <drewry@google.com> wrote:
>>> On 2012/06/18 15:04:57, John Abd-El-Malek wrote:
>>>>
>>>> ping? Will: will you be able to get to this soon?
>>>
>>>
>>> Yep, but I'm not sure if I'll make branch or not yet.
>>>
>>> Now, some continuation of my cluelessness questions :)
>>>
>>> If I put a getter on the BrowserContext and run this on the UI context,
>>> should I
>>> just have it store the device id in a pref value? I can do the generation
in
>>> settings on the file thread and have it post back to UI for storage.  On
the
>>> message filter side, it'll come down to getting that value either from a
>>> getter
>>> broker on the BrowserContext (or are prefs fair game on the UI thread?).
>>
>> I'm not sure I totally follow what you want to do with prefs. Are you
>> thinking of reading the file into a pref and storing it there?
>
> I was thinking of skipping file creation (which Chrome does now) and
> just poke it into prefs instead of writing out the file.

That certainly works from an architectural standpoint. I don't know
what the initial profile creation code looks like though.

Brett