Change the DidChangeView update to take a new ViewChanged resource.

Change the DidChangeView update to take a new ViewChanged resource.

This will allow us to be more flexible about adding data to view changed updates in the future. For now, I've incorporated fullscreen and tab foreground state into the view state.

BUG=
TEST=

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

[dmichael (off chromium), 2011-12-16 04:14:31]

I've only looked at the example so far...

http://codereview.chromium.org/8951014/diff/2001/ppapi/example/example.cc
File ppapi/example/example.cc (right):

http://codereview.chromium.org/8951014/diff/2001/ppapi/example/example.cc#new...
ppapi/example/example.cc:266: if (view.GetViewportRect().size() !=
current_view_.GetViewportRect().size())
That's not bad, though it doesn't handle the initial call yet, does it?

You might get away with just storing the 'current_size_'. Then the initial case
would be comparing against a default-constructed Size, and it should work fine.

I also don't see where you set current_view_. Did I miss it?

It would also be helpful to see an example of handling changes in visibility.
That might be more compelling for having a "DidChangeVisibility", maybe even
directly on PPB_View instead of in a separate interface.

[brettw, 2011-12-19 22:53:40]

New snap up.

http://codereview.chromium.org/8951014/diff/2001/ppapi/example/example.cc
File ppapi/example/example.cc (right):

http://codereview.chromium.org/8951014/diff/2001/ppapi/example/example.cc#new...
ppapi/example/example.cc:266: if (view.GetViewportRect().size() !=
current_view_.GetViewportRect().size())
It does since the current_view_ is intitialized to a null resource and the
default sizes are all 0.

[dmichael (off chromium), 2011-12-20 19:01:34]

Whew. Looks good overall, but there are some vestiges of the PPB_ViewChanged
interface to get rid of.

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:391: delete this;
I'm a little uneasy about "delete this" here, especially with this being a
virtual function. Not really for a technical reason, but because it's another
level of indirection and it seems like it could be a maintenance problem if the
caller doesn't know that OnProcessLaunched can result in the pointer being
invalidated.

Maybe it would make sense to make OnProcessLaunched return a bool indicating
success, and then on failure, you can delete it here:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/brows...

(But I'm new to this part of the code, so you can ignore me if you're sure this
is not going to be a problem).

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.h (right):

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.h:37: // responsibility to avoid
handling the renderer.
"avoid handling the renderer"? Is it supposed to say "hanging" or something?

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:153:
reply_msg->set_reply_error();
Now that I think about it, did you mean for this stuff to be in another CL?
Seems like it could be independent.

http://codereview.chromium.org/8951014/diff/6001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/api/ppb_view.idl#newco...
ppapi/api/ppb_view.idl:19: * <code>PPB_View</code> objects so you will receive
new view information via
This comment is out of date now that there's no PPB_ViewChanged

http://codereview.chromium.org/8951014/diff/6001/ppapi/cpp/view.cc
File ppapi/cpp/view.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/cpp/view.cc#newcode37
ppapi/cpp/view.cc:37: return true;
I guess I would've expected "false" for this and *IsVisible in this case (not
that it matters much in practice)

http://codereview.chromium.org/8951014/diff/6001/ppapi/cpp/view.h
File ppapi/cpp/view.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/cpp/view.h#newcode37
ppapi/cpp/view.h:37: /// @return The rectangle of the instance.
or a default rectangle if the View is null?

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/browser_ppp_instance.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppp_instance.cc:112: //
previous state it sent out, so can do the "changed" part itself.
Is this comment still valid with no PPB_ViewChanged??

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppp_instance.cc:114:
GetBrowserInterface(PPB_VIEW_INTERFACE));
Could you just use PPBViewInterface that you added to browser_globals? It goes
through GetBrowserInterfaceSafe, which is probably preferred (though it appears
the only difference is a DebugPrintf).

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_instance_data.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_instance_data.h:43: bool
sent_view_changed_;
I think this is a vestige of the now-defunct PPB_ViewChanged interface.

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/ppp_instance_combined.cc
(right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/ppp_instance_combined.cc:22:
instance_1_1_.HandleDocumentLoad = instance_if->HandleDocumentLoad;
initialized_ = true; ?

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/ppp_instance_combined.cc:26:
instance_1_1_ = *instance_if;
initialized_ = true; ?

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/upcall_server.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/upcall_server.cc:47: };
Not a big deal, but you have any idea why these upcall* things are in the CL but
apparently unchanged? Could you take them out?

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:1081: pp_instance(),
view.pp_resource());
Wait, I thought you needed to pass the resource as well as the rectangles so
that PPP_InstanceCombined can handle 1.0 as well as 1.1? Does this compile?

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/plugin_dispatche...
File ppapi/proxy/plugin_dispatcher.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.cc:238: // prroxies the 1.1 interface, and then
does fallback to 1.0 inside the
prroxies->proxies

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:30: #include "ppapi/c/dev/pp_video_dev.h"
nit: include order

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppb_instance_pro...
File ppapi/proxy/ppb_instance_proxy.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppb_instance_pro...
ppapi/proxy/ppb_instance_proxy.h:50: virtual PP_Resource GetView(PP_Instance
instance) OVERRIDE;
Should this still be here? You got rid of GetView from PPB_Instance. If you use
this internally, ignore this comment...

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppp_instance_pro...
File ppapi/proxy/ppp_instance_proxy.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/proxy/ppp_instance_pro...
ppapi/proxy/ppp_instance_proxy.cc:237: ViewData old_data = data->view;
It looks like old_data is unused.

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
File ppapi/shared_impl/ppb_view_shared.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
ppapi/shared_impl/ppb_view_shared.cc:17: bool ViewData::Equals(const ViewData&
other) const {
Do you still use this?

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
File ppapi/shared_impl/ppb_view_shared.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
ppapi/shared_impl/ppb_view_shared.h:45: ~PPB_View_Shared();
nit: virtual

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/resource.h
File ppapi/shared_impl/resource.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/resource.h...
ppapi/shared_impl/resource.h:53: F(PPB_ViewChanged_API) \
leftover from previous API?

http://codereview.chromium.org/8951014/diff/6001/ppapi/thunk/ppb_instance_api.h
File ppapi/thunk/ppb_instance_api.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/thunk/ppb_instance_api...
ppapi/thunk/ppb_instance_api.h:33: virtual PP_Resource GetView(PP_Instance
instance) = 0;
Pretty sure now you're not using this one. Please delete.

http://codereview.chromium.org/8951014/diff/6001/ppapi/thunk/thunk.h
File ppapi/thunk/thunk.h (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/thunk/thunk.h#newcode46
ppapi/thunk/thunk.h:46: //typedef PPB_Instance PPB_Instance_2_0;
Please remove

http://codereview.chromium.org/8951014/diff/6001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8951014/diff/6001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1036: sent_did_change_view_ =
true;
Maybe sent_initial_did_change_view_ would be a better name?

http://codereview.chromium.org/8951014/diff/6001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1712:
view_data_))->GetReference();
Don't think you need this one anymore

[dmichael (off chromium), 2011-12-20 19:41:57]

One more thing: Test(s) plz?

[brettw, 2011-12-20 20:49:46]

Since all the tests run when we send DidChangeView, the fact that it's getting
sent at all is tested very well. I'm still thinking about how best to test some
of the numbers in the message.

[dmichael (off chromium), 2011-12-21 02:24:31]

On 2011/12/20 20:49:46, brettw wrote:
> Since all the tests run when we send DidChangeView, the fact that it's getting
> sent at all is tested very well. I'm still thinking about how best to test
some
> of the numbers in the message.
But they're also still using the "old" DidChangeView, so the coverage of
PPB_View is limited to what the C++ interface uses to route to the old version
(maybe that's enough?). In any case, the tests should probably be changed to use
the new DidChangeView, with maybe 1 purpose-built test for verifying the
deprecated DidChangeView?

Also...  I realized the SDK examples should be changed. Because this is
backwards compatible, you/I/somebody can do that in a separate CL.

[brettw, 2011-12-21 22:06:54]

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/8951014/diff/6001/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:391: delete this;
Sorry this shouldn't have been in this CL (got two mixed up). Sadly, "delete
this" is how this component works.

[brettw, 2011-12-21 23:55:28]

I think I got all the comments except for the tests.

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/upcall_server.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/upcall_server.cc:47: };
Yes, I had to run the generator and it switched the line endings to Windows :(

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:1081: pp_instance(),
view.pp_resource());
This is on the browser side. Everything about NaCl assumes 1.1 on the browser
side, and it's remapped on the plugin side if necessary.

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
File ppapi/shared_impl/ppb_view_shared.cc (right):

http://codereview.chromium.org/8951014/diff/6001/ppapi/shared_impl/ppb_view_s...
ppapi/shared_impl/ppb_view_shared.cc:17: bool ViewData::Equals(const ViewData&
other) const {
Yes, in PluginInstance to avoid sending duplicate messages.

[dmichael (off chromium), 2011-12-22 17:40:04]

lgtm

http://codereview.chromium.org/8951014/diff/16001/ppapi/cpp/view.cc
File ppapi/cpp/view.cc (right):

http://codereview.chromium.org/8951014/diff/16001/ppapi/cpp/view.cc#newcode49
ppapi/cpp/view.cc:49: return true;
missed one

http://codereview.chromium.org/8951014/diff/16001/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_globals.cc (right):

http://codereview.chromium.org/8951014/diff/16001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/plugin_globals.cc:115: const void*
instance1_1 = GetPluginInterface(
This is so that we don't spit out Debug, right? May be worth a comment to avoid
somebody coming along and changing it.

[darin (slow to review), 2012-01-31 21:56:47]

http://codereview.chromium.org/8951014/diff/32001/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

http://codereview.chromium.org/8951014/diff/32001/ppapi/api/ppb_view.idl#newc...
ppapi/api/ppb_view.idl:17: * You can get a View object with the
<code>PPB_Instance.GetView()</code>
nit: there is no GetView() method

http://codereview.chromium.org/8951014/diff/32001/ppapi/api/ppb_view.idl#newc...
ppapi/api/ppb_view.idl:18: * function. Additionally, all
</code>PPB_ViewChanged</code> objects are also
nit: PPB_ViewChanged doesn't exist in this rev of the CL

http://codereview.chromium.org/8951014/diff/32001/ppapi/api/ppp_instance.idl
File ppapi/api/ppp_instance.idl (right):

http://codereview.chromium.org/8951014/diff/32001/ppapi/api/ppp_instance.idl#...
ppapi/api/ppp_instance.idl:173: [in] PP_Resource view_resource);
nit: view_resource -> view