Convert resources to take an instance key instead of an Instance*.

Convert resources to take an instance key instead of an Instance*.

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

[dmichael (off chromium), 2012-02-21 23:38:02]

I like the approach...  a few comments so far.

Also...  would it make sense to split this in to two CLs?

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.cc
File ppapi/cpp/instance.cc (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.c...
ppapi/cpp/instance.cc:145: // TODO(brettw) assert we're on the main thread.
May be worth adding a brief comment why the stuff below is only safe on the main
thread?

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.h
File ppapi/cpp/instance.h (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.h...
ppapi/cpp/instance.h:490: /// Static version of AddPerInstanceObject that takes
an InstanceHandle. As with
nit: >80 char

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.h...
ppapi/cpp/instance.h:496: /// {PENDING: summarize Remove method here}
nit: jond made that // instead of /// to keep it out of the generated doxygen

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance.h...
ppapi/cpp/instance.h:505: /// Static version of AddPerInstanceObject that takes
an InstanceHandle. As with
>80 char

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
File ppapi/cpp/instance_handle.h (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
ppapi/cpp/instance_handle.h:14: /// An instance handle identifies an instance in
a constructor for a resource. Its
>80 char

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
ppapi/cpp/instance_handle.h:17: /// pp::Instance objects' lifetime is managed by
the system on the main thread
nit: "A pp::Instance object's"?

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
ppapi/cpp/instance_handle.h:32: /// just pass "this" to resource constructors in
your instance objects).
This paragraph is good for our use but may not add much for a plugin developer.

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/private/fl...
File ppapi/cpp/private/flash.h (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/private/fl...
ppapi/cpp/private/flash.h:33: static void SetInstanceAlwaysOnTop(const
InstanceHandle& instance, bool on_top);
>80 characters

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/url_loader.cc
File ppapi/cpp/url_loader.cc (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/url_loader...
ppapi/cpp/url_loader.cc:36: }*/
So I guess we can delete this now...

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/example/exampl...
File ppapi/example/example.cc (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/example/exampl...
ppapi/example/example.cc:39: pp::SimpleThread* thread_;
I'm guessing the changes in this file weren't meant for this CL?

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/utility/thread...
File ppapi/utility/threading/simple_thread.h (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/utility/thread...
ppapi/utility/threading/simple_thread.h:31: SimpleThread(const InstanceHandle&
instance);
explicit

[brettw, 2012-02-22 20:24:36]

PTAL

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
File ppapi/cpp/instance_handle.h (right):

https://chromiumcodereview.appspot.com/9381010/diff/3001/ppapi/cpp/instance_h...
ppapi/cpp/instance_handle.h:32: /// just pass "this" to resource constructors in
your instance objects).
I added a "background" label to make it more clear regular developers can skip
this.

[dmichael (off chromium), 2012-02-22 21:38:44]

lgtm

This time I looked at the other half of the CL and saw a bunch of other places
where we were missing "explicit" :-(. Not your fault, but probably worth fixing
while you're there.

Otherwise just nits.

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/selection_dev.h
File ppapi/cpp/dev/selection_dev.h (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/selection_dev...
ppapi/cpp/dev/selection_dev.h:39: Selection_Dev(const InstanceHandle& instance);
explicit

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/widget_client...
File ppapi/cpp/dev/widget_client_dev.cc (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/widget_client...
ppapi/cpp/dev/widget_client_dev.cc:77: &scrollbar_interface);
nit: indentation

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/widget_client...
File ppapi/cpp/dev/widget_client_dev.h (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/widget_client...
ppapi/cpp/dev/widget_client_dev.h:22: WidgetClient_Dev(const InstanceHandle&
instance);
explicit

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/zoom_dev.h
File ppapi/cpp/dev/zoom_dev.h (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/dev/zoom_dev.h#ne...
ppapi/cpp/dev/zoom_dev.h:40: Zoom_Dev(const InstanceHandle& instance);
explicit

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/file_io.h
File ppapi/cpp/file_io.h (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/file_io.h#newcode34
ppapi/cpp/file_io.h:34: FileIO(const InstanceHandle& instance);
explicit

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/fullscreen.h
File ppapi/cpp/fullscreen.h (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/fullscreen.h#newc...
ppapi/cpp/fullscreen.h:25: Fullscreen(const InstanceHandle& instance);
explicit

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/instance.cc
File ppapi/cpp/instance.cc (right):

http://codereview.chromium.org/9381010/diff/12001/ppapi/cpp/instance.cc#newco...
ppapi/cpp/instance.cc:146: // and may be deleted from the main thread)>
nit: ">" -> "."