Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022503002:
Add ability to toggle passive state on an individual event listener. (Closed)
|
Created:
4 years, 6 months ago by dtapuska Modified:
4 years, 6 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ability to toggle passive state on an individual event listener.
Add a button to allow the toggle of the passive state on a listener.
It is not implemented for framework listeners yet since this will likely
interfere with the dipsatching of events in jquery if it was enabled.
BUG=615480
Committed: https://crrev.com/d589e0426d993a07c1d12ae685152b756e8fd3ff
Cr-Commit-Position: refs/heads/master@{#397643}
Patch Set 1 #Patch Set 2 : Limit Toggle Passive to touchstart, touchmove, mousewheel, wheel #
Total comments: 18
Patch Set 3 : Move RemoteObject onto EventListener #
Total comments: 6
Patch Set 4 : Fix nits #Patch Set 5 : Fix layout tests #Messages
Total messages: 40 (15 generated)
Description was changed from ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 ========== to ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 ==========
dtapuska@chromium.org changed reviewers: + caseq@chromium.org
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
This shows up on all listeners? To avoid UI that isn't really useful (potentially confusing) should we perhaps restrict this to show up only on the event types where we know it can make a performance difference?
On 2016/05/27 19:46:46, Rick Byers wrote: > This shows up on all listeners? To avoid UI that isn't really useful > (potentially confusing) should we perhaps restrict this to show up only on the > event types where we know it can make a performance difference? Yes it shows up on all non framework listeners. I can adjust it easily to be touch* and wheel* listeners.
On 2016/05/27 19:55:49, dtapuska wrote: > On 2016/05/27 19:46:46, Rick Byers wrote: > > This shows up on all listeners? To avoid UI that isn't really useful > > (potentially confusing) should we perhaps restrict this to show up only on the > > event types where we know it can make a performance difference? > > Yes it shows up on all non framework listeners. I can adjust it easily to be > touch* and wheel* listeners. I'd defer to devtools folks, but it seems like that's probably a good idea to me. I also worry a little that the "blocking" drop-down we already could be confusing in this context - eg. seeing "scroll" (async) listeners as "blocking" may raise alarm bells it shouldn't (and further contribute to the confusion about the difference in perf impact between scroll and wheel/touchmove listeners).
On 2016/05/27 19:58:17, Rick Byers wrote: > On 2016/05/27 19:55:49, dtapuska wrote: > > On 2016/05/27 19:46:46, Rick Byers wrote: > > > This shows up on all listeners? To avoid UI that isn't really useful > > > (potentially confusing) should we perhaps restrict this to show up only on > the > > > event types where we know it can make a performance difference? > > > > Yes it shows up on all non framework listeners. I can adjust it easily to be > > touch* and wheel* listeners. > > I'd defer to devtools folks, but it seems like that's probably a good idea to > me. I also worry a little that the "blocking" drop-down we already could be > confusing in this context - eg. seeing "scroll" (async) listeners as "blocking" > may raise alarm bells it shouldn't (and further contribute to the confusion > about the difference in perf impact between scroll and wheel/touchmove > listeners). Done; limited toggle to touchstart, touchmove, wheel and mousewheel.
On 2016/05/31 19:20:57, dtapuska wrote: > On 2016/05/27 19:58:17, Rick Byers wrote: > > On 2016/05/27 19:55:49, dtapuska wrote: > > > On 2016/05/27 19:46:46, Rick Byers wrote: > > > > This shows up on all listeners? To avoid UI that isn't really useful > > > > (potentially confusing) should we perhaps restrict this to show up only on > > the > > > > event types where we know it can make a performance difference? > > > > > > Yes it shows up on all non framework listeners. I can adjust it easily to be > > > touch* and wheel* listeners. > > > > I'd defer to devtools folks, but it seems like that's probably a good idea to > > me. I also worry a little that the "blocking" drop-down we already could be > > confusing in this context - eg. seeing "scroll" (async) listeners as > "blocking" > > may raise alarm bells it shouldn't (and further contribute to the confusion > > about the difference in perf impact between scroll and wheel/touchmove > > listeners). > > Done; limited toggle to touchstart, touchmove, wheel and mousewheel. caseq@ Can you take a look now?
https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:13: * @param {function(!WebInspector.Event)} changeListener This should either be a callback or an event listener -- currently it looks like both. I think callback is fine, just drop the Event parameter and rename to something like onChange or changeCallback. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css:40: .event-listener-delete-button,.event-listener-toggle-passive-button { let's just generalize this to something like event-listener-button https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css:50: .event-listener-delete-button:hover,.event-listener-toggle-passive-button: hover{ ditto https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); I think this is getting a bit too complicated. Actually, I think this was more complicated than necessary originally -- adding Alexey since he's the owner of this code. Basically, we do a round-trip to the front-end to inject a wrapper function that would only be called in a somewhat unlikely event of user pressing the "remove" button, and you're now adding another round-trip. What we could do instead is to do the same lazily upon remove/togglePassive being called. I think this would also simplify actual injected script by removing extra indirection in the form of nested function that gets bound. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:675: this["on" + type] = null; Not sure if this is something you want to do on toggle. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:848: togglePassiveFunction: function() I don't think this is something we should expose -- the remote object itself is not too useful for the caller, especially considering the amount of boilerplate necessary around the call below. Let's rename to canTogglePassive() and it would be a nice opportunity to encapsulate isScrollBlockingType() as well. Ditto for removeFunction(), I guess. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:883: function callCustomRemove(func, type, listener, useCapture, passive) copy-paste hurts :-) Perhaps, extract something like _dispatchEventListenerHelper()? https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:911: return this._type == "touchstart" || s/==/===/g
caseq@chromium.org changed reviewers: + kozyatinskiy@chromium.org
dtapuska@chromium.org changed reviewers: - kozyatinskiy@chromium.org
https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); On 2016/06/01 17:48:11, caseq wrote: > I think this is getting a bit too complicated. Actually, I think this was more > complicated than necessary originally -- adding Alexey since he's the owner of > this code. Basically, we do a round-trip to the front-end to inject a wrapper > function that would only be called in a somewhat unlikely event of user pressing > the "remove" button, and you're now adding another round-trip. What we could do > instead is to do the same lazily upon remove/togglePassive being called. I think > this would also simplify actual injected script by removing extra indirection in > the form of nested function that gets bound. I completely agree. I tried passing back an array of bound functions but that didn't work. I was a little unclear why it was done this way. I wondered if it was because of framework listeners. It isn't clear to me why you can't just call it to execute a remote function when you goto remove. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:848: togglePassiveFunction: function() On 2016/06/01 17:48:11, caseq wrote: > I don't think this is something we should expose -- the remote object itself is > not too useful for the caller, especially considering the amount of boilerplate > necessary around the call below. Let's rename to canTogglePassive() and it would > be a nice opportunity to encapsulate isScrollBlockingType() as well. Ditto for > removeFunction(), I guess. About merging them... I wanted to separate the choice of toggling passive to the UI. You can toggle any listener as passive or not. But really we just restrict the UI so it doesn't confuse the users because toggling a non scroll blocking listener doesn't really have much perf benefit.
+kozyatinskiy for real now.
kozyatinskiy@chromium.org changed reviewers: + kozyatinskiy@chromium.org
https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); On 2016/06/01 17:54:37, dtapuska wrote: > On 2016/06/01 17:48:11, caseq wrote: > > I think this is getting a bit too complicated. Actually, I think this was more > > complicated than necessary originally -- adding Alexey since he's the owner of > > this code. Basically, we do a round-trip to the front-end to inject a wrapper > > function that would only be called in a somewhat unlikely event of user > pressing > > the "remove" button, and you're now adding another round-trip. What we could > do > > instead is to do the same lazily upon remove/togglePassive being called. I > think > > this would also simplify actual injected script by removing extra indirection > in > > the form of nested function that gets bound. > > I completely agree. I tried passing back an array of bound functions but that > didn't work. I was a little unclear why it was done this way. I wondered if it > was because of framework listeners. It isn't clear to me why you can't just call > it to execute a remote function when you goto remove. It was added here to align implementation for native event listeners with framework event listeners. Since we moved Command Line API getEventListeners method into native, now we can add removeFunction to protocol EventListener object and report this from backend without roundtrip. It will simplify this code. I can prepare CL for this. For toggling passive event listeners it's better to call this function on node when user click toggle button with RemoteObject.callFunctionOn method.
https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); On 2016/06/01 19:05:01, kozyatinskiy wrote: > On 2016/06/01 17:54:37, dtapuska wrote: > > On 2016/06/01 17:48:11, caseq wrote: > > > I think this is getting a bit too complicated. Actually, I think this was > more > > > complicated than necessary originally -- adding Alexey since he's the owner > of > > > this code. Basically, we do a round-trip to the front-end to inject a > wrapper > > > function that would only be called in a somewhat unlikely event of user > > pressing > > > the "remove" button, and you're now adding another round-trip. What we could > > do > > > instead is to do the same lazily upon remove/togglePassive being called. I > > think > > > this would also simplify actual injected script by removing extra > indirection > > in > > > the form of nested function that gets bound. > > > > I completely agree. I tried passing back an array of bound functions but that > > didn't work. I was a little unclear why it was done this way. I wondered if it > > was because of framework listeners. It isn't clear to me why you can't just > call > > it to execute a remote function when you goto remove. > > It was added here to align implementation for native event listeners with > framework event listeners. Since we moved Command Line API getEventListeners > method into native, now we can add removeFunction to protocol EventListener > object and report this from backend without roundtrip. It will simplify this > code. I can prepare CL for this. > For toggling passive event listeners it's better to call this function on node > when user click toggle button with RemoteObject.callFunctionOn method. Do you mean just calling this from EventListenersView? I'd expect that togglePassive be a function on WebInspector.EventListener and it would invoke the code on the target's remote Object. However it appears that the target on the WebInspector.EventListener isn't a RemoteObject but a WebInspector.Target
On 2016/06/01 20:22:59, dtapuska wrote: > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): > > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: > this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); > On 2016/06/01 19:05:01, kozyatinskiy wrote: > > On 2016/06/01 17:54:37, dtapuska wrote: > > > On 2016/06/01 17:48:11, caseq wrote: > > > > I think this is getting a bit too complicated. Actually, I think this was > > more > > > > complicated than necessary originally -- adding Alexey since he's the > owner > > of > > > > this code. Basically, we do a round-trip to the front-end to inject a > > wrapper > > > > function that would only be called in a somewhat unlikely event of user > > > pressing > > > > the "remove" button, and you're now adding another round-trip. What we > could > > > do > > > > instead is to do the same lazily upon remove/togglePassive being called. I > > > think > > > > this would also simplify actual injected script by removing extra > > indirection > > > in > > > > the form of nested function that gets bound. > > > > > > I completely agree. I tried passing back an array of bound functions but > that > > > didn't work. I was a little unclear why it was done this way. I wondered if > it > > > was because of framework listeners. It isn't clear to me why you can't just > > call > > > it to execute a remote function when you goto remove. > > > > It was added here to align implementation for native event listeners with > > framework event listeners. Since we moved Command Line API getEventListeners > > method into native, now we can add removeFunction to protocol EventListener > > object and report this from backend without roundtrip. It will simplify this > > code. I can prepare CL for this. > > For toggling passive event listeners it's better to call this function on node > > when user click toggle button with RemoteObject.callFunctionOn method. > > Do you mean just calling this from EventListenersView? WI.EventListener() should encapsulate the complexity associated with remote function call (the way it does now). Alexey, you seem to imply that remove should be implemented differently from togglePassive. I don't quite see what the difference is. > I'd expect that togglePassive be a function on WebInspector.EventListener and it > would invoke the code on the target's remote Object. > However it appears that the target on the WebInspector.EventListener isn't a RemoteObject but a > WebInspector.Target Correct (and, just in case, "target" means a totally different thing in devtools, basically a debugging target). So one option would be to add an event target to WI.EventListener, another option is to keep EventListener as is and just construct it with the properly bound remove()/togglePassive() callbacks within RemoteObject.getEventListeners() that would rely on RemoteObject.callFunctionOn() internally.
On 2016/06/01 21:11:25, caseq wrote: > On 2016/06/01 20:22:59, dtapuska wrote: > > > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): > > > > > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: > > > this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); > > On 2016/06/01 19:05:01, kozyatinskiy wrote: > > > On 2016/06/01 17:54:37, dtapuska wrote: > > > > On 2016/06/01 17:48:11, caseq wrote: > > > > > I think this is getting a bit too complicated. Actually, I think this > was > > > more > > > > > complicated than necessary originally -- adding Alexey since he's the > > owner > > > of > > > > > this code. Basically, we do a round-trip to the front-end to inject a > > > wrapper > > > > > function that would only be called in a somewhat unlikely event of user > > > > pressing > > > > > the "remove" button, and you're now adding another round-trip. What we > > could > > > > do > > > > > instead is to do the same lazily upon remove/togglePassive being called. > I > > > > think > > > > > this would also simplify actual injected script by removing extra > > > indirection > > > > in > > > > > the form of nested function that gets bound. > > > > > > > > I completely agree. I tried passing back an array of bound functions but > > that > > > > didn't work. I was a little unclear why it was done this way. I wondered > if > > it > > > > was because of framework listeners. It isn't clear to me why you can't > just > > > call > > > > it to execute a remote function when you goto remove. > > > > > > It was added here to align implementation for native event listeners with > > > framework event listeners. Since we moved Command Line API getEventListeners > > > method into native, now we can add removeFunction to protocol EventListener > > > object and report this from backend without roundtrip. It will simplify this > > > code. I can prepare CL for this. > > > For toggling passive event listeners it's better to call this function on > node > > > when user click toggle button with RemoteObject.callFunctionOn method. > > > > Do you mean just calling this from EventListenersView? > > WI.EventListener() should encapsulate the complexity associated with remote > function call (the way it does now). > > Alexey, you seem to imply that remove should be implemented differently from > togglePassive. I don't quite see what the difference is. > > > I'd expect that togglePassive be a function on WebInspector.EventListener and > it > > would invoke the code on the target's remote Object. > > However it appears that the target on the WebInspector.EventListener isn't a > RemoteObject but a > > WebInspector.Target > > Correct (and, just in case, "target" means a totally different thing in > devtools, basically a debugging target). So one option would be to add an event > target to WI.EventListener, another option is to keep EventListener as is and > just construct it with the properly bound remove()/togglePassive() callbacks > within RemoteObject.getEventListeners() that would rely on > RemoteObject.callFunctionOn() internally. If we wish to actually return descendant event listeners from the getEventListeners call it would imply that you may get back different event listeners and their targets might be your descendants. So I'd expect the former to actually be better.
On 2016/06/01 21:18:10, dtapuska wrote: > On 2016/06/01 21:11:25, caseq wrote: > > On 2016/06/01 20:22:59, dtapuska wrote: > > > > > > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js > (right): > > > > > > > > > https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:579: > > > > > > this.callFunctionPromise(nodeTogglePassiveEventListener).then(storeTogglePassiveFunction.bind(this)); > > > On 2016/06/01 19:05:01, kozyatinskiy wrote: > > > > On 2016/06/01 17:54:37, dtapuska wrote: > > > > > On 2016/06/01 17:48:11, caseq wrote: > > > > > > I think this is getting a bit too complicated. Actually, I think this > > was > > > > more > > > > > > complicated than necessary originally -- adding Alexey since he's the > > > owner > > > > of > > > > > > this code. Basically, we do a round-trip to the front-end to inject a > > > > wrapper > > > > > > function that would only be called in a somewhat unlikely event of > user > > > > > pressing > > > > > > the "remove" button, and you're now adding another round-trip. What we > > > could > > > > > do > > > > > > instead is to do the same lazily upon remove/togglePassive being > called. > > I > > > > > think > > > > > > this would also simplify actual injected script by removing extra > > > > indirection > > > > > in > > > > > > the form of nested function that gets bound. > > > > > > > > > > I completely agree. I tried passing back an array of bound functions but > > > that > > > > > didn't work. I was a little unclear why it was done this way. I wondered > > if > > > it > > > > > was because of framework listeners. It isn't clear to me why you can't > > just > > > > call > > > > > it to execute a remote function when you goto remove. > > > > > > > > It was added here to align implementation for native event listeners with > > > > framework event listeners. Since we moved Command Line API > getEventListeners > > > > method into native, now we can add removeFunction to protocol > EventListener > > > > object and report this from backend without roundtrip. It will simplify > this > > > > code. I can prepare CL for this. > > > > For toggling passive event listeners it's better to call this function on > > node > > > > when user click toggle button with RemoteObject.callFunctionOn method. > > > > > > Do you mean just calling this from EventListenersView? > > > > WI.EventListener() should encapsulate the complexity associated with remote > > function call (the way it does now). > > > > Alexey, you seem to imply that remove should be implemented differently from > > togglePassive. I don't quite see what the difference is. > > > > > I'd expect that togglePassive be a function on WebInspector.EventListener > and > > it > > > would invoke the code on the target's remote Object. > > > However it appears that the target on the WebInspector.EventListener isn't a > > RemoteObject but a > > > WebInspector.Target > > > > Correct (and, just in case, "target" means a totally different thing in > > devtools, basically a debugging target). So one option would be to add an > event > > target to WI.EventListener, another option is to keep EventListener as is and > > just construct it with the properly bound remove()/togglePassive() callbacks > > within RemoteObject.getEventListeners() that would rely on > > RemoteObject.callFunctionOn() internally. > > If we wish to actually return descendant event listeners from the > getEventListeners call it would imply that you may get back different event > listeners and their targets might be your descendants. So I'd expect the former > to actually be better. I've put the RemoteObject on the EventListener object and have addressed the other concerns of the callFunction stuff. PTAL.
https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:13: * @param {function(!WebInspector.Event)} changeListener On 2016/06/01 17:48:10, caseq wrote: > This should either be a callback or an event listener -- currently it looks like > both. I think callback is fine, just drop the Event parameter and rename to > something like onChange or changeCallback. Done. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css:40: .event-listener-delete-button,.event-listener-toggle-passive-button { On 2016/06/01 17:48:10, caseq wrote: > let's just generalize this to something like event-listener-button Done. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/eventListenersView.css:50: .event-listener-delete-button:hover,.event-listener-toggle-passive-button: hover{ On 2016/06/01 17:48:10, caseq wrote: > ditto Done. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:675: this["on" + type] = null; On 2016/06/01 17:48:11, caseq wrote: > Not sure if this is something you want to do on toggle. Acknowledged. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right): https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:883: function callCustomRemove(func, type, listener, useCapture, passive) On 2016/06/01 17:48:11, caseq wrote: > copy-paste hurts :-) Perhaps, extract something like > _dispatchEventListenerHelper()? Done. https://codereview.chromium.org/2022503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:911: return this._type == "touchstart" || On 2016/06/01 17:48:11, caseq wrote: > s/==/===/g Done.
On 2016/06/01 21:11:25, caseq wrote: > Alexey, you seem to imply that remove should be implemented differently from > togglePassive. I don't quite see what the difference is. For remove function we can reuse code that we use for Command Line API getEventListeners to generate remove function and report it via protocol and align native with framework event listeners. I've landed a CL with this: https://codereview.chromium.org/2034533002/
lgtm % nits https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:312: passiveButton.title = WebInspector.UIString("Toggle passive event listener field"); nit: perhaps something more distinct from textContent, e.g. "toggle whether event listener is passive or blocking" or "toggle whether events will be delivered synchronously or asynchronously" https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right): https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:717: * @param {!WebInspector.RemoteObject} registeredTarget nit: eventTarget, perhaps? https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:903: this._type === "touchmove" || wrong indent, just indent 4 spaces or fold to one line.
https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js (right): https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/EventListenersView.js:312: passiveButton.title = WebInspector.UIString("Toggle passive event listener field"); On 2016/06/02 19:40:27, caseq wrote: > nit: perhaps something more distinct from textContent, e.g. "toggle whether > event listener is passive or blocking" or "toggle whether events will be > delivered synchronously or asynchronously" Done. https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right): https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:717: * @param {!WebInspector.RemoteObject} registeredTarget On 2016/06/02 19:40:27, caseq wrote: > nit: eventTarget, perhaps? Done. https://codereview.chromium.org/2022503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:903: this._type === "touchmove" || On 2016/06/02 19:40:27, caseq wrote: > wrong indent, just indent 4 spaces or fold to one line. Done.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2022503002/#ps60001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022503002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022503002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2022503002/#ps80001 (title: "Fix layout tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022503002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022503002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022503002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022503002/80001
Message was sent while issue was closed.
Description was changed from ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 ========== to ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 ========== to ========== Add ability to toggle passive state on an individual event listener. Add a button to allow the toggle of the passive state on a listener. It is not implemented for framework listeners yet since this will likely interfere with the dipsatching of events in jquery if it was enabled. BUG=615480 Committed: https://crrev.com/d589e0426d993a07c1d12ae685152b756e8fd3ff Cr-Commit-Position: refs/heads/master@{#397643} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d589e0426d993a07c1d12ae685152b756e8fd3ff Cr-Commit-Position: refs/heads/master@{#397643} |
