Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149053002:
Blink WebTouchPoint stores its location as WebFloatPoint, instead of WebPoint. (Closed)
|
Created:
6 years, 10 months ago by tdresser Modified:
6 years, 9 months ago CC:
blink-reviews, dglazkov+blink, abarth-chromium, Jói Base URL:
https://chromium.googlesource.com/chromium/blink@master Visibility:
Public. |
DescriptionBlink WebTouchPoint stores its location as WebFloatPoint, instead of WebPoint.
This is the second of three patches converting WebTouchPoint to use
WebFloatPoint instead of WebPoint.
The first patch (https://codereview.chromium.org/140973005) allows
chromium to accept WebFloatPoint (it just casts all the floats to
ints).
The third patch (https://codereview.chromium.org/148453012/) will
clean up the chromium side.
BUG=336807
R=jamesr@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168641
Patch Set 1 #Patch Set 2 : Rebase / minor fixes. #
Total comments: 9
Patch Set 3 : Address jamesr's comments. #Patch Set 4 : Add comment explaining why ../ is used in DEPS. #Patch Set 5 : Remove layout test. #Patch Set 6 : Relative path in DEPS lives in public/web. #
Total comments: 1
Patch Set 7 : Remove DEPS change. #Patch Set 8 : Rebase #
Messages
Total messages: 24 (0 generated)
jochen@, can you take a look?
https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... File LayoutTests/fast/events/touch/touch-coords-rounded.html (right): https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/touch/touch-coords-rounded.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> say <!DOCTYPE html> if you want to define a modern HTML document. only use other strings if you are specifically testing a legacy document type https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/touch/touch-coords-rounded.html:8: have their locations floored before they are passed to javascript.</p> if we specify these values as type 'long' in the WebIDL, shouldn't the flooring happen in that bindings layer and not in our C++ code? https://codereview.chromium.org/149053002/diff/20001/public/DEPS File public/DEPS (right): https://codereview.chromium.org/149053002/diff/20001/public/DEPS#newcode3 public/DEPS:3: "+../platform" don't do this https://codereview.chromium.org/149053002/diff/20001/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/149053002/diff/20001/public/web/WebTouchPoint... public/web/WebTouchPoint.h:35: #include "../platform/WebFloatPoint.h" to include files from public/platform/ from within public/web/, just say "public/platform/WebFloatPoint.h" etc.
https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... File LayoutTests/fast/events/touch/touch-coords-rounded.html (right): https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/touch/touch-coords-rounded.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2014/02/03 21:48:23, jamesr wrote: > say <!DOCTYPE html> if you want to define a modern HTML document. only use other > strings if you are specifically testing a legacy document type Done. https://codereview.chromium.org/149053002/diff/20001/LayoutTests/fast/events/... LayoutTests/fast/events/touch/touch-coords-rounded.html:8: have their locations floored before they are passed to javascript.</p> On 2014/02/03 21:48:23, jamesr wrote: > if we specify these values as type 'long' in the WebIDL, shouldn't the flooring > happen in that bindings layer and not in our C++ code? That's what's happening, isn't it? I guess I've worded this badly, it should say "as they are passed to javascript". (DONE) I think it's worth a test to ensure the truncation behaves as we expect it to. https://codereview.chromium.org/149053002/diff/20001/public/DEPS File public/DEPS (right): https://codereview.chromium.org/149053002/diff/20001/public/DEPS#newcode3 public/DEPS:3: "+../platform" On 2014/02/03 21:48:23, jamesr wrote: > don't do this I had a conversation with joi@ about the correct approach here, and this is what he recommended (though he did suggest I could make the rule more specific.) Because this needs to work in both the blink and chromium builds, using an absolute path doesn't work. I agree this seems like a bad plan (there are only 2 other occurrences of using .. in a DEPS file in blink), but I'm not sure what the correct approach is. Any ideas? https://codereview.chromium.org/149053002/diff/20001/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/149053002/diff/20001/public/web/WebTouchPoint... public/web/WebTouchPoint.h:35: #include "../platform/WebFloatPoint.h" On 2014/02/03 21:48:23, jamesr wrote: > to include files from public/platform/ from within public/web/, just say > "public/platform/WebFloatPoint.h" etc. That works fine when building blink level targets, but when building chrome, using a path relative to the blink root doesn't seem to work. (No such file or directory)
https://codereview.chromium.org/149053002/diff/20001/public/DEPS File public/DEPS (right): https://codereview.chromium.org/149053002/diff/20001/public/DEPS#newcode3 public/DEPS:3: "+../platform" On 2014/02/04 14:46:23, tdresser wrote: > On 2014/02/03 21:48:23, jamesr wrote: > > don't do this > > I had a conversation with joi@ about the correct approach here, and this is what > he recommended (though he did suggest I could make the rule more specific.) > > Because this needs to work in both the blink and chromium builds, using an > absolute path doesn't work. > > I agree this seems like a bad plan (there are only 2 other occurrences of using > .. in a DEPS file in blink), but I'm not sure what the correct approach is. Any > ideas? Yeah, checkdeps works on the include paths as they are actually written, so this is the only approach that will currently work, as far as I know, except perhaps to change the include paths for the projects in question so that they can both include this header via the same directory. That is probably a worse solution and would still leave us with a non-standard include rule in this DEPS file. There should be a comment explaining why .. is used, though.
On 2014/02/04 15:10:15, Jói wrote: > https://codereview.chromium.org/149053002/diff/20001/public/DEPS > File public/DEPS (right): > > https://codereview.chromium.org/149053002/diff/20001/public/DEPS#newcode3 > public/DEPS:3: "+../platform" > On 2014/02/04 14:46:23, tdresser wrote: > > On 2014/02/03 21:48:23, jamesr wrote: > > > don't do this > > > > I had a conversation with joi@ about the correct approach here, and this is > what > > he recommended (though he did suggest I could make the rule more specific.) > > > > Because this needs to work in both the blink and chromium builds, using an > > absolute path doesn't work. > > > > I agree this seems like a bad plan (there are only 2 other occurrences of > using > > .. in a DEPS file in blink), but I'm not sure what the correct approach is. > Any > > ideas? > > Yeah, checkdeps works on the include paths as they are actually written, so this > is the only approach that will currently work, as far as I know, except perhaps > to change the include paths for the projects in question so that they can both > include this header via the same directory. That is probably a worse solution > and would still leave us with a non-standard include rule in this DEPS file. > > There should be a comment explaining why .. is used, though. Done.
On 2014/02/04 20:33:34, tdresser wrote: > On 2014/02/04 15:10:15, Jói wrote: > > https://codereview.chromium.org/149053002/diff/20001/public/DEPS > > File public/DEPS (right): > > > > https://codereview.chromium.org/149053002/diff/20001/public/DEPS#newcode3 > > public/DEPS:3: "+../platform" > > On 2014/02/04 14:46:23, tdresser wrote: > > > On 2014/02/03 21:48:23, jamesr wrote: > > > > don't do this > > > > > > I had a conversation with joi@ about the correct approach here, and this is > > what > > > he recommended (though he did suggest I could make the rule more specific.) > > > > > > Because this needs to work in both the blink and chromium builds, using an > > > absolute path doesn't work. > > > > > > I agree this seems like a bad plan (there are only 2 other occurrences of > > using > > > .. in a DEPS file in blink), but I'm not sure what the correct approach is. > > Any > > > ideas? > > > > Yeah, checkdeps works on the include paths as they are actually written, so > this > > is the only approach that will currently work, as far as I know, except > perhaps > > to change the include paths for the projects in question so that they can both > > include this header via the same directory. That is probably a worse solution > > and would still leave us with a non-standard include rule in this DEPS file. > > > > There should be a comment explaining why .. is used, though. > > Done. jamesr@, does that seem reasonable?
I don't think it's reasonable to have tests for WebIDL hidden in something that looks like a test for an unrelated feature. We should by all means be testing WebIDL's numeric behavior, and I believe we are elsewhere, but if you want to test touch point behavior then have tests for that. The test you're adding doesn't actually check anything interesting related to touch points.
On 2014/02/05 20:37:53, jamesr wrote: > I don't think it's reasonable to have tests for WebIDL hidden in something that > looks like a test for an unrelated feature. We should by all means be testing > WebIDL's numeric behavior, and I believe we are elsewhere, but if you want to > test touch point behavior then have tests for that. The test you're adding > doesn't actually check anything interesting related to touch points. I've removed the layout test. jamesr@, if there's additional testing you think should be done, could you point me to where the tests should live? Otherwise, how does this look?
On 2014/02/06 14:50:46, tdresser wrote: > On 2014/02/05 20:37:53, jamesr wrote: > > I don't think it's reasonable to have tests for WebIDL hidden in something > that > > looks like a test for an unrelated feature. We should by all means be testing > > WebIDL's numeric behavior, and I believe we are elsewhere, but if you want to > > test touch point behavior then have tests for that. The test you're adding > > doesn't actually check anything interesting related to touch points. > > I've removed the layout test. jamesr@, if there's additional testing you think > should be done, could you point me to where the tests should live? > > Otherwise, how does this look? jochen@, can you take a look as well? Thanks!
On 2014/02/07 16:44:04, tdresser wrote: > On 2014/02/06 14:50:46, tdresser wrote: > > On 2014/02/05 20:37:53, jamesr wrote: > > > I don't think it's reasonable to have tests for WebIDL hidden in something > > that > > > looks like a test for an unrelated feature. We should by all means be > testing > > > WebIDL's numeric behavior, and I believe we are elsewhere, but if you want > to > > > test touch point behavior then have tests for that. The test you're adding > > > doesn't actually check anything interesting related to touch points. > > > > I've removed the layout test. jamesr@, if there's additional testing you think > > should be done, could you point me to where the tests should live? > > > > Otherwise, how does this look? > > jochen@, can you take a look as well? > Thanks! What parts do you want me to look at? James seems to have everything covered?
On 2014/02/10 12:41:37, jochen wrote: > On 2014/02/07 16:44:04, tdresser wrote: > > On 2014/02/06 14:50:46, tdresser wrote: > > > On 2014/02/05 20:37:53, jamesr wrote: > > > > I don't think it's reasonable to have tests for WebIDL hidden in something > > > that > > > > looks like a test for an unrelated feature. We should by all means be > > testing > > > > WebIDL's numeric behavior, and I believe we are elsewhere, but if you want > > to > > > > test touch point behavior then have tests for that. The test you're > adding > > > > doesn't actually check anything interesting related to touch points. > > > > > > I've removed the layout test. jamesr@, if there's additional testing you > think > > > should be done, could you point me to where the tests should live? > > > > > > Otherwise, how does this look? > > > > jochen@, can you take a look as well? > > Thanks! > > What parts do you want me to look at? James seems to have everything covered? Sorry jochen@, I wasn't clear on correct etiquette when the original reviewer listed isn't the first to review the patch. I'll remove you from the reviewers list. jamesr@, can you take another look?
I still don't think the DEPS change is correct.
I've moved the DEPS change into public/web - reducing the scope of the relative path seems like a good thing. In the current tree, WebTouchPoint.h doesn't satisfy checkdeps. It already includes "../platform/WebCommon.h" and "../platform/WebFloatPoint.h", which have no associated DEPS rule. As checkdeps only works with relative paths, I think using a relative path in public/web/DEPS is the right path forward. Alternatively, we could leave WebTouchPoint.h in it's current state, where it doesn't satisfy checkdeps. Do either of those solutions work for you? If not, can you give more information on what specifically is wrong with the DEPS rule?
https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoint.h File public/web/WebTouchPoint.h (right): https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoin... public/web/WebTouchPoint.h:35: #include "../platform/WebFloatPoint.h" These should be: #include "public/platform/WebCommon.h" but that involves fixing a bunch of GYP issues. Several people have tried, and none have succeeded. It's ok to continue this pattern for now.
On 2014/02/20 21:05:09, abarth wrote: > https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoint.h > File public/web/WebTouchPoint.h (right): > > https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoin... > public/web/WebTouchPoint.h:35: #include "../platform/WebFloatPoint.h" > These should be: > > #include "public/platform/WebCommon.h" > > but that involves fixing a bunch of GYP issues. Several people have tried, and > none have succeeded. It's ok to continue this pattern for now. jamesr@, sounds like this is the best approach to the GYP issue. Can you take another look?
On 2014/02/24 17:00:57, tdresser wrote: > On 2014/02/20 21:05:09, abarth wrote: > > > https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoint.h > > File public/web/WebTouchPoint.h (right): > > > > > https://codereview.chromium.org/149053002/diff/370001/public/web/WebTouchPoin... > > public/web/WebTouchPoint.h:35: #include "../platform/WebFloatPoint.h" > > These should be: > > > > #include "public/platform/WebCommon.h" > > > > but that involves fixing a bunch of GYP issues. Several people have tried, > and > > none have succeeded. It's ok to continue this pattern for now. > > jamesr@, sounds like this is the best approach to the GYP issue. > Can you take another look? To continue the pattern, land this without the DEPS change. If you want to fix it then write the #include out as "public/platform/..." and then see what fails to compile in chromium, then fix the gyp files. Still not lgtm on the DEPS change.
DEPS change removed.
On 2014/02/27 16:18:13, tdresser wrote: > DEPS change removed. jamesr, can you take another look?
lgtm
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/149053002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...
Message was sent while issue was closed.
Committed patchset #8 manually as r168641. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
