Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18478003:
Vibration cannot be canceled during pattern vibration. (Closed)
|
Created:
7 years, 5 months ago by kihong Modified:
7 years, 2 months ago Reviewers:
Nils Barth (inactive), Peter Beverloo, abarth-chromium, tkent, Michael van Ouwerkerk, haraken, do-not-use CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, do-not-use Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionVibration cannot be canceled during pattern vibration.
Vibration could not be canceled during pattern vibration is working.
In the resting time which are even numbers of pattern,
m_isVibrating will be false and cancel will thus return early.
WebKit bug : https://bugs.webkit.org/show_bug.cgi?id=117822
BUG=222504, 261467, 310137
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160289
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160407
Patch Set 1 #
Total comments: 11
Patch Set 2 : Vibration cannot be canceled during pattern vibration. #
Total comments: 9
Patch Set 3 : Vibration cannot be canceled during pattern vibration. #Patch Set 4 : Adoptation for recently changing of NavigatorVibration. #
Total comments: 1
Patch Set 5 : Vibration cannot be canceled during pattern vibration. #
Total comments: 10
Patch Set 6 : Vibration cannot be canceled during pattern vibration. #
Total comments: 12
Patch Set 7 : Vibration cannot be canceled during pattern vibration. #Patch Set 8 : Vibration cannot be canceled during pattern vibration. #Patch Set 9 : Add FIXME: to the web_tests.py #Patch Set 10 : Upload with --no-find-copies #Patch Set 11 : patch #
Total comments: 5
Patch Set 12 : patch #Patch Set 13 : patch #Patch Set 14 : patch #Patch Set 15 : patch #Patch Set 16 : Patch for Relanding #Messages
Total messages: 54 (0 generated)
https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... File LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html (right): https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:14: navigator.vibrate([100, 100, 100, 100, 100]); I find this test really hard to read, with a counter and calculated timeouts based on that counter. Also it lacks comments and does not test whether isVibrating becomes false when the pattern ends. Please perform separate simpler tests, and explicitly comment on the purpose of the separate tests: // Test that cancelling works during a vibration in a pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); // Progress time by 50ms so we are in the middle of the first vibration of the pattern. shouldBeTrue('internals.isVibrating()'); navigator.vibrate(0); shouldBeFalse('internals.isVibrating()'); // Test that cancelling works during a pause in a pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); // Progress time by 150ms so we are in the middle of the first pause of the pattern. shouldBeTrue('internals.isVibrating()'); navigator.vibrate(0); shouldBeFalse('internals.isVibrating()'); // Test that the system stops thinking that it is vibrating when the pattern ends. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); shouldBeTrue('internals.isVibrating()'); // Progress time by 350ms so the pattern ends naturally without interruptions. shouldBeFalse('internals.isVibrating()'); // Test that a trailing pause is stripped so isVibrating becomes false after the last vibration in the pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100, 100]); // Even number of array entries, the trailing pause will be stripped. shouldBeTrue('internals.isVibrating()'); // Progress time by 350ms so the pattern ends naturally without interruptions. shouldBeFalse('internals.isVibrating()'); https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:20: setTimeout(function() { Tests should not wait for timeouts, they will run too long. There should be some code in the animation tests that handles this better. https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:32: } else Please use curly braces for the else clause, because the if clause also has them. https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... File Source/modules/vibration/NavigatorVibration.cpp (left): https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... Source/modules/vibration/NavigatorVibration.cpp:133: m_isVibrating = false; I think you want to keep this line, but make it conditional on the pattern being empty: if (m_pattern.isEmpty()) This way it stays true even during pauses in the pattern, and becomes false when the pattern ends. https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... Source/modules/vibration/NavigatorVibration.cpp:137: if (m_pattern.isEmpty()) How would this ever happen? It should not, since trailing pauses are removed from the pattern on line 69: tempPattern.removeLast(); This line represents step 4 in the algorithm described in the specification: http://dev.w3.org/2009/dap/vibration/#vibration-interface "If the length of pattern is even and is not zero, then remove the last entry in pattern." I think this condition never becomes true, so m_isVibrating never becomes false unless someone calls navigator.vibrate(0) from JavaScript.
https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... File LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html (right): https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:14: navigator.vibrate([100, 100, 100, 100, 100]); On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > I find this test really hard to read, with a counter and calculated timeouts > based on that counter. Also it lacks comments and does not test whether > isVibrating becomes false when the pattern ends. Please perform separate simpler > tests, and explicitly comment on the purpose of the separate tests: > > // Test that cancelling works during a vibration in a pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > // Progress time by 50ms so we are in the middle of the first vibration of the > pattern. > shouldBeTrue('internals.isVibrating()'); > navigator.vibrate(0); > shouldBeFalse('internals.isVibrating()'); > > // Test that cancelling works during a pause in a pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > // Progress time by 150ms so we are in the middle of the first pause of the > pattern. > shouldBeTrue('internals.isVibrating()'); > navigator.vibrate(0); > shouldBeFalse('internals.isVibrating()'); > > // Test that the system stops thinking that it is vibrating when the pattern > ends. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > shouldBeTrue('internals.isVibrating()'); > // Progress time by 350ms so the pattern ends naturally without interruptions. > shouldBeFalse('internals.isVibrating()'); > > // Test that a trailing pause is stripped so isVibrating becomes false after the > last vibration in the pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100, 100]); // Even number of array entries, the > trailing pause will be stripped. > shouldBeTrue('internals.isVibrating()'); > // Progress time by 350ms so the pattern ends naturally without interruptions. > shouldBeFalse('internals.isVibrating()'); > Thanks for your comments. I'm changing my tests more simple. https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:20: setTimeout(function() { On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > Tests should not wait for timeouts, they will run too long. There should be some > code in the animation tests that handles this better. I changed my tests but I don't understand what you said exactly, is there a way to wait without setTimeout()? https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:32: } else On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > Please use curly braces for the else clause, because the if clause also has > them. Done. https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... File Source/modules/vibration/NavigatorVibration.cpp (left): https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... Source/modules/vibration/NavigatorVibration.cpp:133: m_isVibrating = false; On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > I think you want to keep this line, but make it conditional on the pattern being > empty: if (m_pattern.isEmpty()) > > This way it stays true even during pauses in the pattern, and becomes false when > the pattern ends. Done. https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/1/Source/modules/vibration/Navi... Source/modules/vibration/NavigatorVibration.cpp:137: if (m_pattern.isEmpty()) On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > How would this ever happen? It should not, since trailing pauses are removed > from the pattern on line 69: tempPattern.removeLast(); > > This line represents step 4 in the algorithm described in the specification: > http://dev.w3.org/2009/dap/vibration/#vibration-interface > "If the length of pattern is even and is not zero, then remove the last entry in > pattern." > > I think this condition never becomes true, so m_isVibrating never becomes false > unless someone calls navigator.vibrate(0) from JavaScript. Done.
https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... Source/core/testing/Internals.cpp:104: #include "modules/vibration/NavigatorVibration.h" Technically this is a bad dependency. You should be able to compile Core without any of the modules. The right thing to do is to make it possible for modules to implement their own extensions to Internals. That should be relatively easy, but will require a little bit of GYP work. https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... Source/core/testing/Internals.cpp:1999: Document* document = contextDocument(); It's better to pass in |document| explicitly. https://codereview.chromium.org/18478003/diff/6001/Source/modules/vibration/N... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/modules/vibration/N... Source/modules/vibration/NavigatorVibration.cpp:87: m_isVibrating = true; Is that true? Don't we need to wait until the one-shot timer has fired before we'll actually be vibrating? Perhaps this variable just needs to be renamed...
https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... File LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html (right): https://codereview.chromium.org/18478003/diff/1/LayoutTests/vibration/cancelV... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:20: setTimeout(function() { The tests are much easier to read now, thank you. In previous projects (not Blink / Chromium) we used mocks / fakes for time, or we would override setTimeout. Using these tools we could simply set time (synchronously) during tests, and we didn't have to wait for time to pass or make tests asynchronous. I expect something similar exists in Blink, but I don't know where, sorry. Maybe someone more experienced with Blink knows a good example? On 2013/07/08 10:14:06, kihong wrote: > On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > > Tests should not wait for timeouts, they will run too long. There should be > some > > code in the animation tests that handles this better. > > I changed my tests but I don't understand what you said exactly, is there a way > to wait without setTimeout()? https://codereview.chromium.org/18478003/diff/6001/LayoutTests/vibration/canc... File LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html (right): https://codereview.chromium.org/18478003/diff/6001/LayoutTests/vibration/canc... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:32: // Progress time by 50ms so we are in the middle of the first vibration of the pattern. Please update the time in all comments to match the code.
https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... Source/core/testing/Internals.cpp:104: #include "modules/vibration/NavigatorVibration.h" The right solution would be to add modules/vibration/InternalsVibration.idl (a partial interface for Internals.idl) and add isVibrating() to the IDL file. However, currently partial interfaces won't work for Internals' IDL files because Internals' IDL files are treated differently from other IDL files in the IDL compiler. Would you file a bug about the issue and discuss with nbarth@ ? At this point, it might make sense to land your CL first with a FIXME about it.
https://codereview.chromium.org/18478003/diff/6001/LayoutTests/vibration/canc... File LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html (right): https://codereview.chromium.org/18478003/diff/6001/LayoutTests/vibration/canc... LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html:32: // Progress time by 50ms so we are in the middle of the first vibration of the pattern. On 2013/07/10 16:10:05, Michael van Ouwerkerk wrote: > Please update the time in all comments to match the code. Done. https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... Source/core/testing/Internals.cpp:1999: Document* document = contextDocument(); On 2013/07/08 21:04:59, abarth wrote: > It's better to pass in |document| explicitly. Done. https://codereview.chromium.org/18478003/diff/6001/Source/modules/vibration/N... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/modules/vibration/N... Source/modules/vibration/NavigatorVibration.cpp:87: m_isVibrating = true; On 2013/07/08 21:04:59, abarth wrote: > Is that true? Don't we need to wait until the one-shot timer has fired before > we'll actually be vibrating? Perhaps this variable just needs to be renamed... If we move this to the timerStartFired(), there is a problem like below. 1. vibrate is called 2. cancel is called, right before timerStartFired is dispatched. In this case vibration will not be canceled, because m_isVibrating is false yet. And device will vibrate, even if cancel is called. Therefore m_isVibrating need to be positioned here. In addition, I've supposed that timer will be dispatched right after firing. It's not much different from real vibrating time.
https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/18478003/diff/6001/Source/core/testing/Intern... Source/core/testing/Internals.cpp:104: #include "modules/vibration/NavigatorVibration.h" On 2013/07/12 09:21:42, haraken wrote: > > The right solution would be to add modules/vibration/InternalsVibration.idl (a > partial interface for Internals.idl) and add isVibrating() to the IDL file. > > However, currently partial interfaces won't work for Internals' IDL files > because Internals' IDL files are treated differently from other IDL files in the > IDL compiler. Would you file a bug about the issue and discuss with nbarth@ ? > > At this point, it might make sense to land your CL first with a FIXME about it. I added FIXME here and nbarth@ going to implement about this soon. After landing his patch, I will modify test cases for vibration.
Added an issue about partial interface for Internals.idl and removing "#include modules..." https://code.google.com/p/chromium/issues/detail?id=261467
https://codereview.chromium.org/18478003/diff/19001/Source/core/testing/Inter... File Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/18478003/diff/19001/Source/core/testing/Inter... Source/core/testing/Internals.cpp:121: #include "modules/vibration/NavigatorVibration.h" This is a bad dependency. Rather than adding technical debt to the project, please refactor the code to allow modules to extend the Internals interface.
https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.cpp (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.cpp:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Outdated header: http://www.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.h (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.h:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Outdated header. https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.idl (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.idl:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Outdated header https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.cpp:118: if (!m_pattern.size()) if (m_pattern.isEmpty()) https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.h (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.h:55: bool isVibrating() { return m_isVibrating; } getter can be const.
https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.cpp (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.cpp:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY On 2013/08/13 06:41:59, Christophe Dumez wrote: > Outdated header: > http://www.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.h (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.h:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY On 2013/08/13 06:41:59, Christophe Dumez wrote: > Outdated header. Done. https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.idl (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.idl:14: * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY On 2013/08/13 06:41:59, Christophe Dumez wrote: > Outdated header Done. https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.cpp:118: if (!m_pattern.size()) On 2013/08/13 06:41:59, Christophe Dumez wrote: > if (m_pattern.isEmpty()) Done. https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.h (right): https://codereview.chromium.org/18478003/diff/25001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.h:55: bool isVibrating() { return m_isVibrating; } On 2013/08/13 06:41:59, Christophe Dumez wrote: > getter can be const. Done.
LGTM but someone more knowledgeable with gyp definitely needs to take a look.
lgtm
https://codereview.chromium.org/18478003/diff/34001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:40: 'partial_interface_test_support_idl_files': [ The variable name should start with deprecated_perl_. https://codereview.chromium.org/18478003/diff/34001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/18478003/diff/34001/Source/core/core.gypi#new... Source/core/core.gypi:3644: 'internals_test_support_idl_file': [ Why do you add new variable for Internals.idl? https://codereview.chromium.org/18478003/diff/34001/Source/modules/modules.gyp File Source/modules/modules.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/modules/modules.gy... Source/modules/modules.gyp:65: 'include_dirs': [ Is it necessary? https://codereview.chromium.org/18478003/diff/34001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/34001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.cpp:88: m_isVibrating = true; It seems m_isVibrating is equivalent to m_timerStart.isActive() || m_timerStop.isActive() (if we apply the following comment). We can remove m_isVibrating. https://codereview.chromium.org/18478003/diff/34001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.cpp:98: m_timerStop.stop(); We had better stop m_timerStart too though timerStartFired will do nothing because of m_pattern.clear(). https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp File Source/web/web_tests.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', Why do you add this block?
https://codereview.chromium.org/18478003/diff/34001/Source/bindings/derived_s... File Source/bindings/derived_sources.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/bindings/derived_s... Source/bindings/derived_sources.gyp:40: 'partial_interface_test_support_idl_files': [ On 2013/08/28 03:58:07, tkent wrote: > The variable name should start with deprecated_perl_. Done. https://codereview.chromium.org/18478003/diff/34001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/18478003/diff/34001/Source/core/core.gypi#new... Source/core/core.gypi:3644: 'internals_test_support_idl_file': [ On 2013/08/28 03:58:07, tkent wrote: > Why do you add new variable for Internals.idl? "internals.idl" need to be included to 'interface_dependencies' target for implementing partial interface. But other files which are 'deprecated_perl_webcore_test_support_idl_files' does not need to be added there because they have no dependency. Therefore they are still enough to add by "--additional-idl-files" or "--additionalIdlFiles". https://codereview.chromium.org/18478003/diff/34001/Source/modules/modules.gyp File Source/modules/modules.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/modules/modules.gy... Source/modules/modules.gyp:65: 'include_dirs': [ On 2013/08/28 03:58:07, tkent wrote: > Is it necessary? I don't think so, it's eliminated. https://codereview.chromium.org/18478003/diff/34001/Source/modules/vibration/... File Source/modules/vibration/NavigatorVibration.cpp (right): https://codereview.chromium.org/18478003/diff/34001/Source/modules/vibration/... Source/modules/vibration/NavigatorVibration.cpp:88: m_isVibrating = true; On 2013/08/28 03:58:07, tkent wrote: > It seems m_isVibrating is equivalent to m_timerStart.isActive() || > m_timerStop.isActive() (if we apply the following comment). We can remove > m_isVibrating. It's not exactly same because if there are some codes like below, vibration can not be canceled becuase of timer issue. m_timerStart.startOneShort(0); is called by line:1 but timer could not be activated until line:2 is called. Therefore cancel would be not worked. Then device would be vibrating when timer is activated. Therefore IMHO timer is not enough to check vibrating or not. below --------------------------- 1: navigator.vibrate(1000); 2: navigator.vibrate(0); https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp File Source/web/web_tests.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', On 2013/08/28 03:58:07, tkent wrote: > Why do you add this block? There is a build error during linking webkit_unit_tests becuase "V8Internals.cpp" calls InternalsVibration::isVibrating(). Therefore we need to add modules_test_support.
nbarth, haraken, would you review gyp/gypi changes for IDL handling please?
https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp File Source/web/web_tests.gyp (right): https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', On 2013/08/28 05:25:25, kihong wrote: > On 2013/08/28 03:58:07, tkent wrote: > > Why do you add this block? > > There is a build error during linking webkit_unit_tests becuase > "V8Internals.cpp" calls InternalsVibration::isVibrating(). > Therefore we need to add modules_test_support. Why does the unit test require V8Internals.cpp?
On 2013/08/28 06:07:24, tkent wrote: > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp > File Source/web/web_tests.gyp (right): > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... > Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', > On 2013/08/28 05:25:25, kihong wrote: > > On 2013/08/28 03:58:07, tkent wrote: > > > Why do you add this block? > > > > There is a build error during linking webkit_unit_tests becuase > > "V8Internals.cpp" calls InternalsVibration::isVibrating(). > > Therefore we need to add modules_test_support. > > Why does the unit test require V8Internals.cpp? V8Internals.cpp has been included by 'webcore_test_support' on the legacy implementation. In addition, other V8InternalsXXX files included in the 'webcore_test_support' also.
On 2013/08/28 06:27:38, kihong wrote: > On 2013/08/28 06:07:24, tkent wrote: > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp > > File Source/web/web_tests.gyp (right): > > > > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... > > Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', > > On 2013/08/28 05:25:25, kihong wrote: > > > On 2013/08/28 03:58:07, tkent wrote: > > > > Why do you add this block? > > > > > > There is a build error during linking webkit_unit_tests becuase > > > "V8Internals.cpp" calls InternalsVibration::isVibrating(). > > > Therefore we need to add modules_test_support. > > > > Why does the unit test require V8Internals.cpp? > > V8Internals.cpp has been included by 'webcore_test_support' on the legacy > implementation. > In addition, other V8InternalsXXX files included in the 'webcore_test_support' > also. Why do you add webcore_test_support here?
On 2013/08/28 06:01:05, tkent wrote: > nbarth, haraken, would you review gyp/gypi changes for IDL handling please? nbarth: would you take a look?
On 2013/08/28 06:36:44, tkent wrote: > On 2013/08/28 06:27:38, kihong wrote: > > On 2013/08/28 06:07:24, tkent wrote: > > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp > > > File Source/web/web_tests.gyp (right): > > > > > > > > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... > > > Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', > > > On 2013/08/28 05:25:25, kihong wrote: > > > > On 2013/08/28 03:58:07, tkent wrote: > > > > > Why do you add this block? > > > > > > > > There is a build error during linking webkit_unit_tests becuase > > > > "V8Internals.cpp" calls InternalsVibration::isVibrating(). > > > > Therefore we need to add modules_test_support. > > > > > > Why does the unit test require V8Internals.cpp? > > > > V8Internals.cpp has been included by 'webcore_test_support' on the legacy > > implementation. > > In addition, other V8InternalsXXX files included in the 'webcore_test_support' > > also. > > Why do you add webcore_test_support here? I'm not exactly sure about this, but I didn't add 'webcore_test_support' here, it's legacy statement.
> > Why do you add webcore_test_support here? > > I'm not exactly sure about this, but I didn't add 'webcore_test_support' here, > it's legacy statement. What do you mean? You add webcore_test_support in this CL since Patch Set 5.
On 2013/08/28 06:41:13, haraken wrote: > On 2013/08/28 06:01:05, tkent wrote: > > nbarth, haraken, would you review gyp/gypi changes for IDL handling please? > > nbarth: would you take a look? LGTM. One concern is that it's adding Yet Another IDL file list, but it's ok since the category is actually needed. We have several IDL file lists, which we should be able to consolidate; I'll do this when fixing dependency handling, after compiler port done.
On 2013/08/28 07:15:42, tkent wrote: > > > Why do you add webcore_test_support here? > > > > I'm not exactly sure about this, but I didn't add 'webcore_test_support' here, > > it's legacy statement. > > What do you mean? You add webcore_test_support in this CL since Patch Set 5. I'm sorry for this, I was confused with web.gyp. I'll check your comments. Thanks :)
On 2013/08/28 06:07:24, tkent wrote: > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp > File Source/web/web_tests.gyp (right): > > https://codereview.chromium.org/18478003/diff/34001/Source/web/web_tests.gyp#... > Source/web/web_tests.gyp:77: '../modules/modules.gyp:modules_test_support', > On 2013/08/28 05:25:25, kihong wrote: > > On 2013/08/28 03:58:07, tkent wrote: > > > Why do you add this block? > > > > There is a build error during linking webkit_unit_tests becuase > > "V8Internals.cpp" calls InternalsVibration::isVibrating(). > > Therefore we need to add modules_test_support. > > Why does the unit test require V8Internals.cpp? Currently webkit_unit_tests are using 'bindings_derived_sources'. And Internals is added bindings_derived_sources to support partial interface. Therefore newly webkit_unit_tests is dependent on webcore_test_support.
On 2013/08/28 09:06:53, kihong wrote: > > Why does the unit test require V8Internals.cpp? > > Currently webkit_unit_tests are using 'bindings_derived_sources'. > And Internals is added bindings_derived_sources to support partial interface. > Therefore newly webkit_unit_tests is dependent on webcore_test_support. So, your approach is wrong. Unit test should not depend on Internals, and you need to invent a way to support partial interface for Internals without adding Internals to bindings_derived_sources.
tkent: > you need to invent a way to support partial interface for Internals without > adding Internals to bindings_derived_sources. Adding partial interfaces for internals requires changing how IDL dependencies are computed. It's a bit involved because the computed dependencies are also used as inputs to bindings_derived_sources, so we only compute dependencies for main IDLs (core and modules), which is why partial interfaces are not supported for other IDLs (notably internals). I plan to fix this when I redo the IDL dependency resolution, but the IDL compiler porting is top priority, so I don't have time for this now. AFICT easiest (avoiding the hacky design that you correctly criticize) would be to not have a partial interface -- just put the partial interface IDL definitions in the main IDL file, with a FIXME to move it out to a partial interface once we compute dependencies for all IDLs. I.e., partial interfaces are just so you can have separate files, so for now just putting it in one file should work.
Nils Barth: I'm appreciate your kind comments. But abarth doesn't want to make new dependency in the core files even if it is Internals.cpp with "FIXME:". (https://codereview.chromium.org/18478003/diff/19001/Source/core/testing/Inter...) tkent: How do you think about adding "FIXME:" to the web_test.gyp in this patch. And I would like to fix it with other patch because this issue is very simple but currently it's like the tail wagging the dog.
On 2013/08/29 05:34:25, kihong wrote: > How do you think about adding "FIXME:" to the web_test.gyp in this patch. > And I would like to fix it with other patch because this issue is very simple > but currently it's like the tail wagging the dog. It sounds reasonable. It is better than adding core->modules/vibration dependency.
I newly added InternalsVibration.xxx files but they looks like moved from WebGLExtension or OESTextureFloat after "git-cl upload" on the crrev system. And it makes apply fail on the build bot and even I cannot do "git-cl patch" from this. How can I fix this problem?
On 2013/08/30 02:37:22, kihong wrote: > I newly added InternalsVibration.xxx files but they looks like moved from > WebGLExtension or OESTextureFloat after "git-cl upload" on the crrev system. > And it makes apply fail on the build bot and even I cannot do "git-cl patch" > from this. > > How can I fix this problem? git cl upload --no-find-copies
On 2013/08/30 03:16:07, tkent wrote: > On 2013/08/30 02:37:22, kihong wrote: > > I newly added InternalsVibration.xxx files but they looks like moved from > > WebGLExtension or OESTextureFloat after "git-cl upload" on the crrev system. > > And it makes apply fail on the build bot and even I cannot do "git-cl patch" > > from this. > > > > How can I fix this problem? > > git cl upload --no-find-copies tkent: Thank you very much. :-) I updated patch but I think I cannot use try bot. Please take a look at this.
On 2013/09/02 06:16:29, kihong wrote: > I updated patch but I think I cannot use try bot. > Please take a look at this. Hi Kihong, I've kicked off some try bots for you, and requested a read-only SVN account on your behalf so you can use the try bots yourself! Reference: https://sites.google.com/a/chromium.org/dev/developers/testing/try-server-usage http://www.chromium.org/getting-involved/become-a-committer#TOC-Read-only-SVN...
Partial interface for Internal is supported from Revision 159635(Support partial interface for test support idls). So, this patch doesn't need to include code generator related things for partial interface.
Nice! LGTM w/ one nit on file placement. https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gypi File Source/modules/modules.gypi (right): https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gy... Source/modules/modules.gypi:787: ], These should be in vibration/testing/InternalsVibration.idl https://codereview.chromium.org/18478003/diff/83001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.idl (right): https://codereview.chromium.org/18478003/diff/83001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.idl:32: ] partial interface Internals { No need for [ ] before "partial"
https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gypi File Source/modules/modules.gypi (right): https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gy... Source/modules/modules.gypi:787: ], On 2013/10/15 23:56:55, abarth wrote: > These should be in vibration/testing/InternalsVibration.idl Currently Internals includes these modules headers also. #include "modules/speech/DOMWindowSpeechSynthesis.h" #include "modules/speech/SpeechSynthesis.h" I intended to remove these statements also, therefore whole idls for testing moves to the modules/testing/ like core/testing. We may add some other idls for modules testing sooner or later. How do you think about this abarth?
https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gypi File Source/modules/modules.gypi (right): https://codereview.chromium.org/18478003/diff/83001/Source/modules/modules.gy... Source/modules/modules.gypi:787: ], On 2013/10/15 23:56:55, abarth wrote: > These should be in vibration/testing/InternalsVibration.idl Done. https://codereview.chromium.org/18478003/diff/83001/Source/modules/testing/In... File Source/modules/testing/InternalsVibration.idl (right): https://codereview.chromium.org/18478003/diff/83001/Source/modules/testing/In... Source/modules/testing/InternalsVibration.idl:32: ] partial interface Internals { On 2013/10/15 23:56:55, abarth wrote: > No need for [ ] before "partial" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/91001
Retried try job too often on mac_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/91001
Retried try job too often on linux_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/112001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/121001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_blink_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/121001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_blink_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/10/16 07:35:56, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on win_blink_rel. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. What is the status of this patch, is any further work required? It's a very useful change, and I'd like to depend on it :-) Thanks, Michael
On 2013/10/22 13:54:31, Michael van Ouwerkerk wrote: > On 2013/10/16 07:35:56, I haz the power (commit-bot) wrote: > > Sorry for I got bad news for ya. > > Compile failed with a clobber build on win_blink_rel. win failure is a bit surprising as the CL is cross-platform and Mac and Linux building fine. Can someone help us looking into the win bot ?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/121001
Message was sent while issue was closed.
Change committed as 160289
Message was sent while issue was closed.
On 2013/10/23 03:40:47, I haz the power (commit-bot) wrote: > Change committed as 160289 Reverting in this CL: Revert r160289 "Vibration cannot be canceled during pattern vibration." https://codereview.chromium.org/30063003/ Issue is dependency resolution bug, fixed in this CL: Fix IDL dependency computation for partial interfaces https://codereview.chromium.org/29323008/ After that lands, we can reland this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/18478003/289001
On 2013/10/23 07:28:42, Nils Barth wrote: > On 2013/10/23 03:40:47, I haz the power (commit-bot) wrote: > > Change committed as 160289 > > Reverting in this CL: > Revert r160289 "Vibration cannot be canceled during pattern vibration." > https://codereview.chromium.org/30063003/ > > Issue is dependency resolution bug, fixed in this CL: > Fix IDL dependency computation for partial interfaces > https://codereview.chromium.org/29323008/ > > After that lands, we can reland this. I re-uploaded patch for landing. Thanks Nils.
Message was sent while issue was closed.
Change committed as 160407 |
