Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
383993003:
Implement scheduleImmediate on Chrome/Drt/Safari/Firefox. (Closed)
|
Created:
6 years, 5 months ago by floitsch Modified:
6 years, 5 months ago CC:
reviews_dartlang.org, Siggi Cherem (dart-lang) Visibility:
Public. |
DescriptionImplement scheduleImmediate on Chrome/Drt/Safari/Firefox.
BUG= http://dartbug.com/9002
R=lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=38232
Reverted: https://code.google.com/p/dart/source/detail?r=38237
Committed: https://code.google.com/p/dart/source/detail?r=38274
Patch Set 1 #Patch Set 2 : Upload #
Total comments: 8
Patch Set 3 : Change children. #
Total comments: 2
Patch Set 4 : Add comments #Patch Set 5 : Reupload after revert. #
Total comments: 3
Patch Set 6 : Revert changes to html.status. #Messages
Total messages: 13 (0 generated)
Still need to look at tests and potential status-file updates, but the logic should be fine.
lgtm https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... File sdk/lib/_internal/lib/async_patch.dart (right): https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:45: leaveJsAsync(); Is there a matching enterJsAsync? If so, add a comment that says where the match is. https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:57: assert(storedCallback == null); This is just checking that we don't call the register function more than once, right? https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:64: // Also check for other JS options like mutation observer or runImmediate. Comment can be removed now. Replace with "If all else fails, fall back on timer".
https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... File sdk/lib/_internal/lib/async_patch.dart (right): https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:38: if (JS('', 'self.MutationObserver') != null && Now that Promises are in the browser, the best technique is to use them, and only fall back to MutationObserver if they are not present. if (self.Promise && self.Promise.resolve) { var promise = Promise.resolve(); return (void callback()) { promise.then(() { try { callback(); } catch() { // No exceptions should propagate up to the Promise. } }); } }
I still have a few failing tests that need to be investigated, but I just discovered that my Dartium and drt are not updating anymore (drt is from January), so hopefully they will start working with a new version. https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... File sdk/lib/_internal/lib/async_patch.dart (right): https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:38: if (JS('', 'self.MutationObserver') != null && On 2014/07/14 17:02:51, blois wrote: > Now that Promises are in the browser, the best technique is to use them, and > only fall back to MutationObserver if they are not present. > > if (self.Promise && self.Promise.resolve) { > var promise = Promise.resolve(); > return (void callback()) { > promise.then(() { > try { > callback(); > } catch() { > // No exceptions should propagate up to the Promise. > } > }); > } > } Unfortunately FF doesn't follow the spec. https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:45: leaveJsAsync(); On 2014/07/14 11:25:55, Lasse Reichstein Nielsen wrote: > Is there a matching enterJsAsync? If so, add a comment that says where the match > is. Added it to the callback below. It shouldn't be necessary, as we only need to add them for functions that can work in webworkers. But I don't think it hurts. https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:57: assert(storedCallback == null); On 2014/07/14 11:25:56, Lasse Reichstein Nielsen wrote: > This is just checking that we don't call the register function more than once, > right? Right. It's an assert in "user"-code, so we could remove it. https://codereview.chromium.org/383993003/diff/20001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:64: // Also check for other JS options like mutation observer or runImmediate. On 2014/07/14 11:25:56, Lasse Reichstein Nielsen wrote: > Comment can be removed now. Replace with "If all else fails, fall back on > timer". We still need to do the setImmediate for IE10.
https://codereview.chromium.org/383993003/diff/40001/sdk/lib/_internal/lib/as... File sdk/lib/_internal/lib/async_patch.dart (right): https://codereview.chromium.org/383993003/diff/40001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:63: // JS('', '#.hidden = !#.hidden', div, div); Looking further into it, it seems that setting attributes works if you use `setAttribute`, rather than updating the underlying properties of the node. For example, this works: var div = document.createElement('div'); new MutationObserver(function(x) { console.log('here'); }) .observe(div, {attributes: true}) div.setAttribute('data-anything', 'value'); This also works: div.setAttribute('hidden', ''); Setting the property directly seems to be the issue you were hitting. Interestingly, I noticed that for some properties, like `className`, it may work if you set the property. I think it's safer to go with setAttribute or the childList approach you have here for now. It might be worth adding a comment explaining why we do it that way though. Here is the bug I filed for the shadow-dom polyfills in case you want to refer to it here: https://github.com/Polymer/ShadowDOM/issues/468
https://codereview.chromium.org/383993003/diff/40001/sdk/lib/_internal/lib/as... File sdk/lib/_internal/lib/async_patch.dart (right): https://codereview.chromium.org/383993003/diff/40001/sdk/lib/_internal/lib/as... sdk/lib/_internal/lib/async_patch.dart:63: // JS('', '#.hidden = !#.hidden', div, div); On 2014/07/14 22:04:02, Siggi Cherem (dart-lang) wrote: > Looking further into it, it seems that setting attributes works if you use > `setAttribute`, rather than updating the underlying properties of the node. For > example, this works: > > var div = document.createElement('div'); > > new MutationObserver(function(x) { console.log('here'); }) > .observe(div, {attributes: true}) > div.setAttribute('data-anything', 'value'); > > This also works: > div.setAttribute('hidden', ''); > > Setting the property directly seems to be the issue you were hitting. > Interestingly, I noticed that for some properties, like `className`, it may work > if you set the property. > > I think it's safer to go with setAttribute or the childList approach you have > here for now. It might be worth adding a comment explaining why we do it that > way though. Here is the bug I filed for the shadow-dom polyfills in case you > want to refer to it here: https://github.com/Polymer/ShadowDOM/issues/468 > Done.
Local tests are good now. (updating drt helped...). I will commit now (since there is a non-zero probability that I will have to revert). If you have comments I will address them in a separate CL.
Message was sent while issue was closed.
Committed patchset #4 manually as r38232 (presubmit successful).
Had to revert. The following tests started failing. FF: python tools/test.py -mrelease -cdart2js -rff --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document Chrome (can't reproduce locally): python tools/test.py -mrelease -cdart2js -rchrome --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/attribute_changed_callback_test/unsupported_on_polyfill python tools/test.py -mrelease -cdart2js -rchrome --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document Safari: python tools/test.py -mrelease -cdart2js -rsafari --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document python tools/test.py -mrelease -cdart2js -rsafari --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 pkg/polymer/test/js_interop_test python tools/test.py -mrelease -cdart2js -rsafarimobilesim --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document Windows: python tools/test.py -mrelease -cdart2js -rie10 --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 pkg/polymer/test/two_way_bind_test python tools/test.py -mrelease -cdart2js -rchrome --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/attribute_changed_callback_test/unsupported_on_polyfill python tools/test.py -mrelease -cdart2js -rchrome --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document python tools/test.py -mrelease -cdart2js -rie10 --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache --checked -t120 pkg/polymer/test/two_way_bind_test python tools/test.py -mrelease -cdart2js -rff --use-sdk --write-debug-log --write-test-outcome-log --clear_browser_cache -t60 html/custom/entered_left_view_test/viewless_document
https://codereview.chromium.org/383993003/diff/80001/tests/html/html.status File tests/html/html.status (left): https://codereview.chromium.org/383993003/diff/80001/tests/html/html.status#o... tests/html/html.status:25: [ $compiler == dart2js && ($runtime == safari || $runtime == safarimobilesim || $runtime == ff || $ie || $runtime == chrome) ] Why these changes? This should have been correct as it was.
https://codereview.chromium.org/383993003/diff/80001/tests/html/html.status File tests/html/html.status (left): https://codereview.chromium.org/383993003/diff/80001/tests/html/html.status#o... tests/html/html.status:25: [ $compiler == dart2js && ($runtime == safari || $runtime == safarimobilesim || $runtime == ff || $ie || $runtime == chrome) ] On 2014/07/15 17:48:38, blois wrote: > Why these changes? This should have been correct as it was. I believe the test started working on my Chrome-dev and I assumed it was because of my change. Embarrassing... https://codereview.chromium.org/383993003/diff/80001/tests/html/html.status#o... tests/html/html.status:29: custom/attribute_changed_callback_test/unsupported_on_polyfill: Fail # Issue 18931 (Disabled for Chrome 35 roll I guess this one is for the same reasons.
Updated the status file. Will commit again to have an idea of how much is still broken and how much was just a flake.
Message was sent while issue was closed.
Committed patchset #6 manually as r38274 (presubmit successful). |
