Move HiDPI-related Pepper interfaces to stable

Move HiDPI-related Pepper interfaces to stable

This adds the APIs provided in PPB_View_Dev_0_1 and PP_Graphics2D_Dev_0_1 to
the public Pepper APIs for PPB_View and PPB_Graphics2D.

Includes:
- Change to IDL and generated C headers/shim
- Change to PPAPI to export the 1_1 interfaces
- C++ glue
- Example PPAPI plugin for using HiDPI Pepper APIs

BUG=144071
TEST=Existing (prebuilt) PDF plugin using 1.0 interfaces still works

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

[dmichael (off chromium), 2013-03-28 21:38:29]

This looks great to me, overall...

Would you be willing to tweak the NaCl SDK graphics 2d example
(src/native_client_sdk/src/examples/pi_generator/) in a separate CL after this
lands?

I'll re-ping chromium-dev to make sure everyone else has a chance to see the API
and register any complaints.

https://codereview.chromium.org/12989006/diff/2002/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/api/ppb_view.idl#new...
ppapi/api/ppb_view.idl:165: * GetDeviceScale returns the scale factor between
device pixels and DIPs
Would be nice to spell out "Device Independent Pixels" at least once.

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h
File ppapi/cpp/graphics_2d.h (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h#ne...
ppapi/cpp/graphics_2d.h:264: static bool SupportsScale();
Hmm, I don't think we usually have this kind of function in the C++ wrappers. It
also seems like it would be unnecessary...  if we're running on a version of
Chrome that doesn't support these, then the View should give us a device scale
of 1.0, and a scale-aware plugin will just work as if it's on a device with
scale 1.0.

Is there a use case for this function I'm not thinking of?

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h#ne...
ppapi/cpp/graphics_2d.h:287: float GetScale();
It would be good to mention somewhere that the default is always 1.0

https://codereview.chromium.org/12989006/diff/2002/ppapi/examples/scaling/sca...
File ppapi/examples/scaling/scaling.html (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/examples/scaling/sca...
ppapi/examples/scaling/scaling.html:42: Mouse clicks in the plugin will generate
a message in the JS console.<br>
That brings up an interesting point...  should we also update comments in
InputEvent? I'm not even sure if it uses DIPs or CSS pixels when zoomed...

I could also imagine adding a GetFloatPosition or something, though that would
make sense as a separate CL.

[Josh Horwich, 2013-03-29 20:54:23]

PTAL

Also, yes I will modify pi_generator in a follow-on CL - should be
straightforward.

Finally, in light of the thread on C++ glue, has there been a conclusion on if I
should be changing graphics_2d.cc and view_2d.cc to first try for the 1_1
interfaces then fallback to 1_0 for all the existing routines?
https://groups.google.com/a/chromium.org/forum/#!topic/pepper-dev/HrcIduc88ks

https://codereview.chromium.org/12989006/diff/2002/ppapi/api/ppb_view.idl
File ppapi/api/ppb_view.idl (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/api/ppb_view.idl#new...
ppapi/api/ppb_view.idl:165: * GetDeviceScale returns the scale factor between
device pixels and DIPs
On 2013/03/28 21:38:29, dmichael wrote:
> Would be nice to spell out "Device Independent Pixels" at least once.

Done.

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h
File ppapi/cpp/graphics_2d.h (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h#ne...
ppapi/cpp/graphics_2d.h:44: /// measured in device pixels.
I just noticed the use of "device pixels" here (and in size() docs) which is
actually incorrect, but not surprising seeing as this pre-dated HiDPI modes
(devicePixelRatio!=1) in Chrome.

It seems that the wording here should be a bit different, as a context without
SetScale applied will have these in DIPs (not device pixels) and one that has
SetScale applied will have them be "device pixels" only if the scale is actually
the inverse of the device scale (the recommended practice).

maybe just "measured in pixels." is technically accurate and covers all cases,
but might continue to confuse. What do you think?

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h#ne...
ppapi/cpp/graphics_2d.h:264: static bool SupportsScale();
On 2013/03/28 21:38:29, dmichael wrote:
> Hmm, I don't think we usually have this kind of function in the C++ wrappers.
It
> also seems like it would be unnecessary...  if we're running on a version of
> Chrome that doesn't support these, then the View should give us a device scale
> of 1.0, and a scale-aware plugin will just work as if it's on a device with
> scale 1.0.
> 
> Is there a use case for this function I'm not thinking of?

I originally was worried about cases where view did give a scale, but for
whatever reason the graphics2d scale interface wasn't available (since
technically one could implement a Pepper host that supports view1_1 but
graphics2d_1_0), so a plugin could decide to revert to "DIPs only" if it knew
that SetScale wasn't actually available.

However, this is probably overkill in practice (and they could check SetScale's
result too I suppose) so I'll remove it.

https://codereview.chromium.org/12989006/diff/2002/ppapi/cpp/graphics_2d.h#ne...
ppapi/cpp/graphics_2d.h:287: float GetScale();
On 2013/03/28 21:38:29, dmichael wrote:
> It would be good to mention somewhere that the default is always 1.0

Done.

https://codereview.chromium.org/12989006/diff/2002/ppapi/examples/scaling/sca...
File ppapi/examples/scaling/scaling.html (right):

https://codereview.chromium.org/12989006/diff/2002/ppapi/examples/scaling/sca...
ppapi/examples/scaling/scaling.html:42: Mouse clicks in the plugin will generate
a message in the JS console.<br>
On 2013/03/28 21:38:29, dmichael wrote:
> That brings up an interesting point...  should we also update comments in
> InputEvent? I'm not even sure if it uses DIPs or CSS pixels when zoomed...
> 
> I could also imagine adding a GetFloatPosition or something, though that would
> make sense as a separate CL.

InputEvents are in DIPs - hence the motivation for me to show the conversion
from DIPs to CSS pixels in ppapi/examples/scaling/scaling.cc:104.

Things like GetFloatPosition for located input events (and having it accurately
provide fractional position on HiDPI devices) probably requires WebKit changes,
or at least they did when these interfaces were added to dev. But I agree that
augmenting InputEvent would be worthwhile for apps/plugins that want
device-pixel-perfect locations, such as image editors.

[dmichael (off chromium), 2013-04-04 17:15:59]

https://chromiumcodereview.appspot.com/12989006/diff/2002/ppapi/cpp/graphics_...
File ppapi/cpp/graphics_2d.h (right):

https://chromiumcodereview.appspot.com/12989006/diff/2002/ppapi/cpp/graphics_...
ppapi/cpp/graphics_2d.h:44: /// measured in device pixels.
On 2013/03/29 20:54:23, Josh Horwich wrote:
> I just noticed the use of "device pixels" here (and in size() docs) which is
> actually incorrect, but not surprising seeing as this pre-dated HiDPI modes
> (devicePixelRatio!=1) in Chrome.
> 
> It seems that the wording here should be a bit different, as a context without
> SetScale applied will have these in DIPs (not device pixels) and one that has
> SetScale applied will have them be "device pixels" only if the scale is
actually
> the inverse of the device scale (the recommended practice).
> 
> maybe just "measured in pixels." is technically accurate and covers all cases,
> but might continue to confuse. What do you think?
Yes, that would probably be better. You might reference SetScale here for more
information.

https://chromiumcodereview.appspot.com/12989006/diff/51001/ppapi/cpp/graphics...
File ppapi/cpp/graphics_2d.cc (right):

https://chromiumcodereview.appspot.com/12989006/diff/51001/ppapi/cpp/graphics...
ppapi/cpp/graphics_2d.cc:45:
PassRefFromConstructor(get_interface<PPB_Graphics2D_1_0>()->Create(
Based on our other discussion here:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/pepper-dev...
I think we do need fallback code for all these methods.

[Josh Horwich, 2013-04-08 21:08:14]

PTAL

https://chromiumcodereview.appspot.com/12989006/diff/2002/ppapi/cpp/graphics_...
File ppapi/cpp/graphics_2d.h (right):

https://chromiumcodereview.appspot.com/12989006/diff/2002/ppapi/cpp/graphics_...
ppapi/cpp/graphics_2d.h:44: /// measured in device pixels.
On 2013/04/04 17:16:00, dmichael wrote:
> On 2013/03/29 20:54:23, Josh Horwich wrote:
> > I just noticed the use of "device pixels" here (and in size() docs) which is
> > actually incorrect, but not surprising seeing as this pre-dated HiDPI modes
> > (devicePixelRatio!=1) in Chrome.
> > 
> > It seems that the wording here should be a bit different, as a context
without
> > SetScale applied will have these in DIPs (not device pixels) and one that
has
> > SetScale applied will have them be "device pixels" only if the scale is
> actually
> > the inverse of the device scale (the recommended practice).
> > 
> > maybe just "measured in pixels." is technically accurate and covers all
cases,
> > but might continue to confuse. What do you think?
> Yes, that would probably be better. You might reference SetScale here for more
> information.

Done.

https://chromiumcodereview.appspot.com/12989006/diff/51001/ppapi/cpp/graphics...
File ppapi/cpp/graphics_2d.cc (right):

https://chromiumcodereview.appspot.com/12989006/diff/51001/ppapi/cpp/graphics...
ppapi/cpp/graphics_2d.cc:45:
PassRefFromConstructor(get_interface<PPB_Graphics2D_1_0>()->Create(
On 2013/04/04 17:16:00, dmichael wrote:
> Based on our other discussion here:
>
https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/pepper-d...
> I think we do need fallback code for all these methods.

Done.

I changed this glue (and view.cc) to try 1_1 interfaces first, then try 1_0
next, for all the existing methods that go through the interface.

Tested using my example plugin running with newest Chrome (which has the 1_1
interfaces available), and with older Chrome that only exposes 1_0.

I did not use any macros to reduce boilerplate however, please let me know if
this is ok.

[dmichael (off chromium), 2013-04-08 21:29:36]

https://codereview.chromium.org/12989006/diff/51001/ppapi/cpp/graphics_2d.cc
File ppapi/cpp/graphics_2d.cc (right):

https://codereview.chromium.org/12989006/diff/51001/ppapi/cpp/graphics_2d.cc#...
ppapi/cpp/graphics_2d.cc:45:
PassRefFromConstructor(get_interface<PPB_Graphics2D_1_0>()->Create(
On 2013/04/08 21:08:15, Josh Horwich wrote:
> On 2013/04/04 17:16:00, dmichael wrote:
> > Based on our other discussion here:
> >
>
https://groups.google.com/a/chromium.org/forum/?fromgroups=#%2521topic/pepper...
> > I think we do need fallback code for all these methods.
> 
> Done.
> 
> I changed this glue (and view.cc) to try 1_1 interfaces first, then try 1_0
> next, for all the existing methods that go through the interface.
> 
> Tested using my example plugin running with newest Chrome (which has the 1_1
> interfaces available), and with older Chrome that only exposes 1_0.
> 
> I did not use any macros to reduce boilerplate however, please let me know if
> this is ok.
I was kind of anti-macro in that thread, but got misinterpreted :-). If we come
up with some brilliant way to do it cleanly, we can fix it later. This is good
for now.

https://codereview.chromium.org/12989006/diff/67001/ppapi/cpp/view.cc
File ppapi/cpp/view.cc (right):

https://codereview.chromium.org/12989006/diff/67001/ppapi/cpp/view.cc#newcode62
ppapi/cpp/view.cc:62: if (!has_interface<PPB_View_1_1>()) {
^^^ I think you don't want the "!" here and 65

[Josh Horwich, 2013-04-08 21:37:49]

PTAL - ixed pp::View::IsPageVisible

https://chromiumcodereview.appspot.com/12989006/diff/67001/ppapi/cpp/view.cc
File ppapi/cpp/view.cc (right):

https://chromiumcodereview.appspot.com/12989006/diff/67001/ppapi/cpp/view.cc#...
ppapi/cpp/view.cc:62: if (!has_interface<PPB_View_1_1>()) {
On 2013/04/08 21:29:36, dmichael wrote:
> ^^^ I think you don't want the "!" here and 65

Done.  Argh, sorry about that.

[dmichael (off chromium), 2013-04-08 21:43:41]

lgtm, thanks!

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhorwich@chromium.org/12989006/74002

[commit-bot: I haz the power, 2013-04-08 22:40:27]

Presubmit check for 12989006-74002 failed and returned exit status 1.

INFO:root:Found 17 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 **
TODOs found in stable public PPAPI files:

***************
ppapi/cpp/graphics_2d.h
***************

Presubmit checks took 6.0s to calculate.

[Josh Horwich, 2013-04-09 00:13:50]

On 2013/04/08 22:40:27, I haz the power (commit-bot) wrote:
> Presubmit check for 12989006-74002 failed and returned exit status 1.
> 
> INFO:root:Found 17 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 **
> TODOs found in stable public PPAPI files:
> 
> ***************
> ppapi/cpp/graphics_2d.h
> ***************
> 
> Presubmit checks took 6.0s to calculate.

btw, I had to --bypass-hooks on upload because of this (pre-existing TODOs in
graphics_2d.h), and this kills the CQ. So instead I've run try jobs and will
dcommit.

mac_rel try failure is unrelated - other CLs tried with current LKGR 192822 are
failing identically, plus these are not related to PPAPI.

[Josh Horwich, 2013-04-09 15:40:54]

Committed patchset #7 manually as r193110.