Re: Migrate Graphics2D to new design. (issue 11053003)

[victo...@chromium.org]

Done. Please try again. Sorry for bothering!

On 2012/11/07 18:37:43, brettw wrote:
> Ah, sInce you're now using PPB_Graphics2D_Impl outside of the
> webkit_glue project, you need to export it. So add
> WEBKIT_PLUGINS_EXPORT to the class declaration. You can remove it from
> the Create member then since everything in the class will be exported.
> Same with the resource helper class it mentions.

> You also have a warning about bool (use PP_ToBool)

> Brett

> On Wed, Nov 7, 2012 at 12:18 AM, Victor Hsieh
<mailto:victo...@chromium.org> wrote:
> > Brett,
> >
> > I've fixed chromium-style errors caught by buildbot (just learned that
> I can
> > turn on clang_use_chrome_plugins to catch it locally). But there is one
> > more error of "inconsistent dll linkage" on Windows that seems to
> relate to
> > WEBKIT_PLUGINS_EXPORT. Do you have any idea?
> >
> > Victor
> >
> > [Full log:
> >

http://build.chromium.org/p/tryserver.chromium/builders/win/builds/35768/steps/compile/logs/stdio%5D
> >
> > FAILED: ninja -t msvc -r . -o
> > obj\webkit\plugins\ppapi\content.ppb_graphics_2d_impl.obj -e
> environment.x86
> > -- E:\b\build\goma\gomacc.exe cl.exe /nologo /showIncludes /FC
> > @obj\webkit\plugins\ppapi\content.ppb_graphics_2d_impl.obj.rsp /c
> > ..\..\webkit\plugins\ppapi\ppb_graphics_2d_impl.cc
> > /Foobj\webkit\plugins\ppapi\content.ppb_graphics_2d_impl.obj
> /Fdcontent.pdb
> >

e:\b\build\slave\win\build\src\webkit\plugins\ppapi\ppb_graphics_2d_impl.cc(183)
> > : error C2220: warning treated as error - no 'object' file generated
> >
> >

e:\b\build\slave\win\build\src\webkit\plugins\ppapi\ppb_graphics_2d_impl.cc(183)
> > : warning C4273: 'webkit::ppapi::PPB_Graphics2D_Impl::Create' :
> inconsistent
> > dll linkage
> >
> >
> >
> e:\b\build\slave\win\build\src\webkit\plugins\ppapi\ppb_graphics_2d_impl.h(40)
> > : see previous definition of 'Create'
> >
> >

e:\b\build\slave\win\build\src\webkit\plugins\ppapi\ppb_graphics_2d_impl.cc(642)
> > : warning C4273:
> > 'webkit::ppapi::PPB_Graphics2D_Impl::ConvertToLogicalPixels' :
> inconsistent
> > dll linkage
> >
> >
> >
> e:\b\build\slave\win\build\src\webkit\plugins\ppapi\ppb_graphics_2d_impl.h(90)
> > : see previous definition of 'ConvertToLogicalPixels'
> >
> >
> >
> >
> > On Wed, Nov 7, 2012 at 1:34 PM, <mailto:commi...@chromium.org> wrote:
> >>
> >> Sorry for I got bad news for ya.
> >> Compile failed with a clobber build on linux_clang.
> >>
> >>

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=62000
> >>
> >> Your code is likely broken or HEAD is junk. Please ensure your
> >> code is not broken then alert the build sheriffs.
> >> Look at the try server FAQ for more details.
> >>
> >> https://chromiumcodereview.appspot.com/11053003/
> >
> >



https://codereview.chromium.org/11053003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victo...@chromium.org/11053003/81043


https://chromiumcodereview.appspot.com/11053003/

[commi...@chromium.org]

Sorry for I got bad news for ya.

Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_dbg&number=12920

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/victo...@chromium.org/11053003/91004


https://chromiumcodereview.appspot.com/11053003/

[commi...@chromium.org]

Presubmit check for 11053003-91004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
ppapi/proxy/ppapi_messages.h

Presubmit checks took 6.9s to calculate.



https://chromiumcodereview.appspot.com/11053003/

[victo...@chromium.org]

@cdn, would you please take a look at ppapi_messages.h? Thanks.

On 2012/11/13 19:12:10, I haz the power (commit-bot) wrote:
> Presubmit check for 11053003-91004 failed and returned exit status 1.


> Running presubmit commit checks ...

> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].

> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
> ppapi/proxy/ppapi_messages.h

> Presubmit checks took 6.9s to calculate.

https://codereview.chromium.org/11053003/

[c...@chromium.org]

https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc
File ppapi/proxy/graphics_2d_resource.cc (right):

https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc#newcode36
ppapi/proxy/graphics_2d_resource.cc:36:
std::numeric_limits<int32>::max();
when width * height * 4 wraps the 64 bit integer this check will pass.
Seems like you would either need to do an unsigned comparison or check
for a negative result.

https://codereview.chromium.org/11053003/

[victo...@chromium.org]

https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc
File ppapi/proxy/graphics_2d_resource.cc (right):

https://codereview.chromium.org/11053003/diff/81096/ppapi/proxy/graphics_2d_resource.cc#newcode36
ppapi/proxy/graphics_2d_resource.cc:36:
std::numeric_limits<int32>::max();

On 2012/11/14 02:03:46, Cris Neckar wrote:
> when width * height * 4 wraps the 64 bit integer this check will pass.
Seems
> like you would either need to do an unsigned comparison or check for a
negative
> result.

I think the int32 max will be promoted to int64 when comparing, so we
don't need to worry about overflow.

https://codereview.chromium.org/11053003/

[c...@chromium.org]

That's correct it will be promoted to a signed 64 bit into but will wrap to
causing the check to pass.


https://codereview.chromium.org/11053003/

[c...@chromium.org]

Sorry my phone butchered that. Should say wrap to negative. For example
take the
inputs 0xffffffff for both height and width. This will wrap to a negative
64 bit
number.

https://codereview.chromium.org/11053003/

[victo...@chromium.org]

Oh I see what you were saying. Fixed, plus a test case. Thanks for
catching.

https://codereview.chromium.org/11053003/

[c...@chromium.org]

On 2012/11/14 04:38:50, Victor Hsieh wrote:
> Oh I see what you were saying. Fixed, plus a test case. Thanks for
> catching.

That fix should work. Could you also just comment high level on whether
this is
introducing any additional IPCs (or attack surface). It seems to just be
moving
things but I want to make sure I am understanding it correctly.

https://codereview.chromium.org/11053003/

[victo...@chromium.org]

On 2012/11/14 05:16:23, Cris Neckar wrote:
> On 2012/11/14 04:38:50, Victor Hsieh wrote:
> > Oh I see what you were saying. Fixed, plus a test case. Thanks for
catching.

> That fix should work. Could you also just comment high level on whether
> this
is
> introducing any additional IPCs (or attack surface). It seems to just be
moving
> things but I want to make sure I am understanding it correctly.

You remind me that there are one test only message
(PpapiHostMsg_Graphics2D_ReadImageData) and another for ACK. But that
should be
safe from what I can tell. And it's test only, we don't want to call that
for
real since it's a blocking call. Other than that, there is no new message.

https://codereview.chromium.org/11053003/

[c...@chromium.org]

https://codereview.chromium.org/11053003/diff/91008/webkit/plugins/ppapi/ppb_image_data_impl.cc
File webkit/plugins/ppapi/ppb_image_data_impl.cc (right):

https://codereview.chromium.org/11053003/diff/91008/webkit/plugins/ppapi/ppb_image_data_impl.cc#newcode58
webkit/plugins/ppapi/ppb_image_data_impl.cc:58:
std::numeric_limits<int32>::max() / 4)
Oops, actually we need to fix this in a separate patch so that it can be
merged back to other branches.

https://codereview.chromium.org/11053003/

[c...@chromium.org]

> Oops, actually we need to fix this in a separate patch so that it can be
merged
> back to other branches.

I filed a bug and CCed you

https://codereview.chromium.org/11053003/

[p...@chromium.org]

Drive-by: Drive-by: Please be more complete in your change description. See
other drive-by comment, in review 11274036. Thanks.

https://chromiumcodereview.appspot.com/11053003/

[victo...@chromium.org]

On 2012/11/14 13:07:48, Pam wrote:
> Drive-by: Drive-by: Please be more complete in your change description.
> See
> other drive-by comment, in review 11274036. Thanks.

It's actually part of a large refactoring effort, that we migrate resources
one
by one. Anyway, I've updated the description to provide more information.

https://chromiumcodereview.appspot.com/11053003/

[c...@chromium.org]

The new IPC looks OK but I question whether we actually need to add the
potential attack surface if it is only being used for testing. It would be
nice
if we could avoid adding IPCs that aren't used as they introduce attack
surface
which a compromised renderer could use as a sandbox bypass.

other than that nit lgtm

http://codereview.chromium.org/11053003/

[victo...@chromium.org]

Unfortunately it's not trivial to not introduce that message. As I
mentioned,
we are refactoring things one by one, ImageData is a hard one. We should
have a
change to remove this when refactoring ImageData.

http://codereview.chromium.org/11053003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/victo...@chromium.org/11053003/91008


https://chromiumcodereview.appspot.com/11053003/

[commi...@chromium.org]

Retried try job too often for step(s) nacl_integration

https://chromiumcodereview.appspot.com/11053003/