Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1881603002:
Added SetLayerTransform to PPAPI (Closed)
|
Created:
4 years, 8 months ago by alessandroa Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, jam, teravest+watch_chromium.org, darin-cc_chromium.org, bradnelson+warch_chromium.org, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, ihf+watch_chromium.org, bokan Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded SetLayerTransform to PPAPI.
SetLayerTransform() sets a transformation factor that will be applied for the current
layer displayed on the output device.
This function has no effect until you call Flush().
@param[in] scale The scale to be applied.
@param[in] origin The origin of the scale.
@param[in] translate The translation to be applied.
@return Returns true on success or false if the resource is invalid or the scale factor is 0 or less.
BUG=605379
NOPRESUBMIT=true
Reason:
** Presubmit ERRORS **
TODOs found in stable public PPAPI files:
ppapi/cpp/graphics_2d.h
Committed: https://crrev.com/d55efd0d4b7573d763dd6e4d1d9ccfe4ca5be2f7
Cr-Commit-Position: refs/heads/master@{#388851}
Patch Set 1 #
Total comments: 55
Patch Set 2 : Cleaned up the code #
Total comments: 16
Patch Set 3 : Fixed indentation #
Total comments: 19
Patch Set 4 : Moved origin before IPC #Patch Set 5 : #
Total comments: 6
Patch Set 6 : Added M52 to API idl #
Total comments: 4
Patch Set 7 : Cleaned bad copy paste #Patch Set 8 : Changes for version 1.2 of Graphics2D #
Total comments: 4
Patch Set 9 : Changed PPAPI version in tests #Patch Set 10 : Added Graphics2D 1.2 to histograms #Dependent Patchsets: Messages
Total messages: 72 (32 generated)
Description was changed from ========== Added SetLayerTransform to PPAPI BUG= ========== to ========== Added SetLayerTransform to PPAPI BUG= ==========
alessandroa@google.com changed reviewers: + wjmaclean@chromium.org, wjmaclean@google.com
wjmaclean@chromium.org changed reviewers: - wjmaclean@google.com
Here's an initial set of comments. Most are style related. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.cc (left): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:501: // Reuse image data when running out of process. Why are you deleting this comment? Doesn't it apply any longer? If so, what is the purpose of the fluch now? https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:710: please restore this blank line. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:499: return PP_ERROR_INPROGRESS; Please restore the blank line below. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:531: ppapi::host::HostMessageContext* context, indenting looks wrong here ... the arguments should line up vertically https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:534: const PP_Point& transform) { 'transform' seems like the wrong name here ... is this meant to be a translation? https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:538: gfx::Point P(origin.x, origin.y); Call this 'origin' instead of 'P' https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:539: gfx::Point T(transform.x, transform.y); Call this 'translation' instead of 'T'. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:542: transform_matrix.Translate(SkFloatToScalar((1 - S) * P.x() - T.x()), Perhaps add a comment explaining in words what the transformation here represents. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:543: SkFloatToScalar((1 - S) * P.y() - T.y())); this indentation is wrong https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.h:91: const PP_Point& Origin, nit: indenting is wrong here. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.h:209: gfx::Transform transform_; Add a comment explaining what this will be used for. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1573: // If the transform_ is te identity matrix the https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1577: gfx::PointF Translate = gfx::PointF(transform_.matrix().getFloat(0,3), Add a comment to warn the reader here that we're assuming the transform only contains 2-D scale and 2-D translation. Even better, add DCHECK(transform.isScaleTranslate()); https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1580: gfx::PointF top_left = gfx::PointF(-1 * Translate.x() / scale , -Translate.x() ? https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1583: - 1 * Translate.x() / scale, Ditto. Also, even if you are using -1, you shouldn't write "- 1". Finally, use a float literal, i.e. -1.f or -1.0f. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.h:548: void SetLayerTransform(gfx::Transform transform); I would place this function prototype above the ones that implement overrides on inherited classes. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.h:715: //Transform to apply to UV Comments should be proper sentences, so it needs a period. Also, what are UV (people reading this comment shouldn't have to read the entire class to understand that UV are texture coordinates). https://codereview.chromium.org/1881603002/diff/1/ppapi/c/ppb_graphics_2d.h File ppapi/c/ppb_graphics_2d.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/c/ppb_graphics_2d.h#n... ppapi/c/ppb_graphics_2d.h:280: float Scale, don't capitalize parameter names https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.cc File ppapi/cpp/graphics_2d.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.cc#ne... ppapi/cpp/graphics_2d.cc:161: pp_resource(), Indenting is wrong here. https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.h File ppapi/cpp/graphics_2d.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.h#new... ppapi/cpp/graphics_2d.h:287: bool SetLayerTransform(float Scale, Don't capitalize variable names. https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:872: static PP_Bool Pnacl_M27_PPB_Graphics2D_SetLayerTransform(PP_Resource resource, float Scale, const struct PP_Point* Origin, const struct PP_Point* Transform) { Don't capitalize variable names. https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:4801: .SetLayerTransform = (PP_Bool (*)(PP_Resource resource, float Scale, const struct PP_Point* Origin, const struct PP_Point* Transform))&Pnacl_M27_PPB_Graphics2D_SetLayerTransform Don't capitalize variable names. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... ppapi/proxy/graphics_2d_resource.cc:117: const PP_Point* origin, Indenting is wrong here. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... ppapi/proxy/graphics_2d_resource.cc:120: return PP_FALSE; nit: Add blank line after this line. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... File ppapi/proxy/graphics_2d_resource.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... ppapi/proxy/graphics_2d_resource.h:46: const PP_Point* Origin, Don't capitalize variable names. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/ppapi_messages.... ppapi/proxy/ppapi_messages.h:1660: PP_Point /* Origin */, Don't capitalize variable names, even in comments. https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... File ppapi/thunk/ppb_graphics_2d_api.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... ppapi/thunk/ppb_graphics_2d_api.h:39: virtual PP_Bool SetLayerTransform(float Scale, Don't capitalize variable names. https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... File ppapi/thunk/ppb_graphics_2d_thunk.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... ppapi/thunk/ppb_graphics_2d_thunk.cc:106: float Scale, Don't capitalize variable names.
One other thing: you should write a detailed description for this issue, as it will need one before it lands, and the description should be part of the review process.
On 2016/04/13 18:31:15, wjmaclean wrote: > One other thing: you should write a detailed description for this issue, as it > will need one before it lands, and the description should be part of the review > process. Should I just add a link to the design document ? :)
On 2016/04/13 19:05:51, alessandroa wrote: > On 2016/04/13 18:31:15, wjmaclean wrote: > > One other thing: you should write a detailed description for this issue, as it > > will need one before it lands, and the description should be part of the > review > > process. > > Should I just add a link to the design document ? :) No. The description you include in this issue will become a permanent part of the commit in the repo. A link to an external document is not helpful since (1) the document might disappear, and (2) even if it doesn't, not everyone (i.e. the public) can access your design doc.
Description was changed from ========== Added SetLayerTransform to PPAPI BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ==========
On 2016/04/13 20:12:45, wjmaclean wrote: > On 2016/04/13 19:05:51, alessandroa wrote: > > On 2016/04/13 18:31:15, wjmaclean wrote: > > > One other thing: you should write a detailed description for this issue, as > it > > > will need one before it lands, and the description should be part of the > > review > > > process. > > > > Should I just add a link to the design document ? :) > > No. The description you include in this issue will become a permanent part of > the commit in the repo. A link to an external document is not helpful since (1) > the document might disappear, and (2) even if it doesn't, not everyone (i.e. the > public) can access your design doc. Would this one be ok ?
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> If the resource is invalid or the scale factor is 0 or less. BUG= ==========
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> If the resource is invalid or the scale factor is 0 or less. BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ==========
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current graphics context displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ==========
On 2016/04/13 21:48:11, alessandroa wrote: > On 2016/04/13 20:12:45, wjmaclean wrote: > > On 2016/04/13 19:05:51, alessandroa wrote: > > > On 2016/04/13 18:31:15, wjmaclean wrote: > > > > One other thing: you should write a detailed description for this issue, > as > > it > > > > will need one before it lands, and the description should be part of the > > > review > > > > process. > > > > > > Should I just add a link to the design document ? :) > > > > No. The description you include in this issue will become a permanent part of > > the commit in the repo. A link to an external document is not helpful since > (1) > > the document might disappear, and (2) even if it doesn't, not everyone (i.e. > the > > public) can access your design doc. > > Would this one be ok ? Sure, that looks like a good start ... please delete the <code></code> tags though as git log won't make any use of them.
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns <code>true</code> on success or <code>false</code> if the resource is invalid or the scale factor is 0 or less. BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG= ==========
On 2016/04/14 13:45:42, wjmaclean wrote: > On 2016/04/13 21:48:11, alessandroa wrote: > > On 2016/04/13 20:12:45, wjmaclean wrote: > > > On 2016/04/13 19:05:51, alessandroa wrote: > > > > On 2016/04/13 18:31:15, wjmaclean wrote: > > > > > One other thing: you should write a detailed description for this issue, > > as > > > it > > > > > will need one before it lands, and the description should be part of the > > > > review > > > > > process. > > > > > > > > Should I just add a link to the design document ? :) > > > > > > No. The description you include in this issue will become a permanent part > of > > > the commit in the repo. A link to an external document is not helpful since > > (1) > > > the document might disappear, and (2) even if it doesn't, not everyone (i.e. > > the > > > public) can access your design doc. > > > > Would this one be ok ? > > Sure, that looks like a good start ... please delete the <code></code> tags > though as git log won't make any use of them. True :) solved
alessandroa@google.com changed reviewers: + piman@chromium.org
alessandroa@google.com changed required reviewers: + piman@chromium.org
A few more indenting issues. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.cc:542: //conform the origin point given Needs a period. I'm not sure I understand what "conform" means here ... "around" perhaps? https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:209: // This is the transform that will be applied to the layer A comment is a sentence, so it needs a period. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1585: -Translate.x() / scale, I hadn't realized this is the continuation of an expression on the previous line ... should be "- Translate..." in that case. Sorry. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1586: (1 / scale) * plugin_size_in_dip.height() Why is this wrapping around? Is this line longer than 80 chars? https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:716: // This is the transform that will be applied to the layer Needs a period. https://codereview.chromium.org/1881603002/diff/20001/ppapi/cpp/graphics_2d.cc File ppapi/cpp/graphics_2d.cc (right): https://codereview.chromium.org/1881603002/diff/20001/ppapi/cpp/graphics_2d.c... ppapi/cpp/graphics_2d.cc:160: return PP_ToBool(get_interface<PPB_Graphics2D_1_1>()->SetLayerTransform(pp_resource(), Indentation is still wrong. return PP_ToBool(get_interface<PPB_Graphics2D_1_1>()->SetLayerTransform( pp_resource(), scale, &origin.pp_point(), &translate.pp_point())); https://codereview.chromium.org/1881603002/diff/20001/ppapi/proxy/graphics_2d... File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/1881603002/diff/20001/ppapi/proxy/graphics_2d... ppapi/proxy/graphics_2d_resource.cc:122: Post(RENDERER, PpapiHostMsg_Graphics2D_SetLayerTransform(scale, Post(RENDERER, PpapiHostMsg_Graphics2D_SetLayerTransform(scale, *origin, *translate));
bokan@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:124: float Scale, Line up indent with the first parameter. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:140: void ExecuteTransform(gfx::Transform transform); pass by const reference (i.e. const gfx::Transform& transform) https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1550: DCHECK(transform.IsScaleOrTranslation()); Any reason this can't be an arbitrary transform? This API could be used outside of the PDF pinch zoom case. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); pass by const reference.
https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1550: DCHECK(transform.IsScaleOrTranslation()); On 2016/04/14 21:45:01, bokan wrote: > Any reason this can't be an arbitrary transform? This API could be used outside > of the PDF pinch zoom case. Actually, because it's used for the UVs, it can only be a 2D axis-aligned transform (this is even too loose). But, any reason to go from 2D scale+origin+translate to gfx::Transform, back to (essentially) 2D scale+origin+translate? It seems simpler to just pass those? Actually equivalently, just a 2D scale + translate (the origin can be backed into the translate) can represent the range of transforms. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); nit: this only applies to Graphics2D, maybe this should be explicit in the name, e.g. SetGraphics2DTransform ? https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:284: [version=1.1] We need to rev the version (see at the top of the file - 1.2?), and add comments :) https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... File ppapi/thunk/ppb_graphics_2d_api.h (right): https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... ppapi/thunk/ppb_graphics_2d_api.h:40: const PP_Point* srigin, typo: srigin->origin
On 2016/04/14 23:11:35, piman wrote: > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > content/renderer/pepper/pepper_plugin_instance_impl.cc:1550: > DCHECK(transform.IsScaleOrTranslation()); > On 2016/04/14 21:45:01, bokan wrote: > > Any reason this can't be an arbitrary transform? This API could be used > outside > > of the PDF pinch zoom case. > > Actually, because it's used for the UVs, it can only be a 2D axis-aligned > transform (this is even too loose). > > But, any reason to go from 2D scale+origin+translate to gfx::Transform, back to > (essentially) 2D scale+origin+translate? > > It seems simpler to just pass those? Actually equivalently, just a 2D scale + > translate (the origin can be backed into the translate) can represent the range > of transforms. > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > File content/renderer/pepper/pepper_plugin_instance_impl.h (right): > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > content/renderer/pepper/pepper_plugin_instance_impl.h:375: void > SetLayerTransform(gfx::Transform transform); > nit: this only applies to Graphics2D, maybe this should be explicit in the name, > e.g. SetGraphics2DTransform ? > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... > File ppapi/api/ppb_graphics_2d.idl (right): > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... > ppapi/api/ppb_graphics_2d.idl:284: [version=1.1] > We need to rev the version (see at the top of the file - 1.2?), and add comments > :) > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... > File ppapi/thunk/ppb_graphics_2d_api.h (right): > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... > ppapi/thunk/ppb_graphics_2d_api.h:40: const PP_Point* srigin, > typo: srigin->origin Solved :)
On 2016/04/14 23:11:35, piman wrote: > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > content/renderer/pepper/pepper_plugin_instance_impl.cc:1550: > DCHECK(transform.IsScaleOrTranslation()); > On 2016/04/14 21:45:01, bokan wrote: > > Any reason this can't be an arbitrary transform? This API could be used > outside > > of the PDF pinch zoom case. > > Actually, because it's used for the UVs, it can only be a 2D axis-aligned > transform (this is even too loose). > > But, any reason to go from 2D scale+origin+translate to gfx::Transform, back to > (essentially) 2D scale+origin+translate? > > It seems simpler to just pass those? Actually equivalently, just a 2D scale + > translate (the origin can be backed into the translate) can represent the range > of transforms. > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > File content/renderer/pepper/pepper_plugin_instance_impl.h (right): > > https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... > content/renderer/pepper/pepper_plugin_instance_impl.h:375: void > SetLayerTransform(gfx::Transform transform); > nit: this only applies to Graphics2D, maybe this should be explicit in the name, > e.g. SetGraphics2DTransform ? > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... > File ppapi/api/ppb_graphics_2d.idl (right): > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... > ppapi/api/ppb_graphics_2d.idl:284: [version=1.1] > We need to rev the version (see at the top of the file - 1.2?), and add comments > :) > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... > File ppapi/thunk/ppb_graphics_2d_api.h (right): > > https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... > ppapi/thunk/ppb_graphics_2d_api.h:40: const PP_Point* srigin, > typo: srigin->origin Solved :)
https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:90: void SetLayerTransform(float Scale, const PP_Point& Transform); nit: lowercase |scale|, |transform| per style. https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:15: M27 = 1.1 I believe you need to add a version here, M52 I believe. https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:295: * @return Returns <code>true</code> on success or <code>false</code> nit: PP_TRUE, PP_FALSE instead of true, false.
On 2016/04/15 19:24:39, piman wrote: > https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... > File content/renderer/pepper/pepper_graphics_2d_host.h (right): > > https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... > content/renderer/pepper/pepper_graphics_2d_host.h:90: void > SetLayerTransform(float Scale, const PP_Point& Transform); > nit: lowercase |scale|, |transform| per style. > > https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... > File ppapi/api/ppb_graphics_2d.idl (right): > > https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... > ppapi/api/ppb_graphics_2d.idl:15: M27 = 1.1 > I believe you need to add a version here, M52 I believe. > > https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... > ppapi/api/ppb_graphics_2d.idl:295: * @return Returns <code>true</code> on > success or <code>false</code> > nit: PP_TRUE, PP_FALSE instead of true, false. Solved :) This lead to a small change here ppapi/cpp/graphics_2d.cc too :)
https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... ppapi/api/ppb_graphics_2d.idl:75: * M resource is invalid. The output parameters will be set to 0 on a bad copy & paste? https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... ppapi/api/ppb_graphics_2d.idl:131: M same?
On 2016/04/15 20:12:28, piman wrote: > https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... > File ppapi/api/ppb_graphics_2d.idl (right): > > https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... > ppapi/api/ppb_graphics_2d.idl:75: * M resource is invalid. The output parameters > will be set to 0 on a > bad copy & paste? > > https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... > ppapi/api/ppb_graphics_2d.idl:131: M > same? Ups .. my bad. Solved :)
lgtm
alessandroa@google.com changed reviewers: + raymes@chromium.org
alessandroa@google.com changed required reviewers: + raymes@chromium.org
Looks good. I didn't look at the computations in close detail because piman is a better reviewer for that. Should we add a test to test_graphics_2d.cc for this? https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... content/renderer/pepper/pepper_graphics_2d_host.h:129: // reused. Tis is used by ReplaceContents. nit: This https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... content/renderer/pepper/pepper_plugin_instance_impl.cc:1578: // Adding the SetLayerTransform from Graphics2D to the UV. nit: Fill 80 chars for comments https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... content/renderer/pepper/pepper_plugin_instance_impl.h:717: // Those are the scale and the translation that will be applied to the layer. nit: Those->These https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... ppapi/api/ppb_graphics_2d.idl:291: * This function has no effect until you call Flush(). nit fill 80 chars above or add a new line above for a new paragraph. Also add a new line below
On 2016/04/20 at 00:48:55, raymes wrote: > Looks good. I didn't look at the computations in close detail because piman is a better reviewer for that. Should we add a test to test_graphics_2d.cc for this? > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > File content/renderer/pepper/pepper_graphics_2d_host.h (right): > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > content/renderer/pepper/pepper_graphics_2d_host.h:129: // reused. Tis is used by ReplaceContents. > nit: This > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > content/renderer/pepper/pepper_plugin_instance_impl.cc:1578: // Adding the SetLayerTransform from Graphics2D to the UV. > nit: Fill 80 chars for comments > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > File content/renderer/pepper/pepper_plugin_instance_impl.h (right): > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > content/renderer/pepper/pepper_plugin_instance_impl.h:717: // Those are the scale and the translation that will be applied to the layer. > nit: Those->These > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > File ppapi/api/ppb_graphics_2d.idl (right): > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > ppapi/api/ppb_graphics_2d.idl:291: * This function has no effect until you call Flush(). > nit fill 80 chars above or add a new line above for a new paragraph. Also add a new line below Solved :) We will add tests later as there is no feasible approach for that now ..
On Wed, Apr 20, 2016 at 1:17 PM, <alessandroa@google.com> wrote: > On 2016/04/20 at 00:48:55, raymes wrote: > > Looks good. I didn't look at the computations in close detail because > piman is > a better reviewer for that. Should we add a test to test_graphics_2d.cc for > this? > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > File content/renderer/pepper/pepper_graphics_2d_host.h (right): > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > content/renderer/pepper/pepper_graphics_2d_host.h:129: // reused. Tis is > used > by ReplaceContents. > > nit: This > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > content/renderer/pepper/pepper_plugin_instance_impl.cc:1578: // Adding > the > SetLayerTransform from Graphics2D to the UV. > > nit: Fill 80 chars for comments > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > File content/renderer/pepper/pepper_plugin_instance_impl.h (right): > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > content/renderer/pepper/pepper_plugin_instance_impl.h:717: // Those are > the > scale and the translation that will be applied to the layer. > > nit: Those->These > > > > > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > > File ppapi/api/ppb_graphics_2d.idl (right): > > > > > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > > ppapi/api/ppb_graphics_2d.idl:291: * This function has no effect until > you > call Flush(). > > nit fill 80 chars above or add a new line above for a new paragraph. > Also add > a new line below > > Solved :) We will add tests later as there is no feasible approach for > that now > Could you file a bug for this? > .. > > https://codereview.chromium.org/1881603002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/20 at 20:25:32, piman wrote: > On Wed, Apr 20, 2016 at 1:17 PM, <alessandroa@google.com> wrote: > > > On 2016/04/20 at 00:48:55, raymes wrote: > > > Looks good. I didn't look at the computations in close detail because > > piman is > > a better reviewer for that. Should we add a test to test_graphics_2d.cc for > > this? > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > File content/renderer/pepper/pepper_graphics_2d_host.h (right): > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > content/renderer/pepper/pepper_graphics_2d_host.h:129: // reused. Tis is > > used > > by ReplaceContents. > > > nit: This > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > content/renderer/pepper/pepper_plugin_instance_impl.cc:1578: // Adding > > the > > SetLayerTransform from Graphics2D to the UV. > > > nit: Fill 80 chars for comments > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > File content/renderer/pepper/pepper_plugin_instance_impl.h (right): > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/content/renderer/peppe... > > > content/renderer/pepper/pepper_plugin_instance_impl.h:717: // Those are > > the > > scale and the translation that will be applied to the layer. > > > nit: Those->These > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > > > File ppapi/api/ppb_graphics_2d.idl (right): > > > > > > > > > > https://codereview.chromium.org/1881603002/diff/140001/ppapi/api/ppb_graphics... > > > ppapi/api/ppb_graphics_2d.idl:291: * This function has no effect until > > you > > call Flush(). > > > nit fill 80 chars above or add a new line above for a new paragraph. > > Also add > > a new line below > > > > Solved :) We will add tests later as there is no feasible approach for > > that now > > > > Could you file a bug for this? > > > > .. > > > > https://codereview.chromium.org/1881603002/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. > Sure : https://bugs.chromium.org/p/chromium/issues/detail?id=605287
lgtm
The CQ bit was checked by alessandroa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1881603002/#ps160001 (title: "Changed PPAPI version in tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881603002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881603002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bokan@chromium.org changed reviewers: + kenrb@chromium.org
still need approval from an owner of ppapi_messages. +kenrb@
On 2016/04/21 01:28:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) None of those are related to me. What shall I do ? ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: ppapi/proxy/ppapi_messages.h TODOs found in stable public PPAPI files: *************** ppapi/cpp/graphics_2d.h ***************
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881603002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881603002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Also, before committing this patch, please create a tracking bug for the "improve PDF pinch-zoom" project and add it to the "BUG=" entry in the description
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG= ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=605379 ==========
On 2016/04/21 02:01:49, bokan wrote: > Also, before committing this patch, please create a tracking bug for the > "improve PDF pinch-zoom" project and add it to the "BUG=" entry in the > description Sure .. added :)
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=605379 ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 ==========
ipc lgtm
The CQ bit was checked by alessandroa@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881603002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881603002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.cc (left): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:501: // Reuse image data when running out of process. On 2016/04/13 14:47:18, wjmaclean wrote: > Why are you deleting this comment? Doesn't it apply any longer? If so, what is > the purpose of the fluch now? It was a stupid mistake :) solved. I thought it was my comment https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:710: On 2016/04/13 14:47:18, wjmaclean wrote: > please restore this blank line. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:499: return PP_ERROR_INPROGRESS; On 2016/04/13 14:47:18, wjmaclean wrote: > Please restore the blank line below. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:531: ppapi::host::HostMessageContext* context, On 2016/04/13 14:47:18, wjmaclean wrote: > indenting looks wrong here ... the arguments should line up vertically Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:534: const PP_Point& transform) { On 2016/04/13 14:47:18, wjmaclean wrote: > 'transform' seems like the wrong name here ... is this meant to be a > translation? Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:534: const PP_Point& transform) { On 2016/04/13 14:47:18, wjmaclean wrote: > 'transform' seems like the wrong name here ... is this meant to be a > translation? Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:538: gfx::Point P(origin.x, origin.y); On 2016/04/13 14:47:18, wjmaclean wrote: > Call this 'origin' instead of 'P' Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:539: gfx::Point T(transform.x, transform.y); On 2016/04/13 14:47:18, wjmaclean wrote: > Call this 'translation' instead of 'T'. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.cc:542: transform_matrix.Translate(SkFloatToScalar((1 - S) * P.x() - T.x()), On 2016/04/13 14:47:18, wjmaclean wrote: > Perhaps add a comment explaining in words what the transformation here > represents. Done. I simplified all this part :) You will see in the next commit https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.h:91: const PP_Point& Origin, On 2016/04/13 14:47:18, wjmaclean wrote: > nit: indenting is wrong here. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_graphics_2d_host.h:209: gfx::Transform transform_; On 2016/04/13 14:47:18, wjmaclean wrote: > Add a comment explaining what this will be used for. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1573: // If the transform_ is te identity matrix On 2016/04/13 14:47:19, wjmaclean wrote: > the Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1577: gfx::PointF Translate = gfx::PointF(transform_.matrix().getFloat(0,3), On 2016/04/13 14:47:19, wjmaclean wrote: > Add a comment to warn the reader here that we're assuming the transform only > contains 2-D scale and 2-D translation. > > Even better, add > > DCHECK(transform.isScaleTranslate()); Nice one :) https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1580: gfx::PointF top_left = gfx::PointF(-1 * Translate.x() / scale , On 2016/04/13 14:47:18, wjmaclean wrote: > -Translate.x() ? Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:1583: - 1 * Translate.x() / scale, On 2016/04/13 14:47:18, wjmaclean wrote: > Ditto. Also, even if you are using -1, you shouldn't write "- 1". Finally, use a > float literal, i.e. -1.f or -1.0f. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.h:548: void SetLayerTransform(gfx::Transform transform); On 2016/04/13 14:47:19, wjmaclean wrote: > I would place this function prototype above the ones that implement overrides on > inherited classes. Done. https://codereview.chromium.org/1881603002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.h:715: //Transform to apply to UV On 2016/04/13 14:47:19, wjmaclean wrote: > Comments should be proper sentences, so it needs a period. Also, what are UV > (people reading this comment shouldn't have to read the entire class to > understand that UV are texture coordinates). Done. https://codereview.chromium.org/1881603002/diff/1/ppapi/c/ppb_graphics_2d.h File ppapi/c/ppb_graphics_2d.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/c/ppb_graphics_2d.h#n... ppapi/c/ppb_graphics_2d.h:280: float Scale, On 2016/04/13 14:47:19, wjmaclean wrote: > don't capitalize parameter names Done. https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.cc File ppapi/cpp/graphics_2d.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.cc#ne... ppapi/cpp/graphics_2d.cc:161: pp_resource(), On 2016/04/13 14:47:19, wjmaclean wrote: > Indenting is wrong here. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.h File ppapi/cpp/graphics_2d.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/cpp/graphics_2d.h#new... ppapi/cpp/graphics_2d.h:287: bool SetLayerTransform(float Scale, On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names. Done. https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:872: static PP_Bool Pnacl_M27_PPB_Graphics2D_SetLayerTransform(PP_Resource resource, float Scale, const struct PP_Point* Origin, const struct PP_Point* Transform) { On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:4801: .SetLayerTransform = (PP_Bool (*)(PP_Resource resource, float Scale, const struct PP_Point* Origin, const struct PP_Point* Transform))&Pnacl_M27_PPB_Graphics2D_SetLayerTransform On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... ppapi/proxy/graphics_2d_resource.cc:117: const PP_Point* origin, On 2016/04/13 14:47:19, wjmaclean wrote: > Indenting is wrong here. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/graphics_2d_res... ppapi/proxy/graphics_2d_resource.cc:120: return PP_FALSE; On 2016/04/13 14:47:19, wjmaclean wrote: > nit: Add blank line after this line. Done. https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/proxy/ppapi_messages.... ppapi/proxy/ppapi_messages.h:1660: PP_Point /* Origin */, On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names, even in comments. Acknowledged. https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... File ppapi/thunk/ppb_graphics_2d_api.h (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... ppapi/thunk/ppb_graphics_2d_api.h:39: virtual PP_Bool SetLayerTransform(float Scale, On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names. Done. https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... File ppapi/thunk/ppb_graphics_2d_thunk.cc (right): https://codereview.chromium.org/1881603002/diff/1/ppapi/thunk/ppb_graphics_2d... ppapi/thunk/ppb_graphics_2d_thunk.cc:106: float Scale, On 2016/04/13 14:47:19, wjmaclean wrote: > Don't capitalize variable names. Done. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.cc (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.cc:542: //conform the origin point given On 2016/04/14 20:14:16, wjmaclean wrote: > Needs a period. I'm not sure I understand what "conform" means here ... "around" > perhaps? Done. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.cc:542: //conform the origin point given On 2016/04/14 20:14:16, wjmaclean wrote: > Needs a period. I'm not sure I understand what "conform" means here ... "around" > perhaps? Done. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:209: // This is the transform that will be applied to the layer On 2016/04/14 20:14:16, wjmaclean wrote: > A comment is a sentence, so it needs a period. Done. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1585: -Translate.x() / scale, On 2016/04/14 20:14:16, wjmaclean wrote: > I hadn't realized this is the continuation of an expression on the previous line > ... should be "- Translate..." in that case. Sorry. Acknowledged. https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1586: (1 / scale) * plugin_size_in_dip.height() On 2016/04/14 20:14:16, wjmaclean wrote: > Why is this wrapping around? Is this line longer than 80 chars? Yep .. much longer but solved https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:716: // This is the transform that will be applied to the layer On 2016/04/14 20:14:16, wjmaclean wrote: > Needs a period. Done. https://codereview.chromium.org/1881603002/diff/20001/ppapi/cpp/graphics_2d.cc File ppapi/cpp/graphics_2d.cc (right): https://codereview.chromium.org/1881603002/diff/20001/ppapi/cpp/graphics_2d.c... ppapi/cpp/graphics_2d.cc:160: return PP_ToBool(get_interface<PPB_Graphics2D_1_1>()->SetLayerTransform(pp_resource(), On 2016/04/14 20:14:17, wjmaclean wrote: > Indentation is still wrong. > > return PP_ToBool(get_interface<PPB_Graphics2D_1_1>()->SetLayerTransform( > pp_resource(), scale, &origin.pp_point(), &translate.pp_point())); Done. https://codereview.chromium.org/1881603002/diff/20001/ppapi/proxy/graphics_2d... File ppapi/proxy/graphics_2d_resource.cc (right): https://codereview.chromium.org/1881603002/diff/20001/ppapi/proxy/graphics_2d... ppapi/proxy/graphics_2d_resource.cc:122: Post(RENDERER, PpapiHostMsg_Graphics2D_SetLayerTransform(scale, On 2016/04/14 20:14:17, wjmaclean wrote: > Post(RENDERER, > PpapiHostMsg_Graphics2D_SetLayerTransform(scale, *origin, *translate)); Done. https://codereview.chromium.org/1881603002/diff/20001/ppapi/proxy/graphics_2d... ppapi/proxy/graphics_2d_resource.cc:122: Post(RENDERER, PpapiHostMsg_Graphics2D_SetLayerTransform(scale, On 2016/04/14 20:14:17, wjmaclean wrote: > Post(RENDERER, > PpapiHostMsg_Graphics2D_SetLayerTransform(scale, *origin, *translate)); Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:124: float Scale, On 2016/04/14 21:45:00, bokan wrote: > Line up indent with the first parameter. Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:124: float Scale, On 2016/04/14 21:45:00, bokan wrote: > Line up indent with the first parameter. Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:140: void ExecuteTransform(gfx::Transform transform); On 2016/04/14 21:45:00, bokan wrote: > pass by const reference (i.e. const gfx::Transform& transform) Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:140: void ExecuteTransform(gfx::Transform transform); On 2016/04/14 21:45:00, bokan wrote: > pass by const reference (i.e. const gfx::Transform& transform) Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.cc:1550: DCHECK(transform.IsScaleOrTranslation()); On 2016/04/14 21:45:01, bokan wrote: > Any reason this can't be an arbitrary transform? This API could be used outside > of the PDF pinch zoom case. Acknowledged. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_plugin_instance_impl.h (right): https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); On 2016/04/14 21:45:01, bokan wrote: > pass by const reference. Done. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); On 2016/04/14 23:11:35, piman wrote: > nit: this only applies to Graphics2D, maybe this should be explicit in the name, > e.g. SetGraphics2DTransform ? Acknowledged. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); On 2016/04/14 23:11:35, piman wrote: > nit: this only applies to Graphics2D, maybe this should be explicit in the name, > e.g. SetGraphics2DTransform ? Acknowledged. https://codereview.chromium.org/1881603002/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_plugin_instance_impl.h:375: void SetLayerTransform(gfx::Transform transform); On 2016/04/14 21:45:01, bokan wrote: > pass by const reference. Done. https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/40001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:284: [version=1.1] On 2016/04/14 23:11:35, piman wrote: > We need to rev the version (see at the top of the file - 1.2?), and add comments > :) I was suspecting this :) https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... File ppapi/thunk/ppb_graphics_2d_api.h (right): https://codereview.chromium.org/1881603002/diff/40001/ppapi/thunk/ppb_graphic... ppapi/thunk/ppb_graphics_2d_api.h:40: const PP_Point* srigin, On 2016/04/14 23:11:35, piman wrote: > typo: srigin->origin Acknowledged. https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... File content/renderer/pepper/pepper_graphics_2d_host.h (right): https://codereview.chromium.org/1881603002/diff/80001/content/renderer/pepper... content/renderer/pepper/pepper_graphics_2d_host.h:90: void SetLayerTransform(float Scale, const PP_Point& Transform); On 2016/04/15 19:24:39, piman wrote: > nit: lowercase |scale|, |transform| per style. Done. https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:15: M27 = 1.1 On 2016/04/15 19:24:39, piman wrote: > I believe you need to add a version here, M52 I believe. Done. https://codereview.chromium.org/1881603002/diff/80001/ppapi/api/ppb_graphics_... ppapi/api/ppb_graphics_2d.idl:295: * @return Returns <code>true</code> on success or <code>false</code> On 2016/04/15 19:24:39, piman wrote: > nit: PP_TRUE, PP_FALSE instead of true, false. Done. https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... File ppapi/api/ppb_graphics_2d.idl (right): https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... ppapi/api/ppb_graphics_2d.idl:75: * M resource is invalid. The output parameters will be set to 0 on a On 2016/04/15 20:12:28, piman wrote: > bad copy & paste? Done. https://codereview.chromium.org/1881603002/diff/100001/ppapi/api/ppb_graphics... ppapi/api/ppb_graphics_2d.idl:131: M On 2016/04/15 20:12:28, piman wrote: > same? Done.
The CQ bit was checked by alessandroa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, raymes@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1881603002/#ps180001 (title: "Added Graphics2D 1.2 to histograms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881603002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881603002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alessandroa@google.com changed reviewers: + asvitkine@chromium.org
lgtm
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true ( ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ) ==========
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true ( ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ) ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true Reason: ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ==========
The CQ bit was checked by alessandroa@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881603002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881603002/180001
Message was sent while issue was closed.
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true Reason: ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true Reason: ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true Reason: ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h ========== to ========== Added SetLayerTransform to PPAPI. SetLayerTransform() sets a transformation factor that will be applied for the current layer displayed on the output device. This function has no effect until you call Flush(). @param[in] scale The scale to be applied. @param[in] origin The origin of the scale. @param[in] translate The translation to be applied. @return Returns true on success or false if the resource is invalid or the scale factor is 0 or less. BUG=605379 NOPRESUBMIT=true Reason: ** Presubmit ERRORS ** TODOs found in stable public PPAPI files: ppapi/cpp/graphics_2d.h Committed: https://crrev.com/d55efd0d4b7573d763dd6e4d1d9ccfe4ca5be2f7 Cr-Commit-Position: refs/heads/master@{#388851} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d55efd0d4b7573d763dd6e4d1d9ccfe4ca5be2f7 Cr-Commit-Position: refs/heads/master@{#388851} |
