Implement HiDPI support in Pepper dev interface

Implement HiDPI support in Pepper dev interface

This patch requires WebKit patch https://bugs.webkit.org/show_bug.cgi?id=87874 (WebKit r121364)

Expose device_scale_factor and css_scale_factor to Pepper plugins via a dev
interface on View resource.
Allow Pepper plugins to create a 2D graphics context with a scale factor so the
plugins can render at device resolution rather than DIPs if they want.

BUG=114673
TEST=browser_tests --gtest_filter="PPAPITest.*" 
TEST=browser_tests --gtest_filter="OutOfProcessPPAPITest.*"
TEST=Build, run existing PPAPI plugin at 2x scale
TEST=Build, run test HiDPI aware plugin to render at device rez, at 1x, 2x scale

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

[Josh Horwich, 2012-06-15 00:22:38]

Brett,
  Please take a look. Also, let me know if you want me to spread it to several
reviewers since it is longish.
  This obsoletes the earlier patch I uploaded (issue 10444077) and implements
the HiDPI interfaces for PPAPI as private interfaces, as we discussed earlier
this week. As with that CL, this one has a trivial accompanying WebKit patch
used to expose the device scale (and the CSS scale), but this one will behave as
if those scales are 1.0 if the WebKit patch hasn't landed.
  In addition to normal review feedback, I'd like to know if you'd like me to
include my test plugin (I haven't cleaned up that code yet), and/or if you think
I shouldn't include the CSS scale stuff - I included the CSS scale this time so
I could test proper handling of page zoom as well as HiDPI.

[brettw, 2012-06-18 21:17:23]

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
File ppapi/api/private/ppb_graphics_2d_private.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_graphics_2d_private.idl:43: PP_Resource Create(
This is a bit confusing because normally if I see an argument on a graphics
display that takes a scale factor and I pass 2, I would expect it to be drawn
twice as large. Your scale value is actually the inverse scale.

Can we instead use the "real" scale and then maybe on the getter for the scale
on the view we can say if you want to paint at device pixels, pass
1.0/GetDeviceScale() as the scale here.

I think it would be simpler to have a scale setter rather than a different
constructor:
  PP_Bool SetScale(PP_Resource resource, float_t scale).
Maybe we should also have a
  float_t GetScale(PP_Resource resource);

We should probably document that the scale can't be 0 or it will fail (and make
sure that this actually happens).

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
File ppapi/api/private/ppb_view_private.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:12: interface PPB_View_Private {
Do you mind making this a dev interface instead of a private one? Sorry to make
you move/rename the file. But this is the type of thing we want to expose to web
content eventually, rather than something we want to keep internal to the
browser.

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:31: PP_Bool GetDeviceScale([in]
PP_Resource resource,
I usually prefer APIs that just return the value when possible. So can this be:
  float GetDeviceScale([in] PP_Resource resource);
and then say that it returns 0 for invalid resources?

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:48: PP_Bool GetCSSScale([in] PP_Resource
resource,
Ditto on the return value.

[Josh Horwich, 2012-06-19 02:01:04]

Please take a look.

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
File ppapi/api/private/ppb_graphics_2d_private.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_graphics_2d_private.idl:43: PP_Resource Create(
On 2012/06/18 21:17:24, brettw wrote:
> This is a bit confusing because normally if I see an argument on a graphics
> display that takes a scale factor and I pass 2, I would expect it to be drawn
> twice as large. Your scale value is actually the inverse scale.
> 
> Can we instead use the "real" scale and then maybe on the getter for the scale
> on the view we can say if you want to paint at device pixels, pass
> 1.0/GetDeviceScale() as the scale here.
> 
> I think it would be simpler to have a scale setter rather than a different
> constructor:
>   PP_Bool SetScale(PP_Resource resource, float_t scale).
> Maybe we should also have a
>   float_t GetScale(PP_Resource resource);
> 
> We should probably document that the scale can't be 0 or it will fail (and
make
> sure that this actually happens).

Done.

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
File ppapi/api/private/ppb_view_private.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:12: interface PPB_View_Private {
On 2012/06/18 21:17:24, brettw wrote:
> Do you mind making this a dev interface instead of a private one? Sorry to
make
> you move/rename the file. But this is the type of thing we want to expose to
web
> content eventually, rather than something we want to keep internal to the
> browser.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:31: PP_Bool GetDeviceScale([in]
PP_Resource resource,
On 2012/06/18 21:17:24, brettw wrote:
> I usually prefer APIs that just return the value when possible. So can this
be:
>   float GetDeviceScale([in] PP_Resource resource);
> and then say that it returns 0 for invalid resources?

Done.

https://chromiumcodereview.appspot.com/10544168/diff/1/ppapi/api/private/ppb_...
ppapi/api/private/ppb_view_private.idl:48: PP_Bool GetCSSScale([in] PP_Resource
resource,
On 2012/06/18 21:17:24, brettw wrote:
> Ditto on the return value.

Done.

[brettw, 2012-06-19 18:28:21]

API comments, didn't look at the impl yet.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
File ppapi/api/dev/ppb_graphics_2d_dev.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
ppapi/api/dev/ppb_graphics_2d_dev.idl:31: [in] PP_Resource resource,
Style nit: indent 2 more spaces (same below).

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
ppapi/api/dev/ppb_graphics_2d_dev.idl:39: */
This comment should specify the return value on invalid resource.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/graph...
File ppapi/cpp/dev/graphics_2d_dev.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/graph...
ppapi/cpp/dev/graphics_2d_dev.h:43: /// graphics context onto the output device.
This should also specify the return value on error.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/view_...
File ppapi/cpp/dev/view_dev.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/view_...
ppapi/cpp/dev/view_dev.cc:21: if (has_interface<PPB_View_Dev>()) {
It's not clear to me why this remaps error values. I think this should just
return the result of the C interface in all cases except when the interface
doesn't exist (when it should return 1.0 for backward-compat).

In this case, I liked the way you did it for the Graphics2D one better where you
early return if the interface doesn't exist. Same for the function below.

[brettw, 2012-06-19 20:21:27]

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppapi_m...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:850:
IPC_SYNC_MESSAGE_ROUTED2_1(PpapiHostMsg_PPBGraphics2D_Dev_SetScale,
Don't make this a sync message and just remove the "result". If we already
validated the resource on the plugin side, it should be almost impossible for it
to fail, and sync messages are much higher overhead.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppb_gra...
File ppapi/proxy/ppb_graphics_2d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppb_gra...
ppapi/proxy/ppb_graphics_2d_proxy.cc:47: float GetScale() { return scale_; }
Can you make this non-inline? It makes it difficult to find the code when it's
in a different place.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_view_shared.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_view_shared.cc:12: device_scale = 1.0;
I think you can skip setting these to 1 and just use 0. We'll have to set them
later (a default-initialized ViewData isn't actually useful for anything).

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/tests/test_gr...
File ppapi/tests/test_graphics_2d.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/tests/test_gr...
ppapi/tests/test_graphics_2d.cc:657: // Verify that the context has the
specified number of pixels, desipte the
spelling -> despite

https://chromiumcodereview.appspot.com/10544168/diff/4002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_2d_impl.h:54: virtual float GetScale()
OVERRIDE { return scale_; }
No inline virtual functions. This should throw an error in clang, I think.

[Josh Horwich, 2012-06-19 23:56:15]

Please take a look.

Updated based on review feedback, and disabled running tests of the
Graphics2D_Dev interfaces under NaCl

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
File ppapi/api/dev/ppb_graphics_2d_dev.idl (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
ppapi/api/dev/ppb_graphics_2d_dev.idl:31: [in] PP_Resource resource,
On 2012/06/19 18:28:21, brettw wrote:
> Style nit: indent 2 more spaces (same below).

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/api/dev/ppb_g...
ppapi/api/dev/ppb_graphics_2d_dev.idl:39: */
On 2012/06/19 18:28:21, brettw wrote:
> This comment should specify the return value on invalid resource.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/graph...
File ppapi/cpp/dev/graphics_2d_dev.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/graph...
ppapi/cpp/dev/graphics_2d_dev.h:43: /// graphics context onto the output device.
On 2012/06/19 18:28:21, brettw wrote:
> This should also specify the return value on error.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/view_...
File ppapi/cpp/dev/view_dev.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/cpp/dev/view_...
ppapi/cpp/dev/view_dev.cc:21: if (has_interface<PPB_View_Dev>()) {
On 2012/06/19 18:28:21, brettw wrote:
> It's not clear to me why this remaps error values. I think this should just
> return the result of the C interface in all cases except when the interface
> doesn't exist (when it should return 1.0 for backward-compat).
> 
> In this case, I liked the way you did it for the Graphics2D one better where
you
> early return if the interface doesn't exist. Same for the function below.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppapi_m...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:850:
IPC_SYNC_MESSAGE_ROUTED2_1(PpapiHostMsg_PPBGraphics2D_Dev_SetScale,
On 2012/06/19 20:21:27, brettw wrote:
> Don't make this a sync message and just remove the "result". If we already
> validated the resource on the plugin side, it should be almost impossible for
it
> to fail, and sync messages are much higher overhead.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppb_gra...
File ppapi/proxy/ppb_graphics_2d_proxy.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/proxy/ppb_gra...
ppapi/proxy/ppb_graphics_2d_proxy.cc:47: float GetScale() { return scale_; }
On 2012/06/19 20:21:27, brettw wrote:
> Can you make this non-inline? It makes it difficult to find the code when it's
> in a different place.

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_view_shared.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_view_shared.cc:12: device_scale = 1.0;
On 2012/06/19 20:21:27, brettw wrote:
> I think you can skip setting these to 1 and just use 0. We'll have to set them
> later (a default-initialized ViewData isn't actually useful for anything).

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/tests/test_gr...
File ppapi/tests/test_graphics_2d.cc (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/ppapi/tests/test_gr...
ppapi/tests/test_graphics_2d.cc:657: // Verify that the context has the
specified number of pixels, desipte the
On 2012/06/19 20:21:27, brettw wrote:
> spelling -> despite

Done.

https://chromiumcodereview.appspot.com/10544168/diff/4002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.h (right):

https://chromiumcodereview.appspot.com/10544168/diff/4002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_2d_impl.h:54: virtual float GetScale()
OVERRIDE { return scale_; }
On 2012/06/19 20:21:27, brettw wrote:
> No inline virtual functions. This should throw an error in clang, I think.

Done.

(Clang didn't complain, FWIW, but I moved it into the .cc file for consistency.)

[brettw, 2012-06-22 17:59:16]

LGTM!

[commit-bot: I haz the power, 2012-06-28 18:32:12]

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

[commit-bot: I haz the power, 2012-06-28 19:33:35]

Change committed as 144773