Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994683004:
Add web platform pointerevents tests to Layouttest (Closed)
|
Created:
4 years, 7 months ago by Navid Zolghadr Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd web platform pointerevents tests to Layouttest
Adding two of the tests and using eventsender and
gpu benchmarking to give inputs automatically to
them.
BUG=612924
Committed: https://crrev.com/c632c23f1b36f09e737d67e23a1c37fcd98836ab
Cr-Commit-Position: refs/heads/master@{#398672}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Enable GPU benchmarking #Patch Set 4 : Fix webexposed global interface test #
Total comments: 1
Patch Set 5 : Skip touch test on Mac #
Total comments: 2
Patch Set 6 : Renaming the test folder #Patch Set 7 : Renaming input method #Patch Set 8 : Remove the -manual suffix #Patch Set 9 : Separate input methods from actual tests #Patch Set 10 : #
Total comments: 5
Patch Set 11 : #
Total comments: 2
Messages
Total messages: 45 (10 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org, tdresser@chromium.org
This is the first CL to use gpu benchmarking and event sender to send events to web platform tests for pointer events. I was trying to use wrapper iframes but I couldn't get a reliable way of inner iframe passing the tests. So I just minimally changed the actual test by adding the input() method. Let me know what you guys think. Regarding the gpu benchmarking, do I need to create another virtual suite to add --enable-gpu-benchmarking to the flags or is that on by default on the bots?
I think we'll want to add --enable-gpu-benchmarking to the default bot flags. It should have zero overhead unless it's being used. Do we want some kind of naming scheme or something to indicate that this is a manual test that's had JS input automation tacked on? https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html (right): https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:14: eventSender.mouseMoveTo(200, 220); You don't need the gpuBenchmarking extension to use eventSender. Once Lan has landed the pointer movement methods to the gpuBenchmarking extension, it would be nice if we used those here.
Do you know who I need to talk to about adding that flag? Also the bots are all seem to be failing as the error is very very clear to me. error: third_party/WebKit/LayoutTests/imported/web-platform-tests/pointerevents/pointerevent_styles.css: does not exist in index https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html (right): https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:14: eventSender.mouseMoveTo(200, 220); On 2016/05/19 14:40:03, tdresser wrote: > You don't need the gpuBenchmarking extension to use eventSender. > > Once Lan has landed the pointer movement methods to the gpuBenchmarking > extension, it would be nice if we used those here. Oops. Yup. I forgot to remove it when copy pasting from the other input. Sure. I believe we switch over as soon as the methods are available.
On 2016/05/19 14:44:23, Navid Zolghadr wrote: > Do you know who I need to talk to about adding that flag? > > Also the bots are all seem to be failing as the error is very very clear to me. > error: > third_party/WebKit/LayoutTests/imported/web-platform-tests/pointerevents/pointerevent_styles.css: > does not exist in index > > https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html > (right): > > https://codereview.chromium.org/1994683004/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:14: > eventSender.mouseMoveTo(200, 220); > On 2016/05/19 14:40:03, tdresser wrote: > > You don't need the gpuBenchmarking extension to use eventSender. > > > > Once Lan has landed the pointer movement methods to the gpuBenchmarking > > extension, it would be nice if we used those here. > > Oops. Yup. I forgot to remove it when copy pasting from the other input. > > Sure. I believe we switch over as soon as the methods are available. Based on a git blame, dpranke@ might know where to add the flag. https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebK...
https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... content/public/common/content_switches.h:116: CONTENT_EXPORT extern const char kEnableGpuBenchmarking[]; I think if we are doing this; we really should rename this flag. GPU benchmarking is conflated with what we want. It also records a bunch of statistics in the compositor but we really only want input injection.
On 2016/05/20 13:28:11, dtapuska wrote: > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > File content/public/common/content_switches.h (right): > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > content/public/common/content_switches.h:116: CONTENT_EXPORT extern const char > kEnableGpuBenchmarking[]; > I think if we are doing this; we really should rename this flag. > > GPU benchmarking is conflated with what we want. It also records a bunch of > statistics in the compositor but we really only want input injection. Do you want me to change the flag name or the variable name? What do you suggest for the name?
On 2016/05/20 14:16:44, Navid Zolghadr wrote: > On 2016/05/20 13:28:11, dtapuska wrote: > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > File content/public/common/content_switches.h (right): > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > content/public/common/content_switches.h:116: CONTENT_EXPORT extern const char > > kEnableGpuBenchmarking[]; > > I think if we are doing this; we really should rename this flag. > > > > GPU benchmarking is conflated with what we want. It also records a bunch of > > statistics in the compositor but we really only want input injection. > > Do you want me to change the flag name or the variable name? What do you suggest > for the name? Ideally we would add a separate flag to control the input synthesis, which is implied by the gpu-benchmarking-extension flag. Something like --enable-synthetic-input.
Description was changed from ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 ========== to ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 ==========
nzolghadr@chromium.org changed reviewers: + lanwei@chromium.org
On 2016/05/20 14:27:16, tdresser wrote: > On 2016/05/20 14:16:44, Navid Zolghadr wrote: > > On 2016/05/20 13:28:11, dtapuska wrote: > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > File content/public/common/content_switches.h (right): > > > > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > content/public/common/content_switches.h:116: CONTENT_EXPORT extern const > char > > > kEnableGpuBenchmarking[]; > > > I think if we are doing this; we really should rename this flag. > > > > > > GPU benchmarking is conflated with what we want. It also records a bunch of > > > statistics in the compositor but we really only want input injection. > > > > Do you want me to change the flag name or the variable name? What do you > suggest > > for the name? > > Ideally we would add a separate flag to control the input synthesis, which is > implied by the gpu-benchmarking-extension flag. > Something like --enable-synthetic-input. So I got rid of the duplication and now using the same switch in cc rather than defining in the public at all. Is that okay Tim for now? Also regarding the naming what do you suggest? All these tests ends with _manual and live in wpt folder. So in that sense the naming are a bit different that the other tests. Do you me to replace _manual with like a _input_automation instead? Although other layout tests all having the synthetic input as well to some extent. Right?
On 2016/05/20 19:40:18, Navid Zolghadr wrote: > On 2016/05/20 14:27:16, tdresser wrote: > > On 2016/05/20 14:16:44, Navid Zolghadr wrote: > > > On 2016/05/20 13:28:11, dtapuska wrote: > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > > File content/public/common/content_switches.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > > content/public/common/content_switches.h:116: CONTENT_EXPORT extern const > > char > > > > kEnableGpuBenchmarking[]; > > > > I think if we are doing this; we really should rename this flag. > > > > > > > > GPU benchmarking is conflated with what we want. It also records a bunch > of > > > > statistics in the compositor but we really only want input injection. > > > > > > Do you want me to change the flag name or the variable name? What do you > > suggest > > > for the name? > > > > Ideally we would add a separate flag to control the input synthesis, which is > > implied by the gpu-benchmarking-extension flag. > > Something like --enable-synthetic-input. > > So I got rid of the duplication and now using the same switch in cc rather than > defining in the public at all. > Is that okay Tim for now? > Also regarding the naming what do you suggest? All these tests ends with _manual > and live in wpt folder. So in that sense the naming are a bit different that the > other tests. Do you me to replace _manual with like a _input_automation instead? > Although other layout tests all having the synthetic input as well to some > extent. Right? I agree _manual doesn't make sense anymore, but renaming would make it harder to link the tests back to the original web platform tests. E.g. we can't easily find for tests that are duplicated between imported/wpt/ vs fast/events/pointerevents/wpt/. I suggest putting automated tests in a dedicated folder instead, like wpt_automated. Thoughts? This could even be imported/wpt_automated/ instead of inside fast/events/. The tests in imported/wpt are maintained "manually" anyway (i.e. they are not mass imported w/o manual attention). Btw, why did we choose to put them in
On 2016/05/20 20:46:48, mustaq wrote: > On 2016/05/20 19:40:18, Navid Zolghadr wrote: > > On 2016/05/20 14:27:16, tdresser wrote: > > > On 2016/05/20 14:16:44, Navid Zolghadr wrote: > > > > On 2016/05/20 13:28:11, dtapuska wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > > > File content/public/common/content_switches.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/50001/content/public/common/c... > > > > > content/public/common/content_switches.h:116: CONTENT_EXPORT extern > const > > > char > > > > > kEnableGpuBenchmarking[]; > > > > > I think if we are doing this; we really should rename this flag. > > > > > > > > > > GPU benchmarking is conflated with what we want. It also records a bunch > > of > > > > > statistics in the compositor but we really only want input injection. > > > > > > > > Do you want me to change the flag name or the variable name? What do you > > > suggest > > > > for the name? > > > > > > Ideally we would add a separate flag to control the input synthesis, which > is > > > implied by the gpu-benchmarking-extension flag. > > > Something like --enable-synthetic-input. > > > > So I got rid of the duplication and now using the same switch in cc rather > than > > defining in the public at all. > > Is that okay Tim for now? > > Also regarding the naming what do you suggest? All these tests ends with > _manual > > and live in wpt folder. So in that sense the naming are a bit different that > the > > other tests. Do you me to replace _manual with like a _input_automation > instead? > > Although other layout tests all having the synthetic input as well to some > > extent. Right? > > I agree _manual doesn't make sense anymore, but renaming would make it harder to > link the tests back to the original web platform tests. E.g. we can't easily > find > for tests that are duplicated between imported/wpt/ vs > fast/events/pointerevents/wpt/. I suggest putting automated > tests in a dedicated folder instead, like wpt_automated. Thoughts? > > This could even be imported/wpt_automated/ instead of inside fast/events/. The > tests > in imported/wpt are maintained "manually" anyway (i.e. they are not mass > imported > w/o manual attention). > > > > Btw, why did we choose to put them in Sure I can change the name of the folder. The only reason was that there is more change inside each file than just the testharness path change so I thought it is better not to put it in imported folder. Also it was easier to run them in the virtual suite tests rather than putting it in the root of imported tests. What do you think?
ptal.
LGTM. https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html (right): https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: <body onload="run(); input();"> Nit: inject_input() or simulate_input()?
https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html (right): https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: <body onload="run(); input();"> On 2016/05/24 19:40:55, mustaq wrote: > Nit: inject_input() or simulate_input()? Done.
On 2016/05/24 19:50:33, Navid Zolghadr wrote: > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html > (right): > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: > <body onload="run(); input();"> > On 2016/05/24 19:40:55, mustaq wrote: > > Nit: inject_input() or simulate_input()? > > Done. The manual.XXX suffix has implications for some tooling related to web platform tests. I'm thinking it is probably best if we don't name these manual even though they were adapted from the manual tests? https://github.com/w3c/web-platform-tests/blob/master/docs/manual-test.md
On 2016/05/24 22:19:24, dtapuska wrote: > On 2016/05/24 19:50:33, Navid Zolghadr wrote: > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html > > (right): > > > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: > > <body onload="run(); input();"> > > On 2016/05/24 19:40:55, mustaq wrote: > > > Nit: inject_input() or simulate_input()? > > > > Done. > > The manual.XXX suffix has implications for some tooling related to web platform > tests. I'm thinking it is probably best if we don't name these manual even > though they were adapted from the manual tests? > > https://github.com/w3c/web-platform-tests/blob/master/docs/manual-test.md How about this? I removed the "-manual" from the name of the files. I'm also going to add _touch/_mouse/_pen to their name when adding the input events. Although in the case of these two tests in this CL they already did have that as they were specific to those input methods.
On 2016/05/25 14:38:33, Navid Zolghadr wrote: > On 2016/05/24 22:19:24, dtapuska wrote: > > On 2016/05/24 19:50:33, Navid Zolghadr wrote: > > > > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > > File > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: > > > <body onload="run(); input();"> > > > On 2016/05/24 19:40:55, mustaq wrote: > > > > Nit: inject_input() or simulate_input()? > > > > > > Done. > > > > The manual.XXX suffix has implications for some tooling related to web > platform > > tests. I'm thinking it is probably best if we don't name these manual even > > though they were adapted from the manual tests? > > > > https://github.com/w3c/web-platform-tests/blob/master/docs/manual-test.md > > How about this? I removed the "-manual" from the name of the files. I'm also > going to add _touch/_mouse/_pen to their name when adding the input events. > Although in the case of these two tests in this CL they already did have that as > they were specific to those input methods. Renaming tests makes it terribly hard to sync with w3c IMO. How will we ever detect if some tests change/get-deleted in w3c repo in future? Maintaining the filenames is a very low-overhead way to manage this. I had a quick chat with Dave about script injection idea---I believe we all agree that it is the best solution but it will take time to implement. Until it happens, we unfortunately have to accept copying-and-modifying tests from w3c. Let's not lose the link back to the w3c repo at least.
On 2016/05/26 14:33:03, mustaq wrote: > On 2016/05/25 14:38:33, Navid Zolghadr wrote: > > On 2016/05/24 22:19:24, dtapuska wrote: > > > On 2016/05/24 19:50:33, Navid Zolghadr wrote: > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > > > File > > > > > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/70001/third_party/WebKit/Layo... > > > > > > > > > > third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/wpt/pointerevent_button_attribute_mouse-manual.html:18: > > > > <body onload="run(); input();"> > > > > On 2016/05/24 19:40:55, mustaq wrote: > > > > > Nit: inject_input() or simulate_input()? > > > > > > > > Done. > > > > > > The manual.XXX suffix has implications for some tooling related to web > > platform > > > tests. I'm thinking it is probably best if we don't name these manual even > > > though they were adapted from the manual tests? > > > > > > https://github.com/w3c/web-platform-tests/blob/master/docs/manual-test.md > > > > How about this? I removed the "-manual" from the name of the files. I'm also > > going to add _touch/_mouse/_pen to their name when adding the input events. > > Although in the case of these two tests in this CL they already did have that > as > > they were specific to those input methods. > > Renaming tests makes it terribly hard to sync with w3c IMO. How will we ever > detect > if some tests change/get-deleted in w3c repo in future? Maintaining the > filenames > is a very low-overhead way to manage this. > > I had a quick chat with Dave about script injection idea---I believe we all > agree > that it is the best solution but it will take time to implement. Until it > happens, > we unfortunately have to accept copying-and-modifying tests from w3c. Let's not > lose the link back to the w3c repo at least. I will test a few ways we discussed yesterday like import and injection idea to see if I can quickly do them. If so I guess we should be okay.
nzolghadr@chromium.org changed reviewers: + dpranke@chromium.org
ptal. This is the improved way to inject the input. Basically the js files that generate input are all in another folder and we load them if they exist. Note that the plan is to add one single pointer type for now for the tests that can pass with different pointer types.
lgtm https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:157: nit: remove blank line. https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js (right): https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js:2: chrome.gpuBenchmarking.smoothDrag(250, 250, 200, 200, function() {}, 1); It seems a bit odd that this has something to do w/ gpu benchmarking rather than being a generic action?
https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:157: On 2016/06/06 23:27:04, Dirk Pranke wrote: > nit: remove blank line. Done. https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js (right): https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js:2: chrome.gpuBenchmarking.smoothDrag(250, 250, 200, 200, function() {}, 1); On 2016/06/06 23:27:04, Dirk Pranke wrote: > It seems a bit odd that this has something to do w/ gpu benchmarking rather than > being a generic action? I was told that the naming of gpu benchmarking is not a good choice for this extension as it express all the purpose. I believe it will change in the future.
nzolghadr@chromium.org changed reviewers: + creis@chromium.org, rbyers@chromium.org
creis@chromium.org: Please review changes in content/shell/app/shell_main_delegate.cc rbyers@chromium.org: Please review changes in third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt Rick, since internals field was already in this expected file as you suggested I left this Chrome field exposed in there too.
LGTM https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js (right): https://codereview.chromium.org/1994683004/diff/170001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual-input.js:2: chrome.gpuBenchmarking.smoothDrag(250, 250, 200, 200, function() {}, 1); On 2016/06/06 23:27:04, Dirk Pranke wrote: > It seems a bit odd that this has something to do w/ gpu benchmarking rather than > being a generic action? The naming here is definitely unfortunate. In the medium term, we hope to switch to using the chromeDriver extension, which at least be a better name.
content/shell/app/shell_main_delegate.cc LGTM
LGTM! Btw, I think chrome.gpuBenchmarking.smoothDrag(250, 250, 200, 200) also works.
On 2016/06/08 00:34:18, lanwei wrote: > LGTM! Btw, I think chrome.gpuBenchmarking.smoothDrag(250, 250, 200, 200) also > works. I was hoping in does. But the last time I saw the code those parameters were required. Is that not the case anymore?
On 2016/06/07 14:27:44, Navid Zolghadr wrote: > mailto:creis@chromium.org: Please review changes in > > content/shell/app/shell_main_delegate.cc > > > mailto:rbyers@chromium.org: Please review changes in > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt > > Rick, since internals field was already in this expected file as you suggested I > left this Chrome field exposed in there too. Yeah this is a limitation of our webexposed testing. I see you're adding this only when running layout tests, so webexposed LGTM.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1994683004/#ps190001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994683004/190001
Message was sent while issue was closed.
Description was changed from ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 ========== to ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 ========== to ========== Add web platform pointerevents tests to Layouttest Adding two of the tests and using eventsender and gpu benchmarking to give inputs automatically to them. BUG=612924 Committed: https://crrev.com/c632c23f1b36f09e737d67e23a1c37fcd98836ab Cr-Commit-Position: refs/heads/master@{#398672} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c632c23f1b36f09e737d67e23a1c37fcd98836ab Cr-Commit-Position: refs/heads/master@{#398672}
Message was sent while issue was closed.
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt (right): https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: attribute chrome do we really need to expose this in stable? not familiar with the usage, but looks like 'window.chrome' is used in tests without it being exposed here?
Message was sent while issue was closed.
https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt (right): https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: attribute chrome On 2016/06/09 11:25:39, timvolodine wrote: > do we really need to expose this in stable? not familiar with the usage, but > looks like 'window.chrome' is used in tests without it being exposed here? I agree this isn't ideal - I'm not sure how we'd prevent this though. Any ideas? We want to expose this to all layout tests, except this one.
Message was sent while issue was closed.
On 2016/06/09 11:54:48, tdresser wrote: > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt > (right): > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: > attribute chrome > On 2016/06/09 11:25:39, timvolodine wrote: > > do we really need to expose this in stable? not familiar with the usage, but > > looks like 'window.chrome' is used in tests without it being exposed here? > > I agree this isn't ideal - I'm not sure how we'd prevent this though. Any ideas? > We want to expose this to all layout tests, except this one. Note that this isn't "exposed in stable" - it's only exposed when running layout tests, exactly like window.internals is and our virtual/stable/webexposed testsuite isn't smart enough to know that. We could make the test suite smarter (eg. maybe run "webexposed" in it's own virtual testsuite that disables all test-specific APIs) but I don't think it'd buy us that much in practice. What specifically is the concern here?
Message was sent while issue was closed.
On 2016/06/09 12:39:32, Rick Byers wrote: > On 2016/06/09 11:54:48, tdresser wrote: > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt > > (right): > > > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: > > attribute chrome > > On 2016/06/09 11:25:39, timvolodine wrote: > > > do we really need to expose this in stable? not familiar with the usage, but > > > looks like 'window.chrome' is used in tests without it being exposed here? > > > > I agree this isn't ideal - I'm not sure how we'd prevent this though. Any > ideas? > > We want to expose this to all layout tests, except this one. > > Note that this isn't "exposed in stable" - it's only exposed when running layout > tests, exactly like window.internals is and our virtual/stable/webexposed > testsuite isn't smart enough to know that. We could make the test suite smarter > (eg. maybe run "webexposed" in it's own virtual testsuite that disables all > test-specific APIs) but I don't think it'd buy us that much in practice. What > specifically is the concern here? Right, the reason I am asking is that we have some WebView bots that trigger when something is exposed in 'stable' but not in WebView. In this case it seems to be listed but not really exposed, which is a bit confusing. The comparison is based on virtual/stable/ in the assumption that the attributes/methods there are web-exposed in stable as the name implies. I've added a specific exception for this (like we have for window.internals mentioned by Rick), so it's fine now. However I was wondering if we could just drop the chrome attribute in virtual/stable/.. and only keep it in webexposed/ would that work?
Message was sent while issue was closed.
On 2016/06/09 12:59:33, timvolodine wrote: > On 2016/06/09 12:39:32, Rick Byers wrote: > > On 2016/06/09 11:54:48, tdresser wrote: > > > > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt > > > (right): > > > > > > > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: > > > attribute chrome > > > On 2016/06/09 11:25:39, timvolodine wrote: > > > > do we really need to expose this in stable? not familiar with the usage, > but > > > > looks like 'window.chrome' is used in tests without it being exposed here? > > > > > > I agree this isn't ideal - I'm not sure how we'd prevent this though. Any > > ideas? > > > We want to expose this to all layout tests, except this one. > > > > Note that this isn't "exposed in stable" - it's only exposed when running > layout > > tests, exactly like window.internals is and our virtual/stable/webexposed > > testsuite isn't smart enough to know that. We could make the test suite > smarter > > (eg. maybe run "webexposed" in it's own virtual testsuite that disables all > > test-specific APIs) but I don't think it'd buy us that much in practice. What > > specifically is the concern here? > > Right, the reason I am asking is that we have some WebView bots that trigger > when something is exposed in 'stable' but not in WebView. In this case it seems > to be listed but not really exposed, which is a bit confusing. The comparison is > based on virtual/stable/ in the assumption that the attributes/methods there are > web-exposed in stable as the name implies. I've added a specific exception for > this (like we have for window.internals mentioned by Rick), so it's fine now. > However I was wondering if we could just drop the chrome attribute in > virtual/stable/.. and only keep it in webexposed/ would that work? Ah sorry ignore my question, I was reading the other way around apparently. So yes the point is that it's slightly confusing maybe some kind of annotation would help, but it's non-essential :)
Message was sent while issue was closed.
On 2016/06/09 13:13:50, timvolodine wrote: > On 2016/06/09 12:59:33, timvolodine wrote: > > On 2016/06/09 12:39:32, Rick Byers wrote: > > > On 2016/06/09 11:54:48, tdresser wrote: > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > > > File > > > > > > > > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1994683004/diff/190001/third_party/WebKit/Lay... > > > > > > > > > > third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:6462: > > > > attribute chrome > > > > On 2016/06/09 11:25:39, timvolodine wrote: > > > > > do we really need to expose this in stable? not familiar with the usage, > > but > > > > > looks like 'window.chrome' is used in tests without it being exposed > here? > > > > > > > > I agree this isn't ideal - I'm not sure how we'd prevent this though. Any > > > ideas? > > > > We want to expose this to all layout tests, except this one. > > > > > > Note that this isn't "exposed in stable" - it's only exposed when running > > layout > > > tests, exactly like window.internals is and our virtual/stable/webexposed > > > testsuite isn't smart enough to know that. We could make the test suite > > smarter > > > (eg. maybe run "webexposed" in it's own virtual testsuite that disables all > > > test-specific APIs) but I don't think it'd buy us that much in practice. > What > > > specifically is the concern here? > > > > Right, the reason I am asking is that we have some WebView bots that trigger > > when something is exposed in 'stable' but not in WebView. In this case it > seems > > to be listed but not really exposed, which is a bit confusing. The comparison > is > > based on virtual/stable/ in the assumption that the attributes/methods there > are > > web-exposed in stable as the name implies. I've added a specific exception for > > this (like we have for window.internals mentioned by Rick), so it's fine now. > > However I was wondering if we could just drop the chrome attribute in > > virtual/stable/.. and only keep it in webexposed/ would that work? > > Ah sorry ignore my question, I was reading the other way around apparently. > So yes the point is that it's slightly confusing maybe some kind of annotation > would help, but it's non-essential :) Yeah sorry for the confusion - this should be a one-off (and I should have thought to ask mustaq to update the webview results at the same time and/or ping you). Still I've filed https://crbug.com/618689 to track / discuss further. |
