Add interface to set the sub resource crash key from Flash

Add interface to set the sub resource crash key from Flash

BUG=N/A
TEST=N/A

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

[raymes1, 2013-04-16 23:31:54]

https://codereview.chromium.org/14311004/diff/1/ppapi/api/private/ppb_flash.idl
File ppapi/api/private/ppb_flash.idl (right):

https://codereview.chromium.org/14311004/diff/1/ppapi/api/private/ppb_flash.i...
ppapi/api/private/ppb_flash.idl:109: * Specifies the URL of the current flash
file.
maybe "swf" instead of "file"?

https://codereview.chromium.org/14311004/diff/1/ppapi/proxy/flash_resource.cc
File ppapi/proxy/flash_resource.cc (right):

https://codereview.chromium.org/14311004/diff/1/ppapi/proxy/flash_resource.cc...
ppapi/proxy/flash_resource.cc:92:
PluginGlobals::Get()->SetActiveURL(url_string_var->value());
Maybe change this one to the same code as the one below and remove the
now-unneeded plumbing? I don't think it's too hard, but if you really don't want
to, please add a TODO.

[Cris Neckar, 2013-04-17 19:20:06]

Well that got nice and big :)

We are going to want to have the trybots have their way with this one.

[Cris Neckar, 2013-04-17 19:20:30]

https://codereview.chromium.org/14311004/diff/1/ppapi/api/private/ppb_flash.idl
File ppapi/api/private/ppb_flash.idl (right):

https://codereview.chromium.org/14311004/diff/1/ppapi/api/private/ppb_flash.i...
ppapi/api/private/ppb_flash.idl:109: * Specifies the URL of the current flash
file.
On 2013/04/16 23:31:54, raymes1 wrote:
> maybe "swf" instead of "file"?

Done.

https://codereview.chromium.org/14311004/diff/1/ppapi/proxy/flash_resource.cc
File ppapi/proxy/flash_resource.cc (right):

https://codereview.chromium.org/14311004/diff/1/ppapi/proxy/flash_resource.cc...
ppapi/proxy/flash_resource.cc:92:
PluginGlobals::Get()->SetActiveURL(url_string_var->value());
On 2013/04/16 23:31:54, raymes1 wrote:
> Maybe change this one to the same code as the one below and remove the
> now-unneeded plumbing? I don't think it's too hard, but if you really don't
want
> to, please add a TODO.

Done.

[Cris Neckar, 2013-04-17 21:02:35]

+cpu@ for sanity check to make sure we aren't going to break all the things by
switching the way the base url is reported.

+brettw@ for owners lgtm on all the things we have to touch to accomplish this.

[raymes, 2013-04-18 17:20:55]

+rsesek to comment on whether the using the "url" key with the new scheme will
map correctly with the chunking in the old scheme.

Oh you did more than I expected here! :P In our conversation I was actually only
referring to the plumbing related to the pepper SetActiveURL i.e. the changes
you have in:
content/ppapi_plugin/ppapi_thread.h
content/ppapi_plugin/ppapi_thread.cc
ppapi/proxy/plugin_globals.h
ppapi/proxy/plugin_globals.cc
ppapi/proxy/plugin_main_nacl.cc
ppapi/proxy/plugin_proxy_delegate.h
ppapi/proxy/ppapi_proxy_test.h
ppapi/proxy/ppapi_proxy_test.cc

If you want to go ahead and change all these other instances, I think that is
good as well but I'm not convinced that trybot testing has good enough coverage
of this stuff. I think we would need to do some manual testing to ensure that
urls still end up in crash reports.

[Cris Neckar, 2013-04-18 17:30:26]

Ah gotcha. Let me submit a new patch with just those changes. Lets make sure
they will map correctly and if not we can leave in the calls to SetActiveURL()
in and still add the "url" crash key. then I can work on migrating the crash
backend over to accept the new version while we include both and eventually get
rid of the SetActiveURL() method.

[Robert Sesek, 2013-04-18 17:41:31]

This is nice! But as raymes suggests, you should definitely test this all
manually. The test coverage here is very sparse.

Unfortunately, this will break reporting on Linux since that doesn't have the
connection to the new crash logging system. I'm working on that this week,
though.

[Cris Neckar, 2013-04-18 19:39:36]

Cool, I'll save this patch for later and we can just do the basic change for
now. Let me know once it works for linux. 

rsesek it wasn't clear to me whether this would just work or not (seems like it
might). Will we need to change the crash backend to know the field name for url
crash key or does it overlap the old name?

if we need to change backend we can do this staged. Initially just add the crash
key, change the backend once it has shipped to stable, then remove the
SetActiveURL stuff after the backend is updated.

[raymes, 2013-04-18 20:05:27]

thanks! lgtm

[cpu_(ooo_6.6-7.5), 2013-04-18 20:07:56]

lgtm

[commit-bot: I haz the power, 2013-04-18 20:08:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/14311004/22002

[commit-bot: I haz the power, 2013-04-18 20:09:04]

Presubmit check for 14311004-22002 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ppapi/api/private/ppb_flash.idl
    ppapi/proxy/flash_resource.cc
    ppapi/c/private/ppb_flash.h

PPAPI IDL Diff detected: Run the generator.

***************
--- OLD ../c/private/ppb_flash.h
+++ NEW ../c/private/ppb_flash.h
@@ -5,3 +5,3 @@
 
-/* From private/ppb_flash.idl modified Thu Mar 28 10:30:53 2013. */
+/* From private/ppb_flash.idl modified Thu Apr 18 13:09:00 2013. */
 
@@ -122,3 +122,3 @@
   /**
-   * Specifies the URL of the current flash file.
+   * Specifies the URL of the current swf.
    */
Error C Header : Failed to generate range 0 : 20.
***************

Presubmit checks took 2.0s to calculate.

[Cris Neckar, 2013-04-18 20:13:59]

+noelallen, owners review for ppapi/api
+viettrungluu, owners review for ppapi/proxy and ppapi/

[noelallen1, 2013-04-18 20:16:20]

lgtm

[Cris Neckar, 2013-04-18 20:49:05]

+dmichael for owners review

[dmichael (off chromium), 2013-04-18 20:54:40]

Is it okay that we're not updating the version here? Does Flash need to know
whether this new enum value is valid, given that it might be pushed out to a
version of Chrome that doesn't support it?

[raymes, 2013-04-18 20:59:47]

I designed the function with this in mind - I don't think there is any
issue. A new version of flash with an old chrome will try to pass the
new enum value, which will not be caught by the switch statement in
flash_resource.cc, PP_FALSE will be returned and nothing will happen.

Old versions of flash with new chrome won't pass the value. So we
should be good?



On Thu, Apr 18, 2013 at 1:54 PM,  <dmichael@chromium.org> wrote:
> Is it okay that we're not updating the version here? Does Flash need to know
> whether this new enum value is valid, given that it might be pushed out to a
> version of Chrome that doesn't support it?
>
> https://codereview.chromium.org/14311004/

[dmichael (off chromium), 2013-04-18 21:43:16]

lgtm

[commit-bot: I haz the power, 2013-04-18 21:46:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/14311004/22002

[commit-bot: I haz the power, 2013-04-18 21:46:50]

Presubmit check for 14311004-22002 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **
PPAPI IDL Diff detected: Run the generator.

***************
--- OLD ../c/private/ppb_flash.h
+++ NEW ../c/private/ppb_flash.h
@@ -5,3 +5,3 @@
 
-/* From private/ppb_flash.idl modified Thu Mar 28 10:30:53 2013. */
+/* From private/ppb_flash.idl modified Thu Apr 18 14:46:47 2013. */
 
@@ -122,3 +122,3 @@
   /**
-   * Specifies the URL of the current flash file.
+   * Specifies the URL of the current swf.
    */
Error C Header : Failed to generate range 0 : 20.
***************

Presubmit checks took 2.2s to calculate.

[commit-bot: I haz the power, 2013-04-18 22:00:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/14311004/46001

[commit-bot: I haz the power, 2013-04-18 22:00:20]

Presubmit check for 14311004-46001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ppapi/PRESUBMIT.py

** Presubmit ERRORS **
PPAPI IDL Diff detected: Run the generator.

***************
--- OLD ../c/private/ppb_flash.h
+++ NEW ../c/private/ppb_flash.h
@@ -5,3 +5,3 @@
 
-/* From private/ppb_flash.idl modified Thu Mar 28 10:30:53 2013. */
+/* From private/ppb_flash.idl modified Thu Apr 18 15:00:17 2013. */
 
@@ -121,3 +121,2 @@
   PP_FLASHCRASHKEY_URL = 1,
-
   /**
Error C Header : Failed to generate range 0 : 20.
***************

Presubmit checks took 2.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-04-18 22:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/14311004/7014

[commit-bot: I haz the power, 2013-04-19 14:45:18]

Change committed as 195188