Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928413002:
Add RectF and PointF typemaps in blink. (Closed)
|
Created:
3 years, 6 months ago by mthiesse Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, chromium-reviews, darin (slow to review), haraken, kinuko+watch, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd RectF and PointF typemaps in blink.
Maps gfx::PointF and gfx::RectF to blink::WebFloatSize and blink::WebFloatPoint.
BUG=732403
Review-Url: https://codereview.chromium.org/2928413002
Cr-Commit-Position: refs/heads/master@{#479473}
Committed: https://chromium.googlesource.com/chromium/src/+/d1c41b9314ceaa09baec93237c10eff69c346f5a
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : Use Web* types #Patch Set 4 : address comment #
Messages
Total messages: 40 (23 generated)
mthiesse@chromium.org changed reviewers: + jam@chromium.org, mkwst@chromium.org, vollick@chromium.org
jam@ please review PRESUBMIT.py mkwst@ please review Source/modules/ and Source/platform/ +vollick for thoughts about using gfx geometry types in blink code, feel free to add others who may have opinions.
The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+cc dcheng, https://chromium-review.googlesource.com/c/524474/ depends on this change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Use gfx types for geometry typemaps in blink. Replaces blink::WebSize with gfx::Size and adds missing gfx::RectF typemap. BUG=732403 ========== to ========== Use gfx types for geometry typemaps in blink. Replaces blink::WebSize with gfx::Size and adds missing gfx::RectF typemap. BUG=732403 ==========
vollick@chromium.org changed reviewers: + danakj@chromium.org - vollick@chromium.org
On 2017/06/12 15:00:28, mthiesse wrote: > +cc dcheng, https://chromium-review.googlesource.com/c/524474/ depends on this > change. I think danakj's the right person to comment on the use of these gfx types in blink.
The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, would like to see us replace IntSize, etc with gfx types eventually. And definitely get rid of WebSize and friends.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo bits LGTM.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Given the comments in the other patch (with presubmit warning about gfx::), shouldn't this CL have mapped it to WebFloatRect rather than RectF?
On 2017/06/13 09:18:12, dcheng wrote: > Given the comments in the other patch (with presubmit warning about gfx::), > shouldn't this CL have mapped it to WebFloatRect rather than RectF? Oh, I see this CL explicitly allows them in the PRESUBMIT. I think we should probably have a plan to uniformly use one (or the other) before doing this.
On 2017/06/13 09:19:26, dcheng wrote: > On 2017/06/13 09:18:12, dcheng wrote: > > Given the comments in the other patch (with presubmit warning about gfx::), > > shouldn't this CL have mapped it to WebFloatRect rather than RectF? > > Oh, I see this CL explicitly allows them in the PRESUBMIT. > > I think we should probably have a plan to uniformly use one (or the other) > before doing this. As Dana mentions, I think the plan is to move everything to gfx types eventually. I would really hate to add more dependencies on WebSize/WebFloatRect types, as these are pretty nasty. I think since we can't use IntSize/WebFloatRect directly anyways, it's basically equivalent to create a FloatRect from either a gfx::RectF or a WebFloatRect. We could even add a FloatRect(const gfx::RectF& rect) constructor if we wanted to.
On 2017/06/13 14:26:15, mthiesse wrote: > On 2017/06/13 09:19:26, dcheng wrote: > > On 2017/06/13 09:18:12, dcheng wrote: > > > Given the comments in the other patch (with presubmit warning about gfx::), > > > shouldn't this CL have mapped it to WebFloatRect rather than RectF? > > > > Oh, I see this CL explicitly allows them in the PRESUBMIT. > > > > I think we should probably have a plan to uniformly use one (or the other) > > before doing this. > > As Dana mentions, I think the plan is to move everything to gfx types > eventually. I would really hate to add more dependencies on WebSize/WebFloatRect > types, as these are pretty nasty. > To be clear, I think we should be using gfx geometry types everywhere. But as far as I know, this plan hasn't actually been written down somewhere? There's been some opposition to using non-Blink types in Blink previously, and it'd be good to make sure everyone's on board with this. Again, I don't see any obvious issues (especially since ui/gfx/geometry has no DEPS), but I think the most expedient path to landing the VR patch is to just use the current Blink convention (i.e. use the unfortunate Web* types) and have the discussion in parallel. > I think since we can't use IntSize/WebFloatRect directly anyways, it's basically > equivalent to create a FloatRect from either a gfx::RectF or a WebFloatRect. We > could even add a FloatRect(const gfx::RectF& rect) constructor if we wanted to. I don't think I understand the problem--WebFloatRect should Just Work?
On 2017/06/12 14:56:26, mthiesse wrote: > jam@ please review PRESUBMIT.py > mkwst@ please review Source/modules/ and Source/platform/ > > +vollick for thoughts about using gfx geometry types in blink code, feel free to > add others who may have opinions. I'm not an owner in webkit, please pick someone from third_party/webkit/owners
Description was changed from ========== Use gfx types for geometry typemaps in blink. Replaces blink::WebSize with gfx::Size and adds missing gfx::RectF typemap. BUG=732403 ========== to ========== Add RectF and PointF typemaps in blink. Maps gfx::PointF and gfx::RectF to blink::WebFloatSize and blink::WebFloatPoint. BUG=732403 ==========
mthiesse@chromium.org changed reviewers: - jam@chromium.org
I feel like I need to take a shower. Anyways, updated to use the Web* types. No longer need presubmit review, so -jam. dcheng PTAL
The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't think it's immediately problematic in this CL, but eventually, we are going to have to resolve some validation conflicts around constraints of valid ranges of widths/heights (gfx::Size doesn't allow negative widths/heights, while the Blink version does) Perhaps... we should DCHECK that widths/heights are non-negative in this CL (and check in the deserialize) routines? It'll be easier to loosen the restrictions than to tighten them later.
On 2017/06/13 20:52:03, dcheng wrote: > I don't think it's immediately problematic in this CL, but eventually, we are > going to have to resolve some validation conflicts around constraints of valid > ranges of widths/heights (gfx::Size doesn't allow negative widths/heights, while > the Blink version does) > > Perhaps... we should DCHECK that widths/heights are non-negative in this CL (and > check in the deserialize) routines? It'll be easier to loosen the restrictions > than to tighten them later. It seems the current practice is to just return false? I added a return false for negative width/height. When you said "and check in the deserialize", where you suggesting adding CHECKs somewhere? I'm not sure where that would be (new to this area of the codebase).
On 2017/06/14 14:26:11, mthiesse wrote: > On 2017/06/13 20:52:03, dcheng wrote: > > I don't think it's immediately problematic in this CL, but eventually, we are > > going to have to resolve some validation conflicts around constraints of valid > > ranges of widths/heights (gfx::Size doesn't allow negative widths/heights, > while > > the Blink version does) > > > > Perhaps... we should DCHECK that widths/heights are non-negative in this CL > (and > > check in the deserialize) routines? It'll be easier to loosen the restrictions > > than to tighten them later. > > It seems the current practice is to just return false? I added a return false > for negative width/height. > > When you said "and check in the deserialize", where you suggesting adding CHECKs > somewhere? I'm not sure where that would be (new to this area of the codebase). Yes, sorry, I should have been clearer: i meant that it should return false. (We can't CHECK in deserialization, as we don't want to arbitrarily crash the receiving process because of bad data)
also lgtm. sorry!
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2928413002/#ps60001 (title: "address comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497462581391660,
"parent_rev": "4d3b5c089c535512ac4c9f4b79f95f7f216454d3", "commit_rev":
"f2f30ac8a5e182d13952bb03a2e798aede2a0579"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497462581391660,
"parent_rev": "98f3a3fb7abb2b0ffc31a065e064fd6ed9ee439d", "commit_rev":
"d1c41b9314ceaa09baec93237c10eff69c346f5a"}
Message was sent while issue was closed.
Description was changed from ========== Add RectF and PointF typemaps in blink. Maps gfx::PointF and gfx::RectF to blink::WebFloatSize and blink::WebFloatPoint. BUG=732403 ========== to ========== Add RectF and PointF typemaps in blink. Maps gfx::PointF and gfx::RectF to blink::WebFloatSize and blink::WebFloatPoint. BUG=732403 Review-Url: https://codereview.chromium.org/2928413002 Cr-Commit-Position: refs/heads/master@{#479473} Committed: https://chromium.googlesource.com/chromium/src/+/d1c41b9314ceaa09baec93237c10... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d1c41b9314ceaa09baec93237c10... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
