Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1532503003:
Add FakeChromeEvent (Closed)
DescriptionAdd FakeChromeEvent
This will be needed for bluetooth and networking tests (and any other
tests that rely on extension APIs with events).
Initially use it in FakeSettingsPrivate.
BUG=425627
Committed: https://crrev.com/5afbdd002bc714d522b28ed13fcbb61c3e1bbd5b
Cr-Commit-Position: refs/heads/master@{#366181}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use Set for listeners #
Total comments: 4
Patch Set 3 : Feedback #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : var_args #Patch Set 6 : Fix extraLibraries #
Messages
Total messages: 29 (9 generated)
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org, dschuyler@chromium.org, michaelpg@chromium.org
https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... chrome/test/data/webui/fake_chrome_event.js:26: var index = this.listeners_.indexOf(listener); Can you use a native Javascript Set instead of an array? I used it for the first time in Chromium's codebase at https://codereview.chromium.org/1518203003. This way you don't need to do linear searching (indexOf). https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/fake_settings_private.js:40: for (var key in this.prefs) Drive by: That is fine, but as FYI you could also use Array#map as follows. var prefs = Object.keys(this.prefs).map(function(key) { return deepCopy(this.prefs[key]; }.bind(this));
https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... chrome/test/data/webui/fake_chrome_event.js:26: var index = this.listeners_.indexOf(listener); On 2015/12/16 21:18:07, dpapad wrote: > Can you use a native Javascript Set instead of an array? I used it for the first > time in Chromium's codebase at https://codereview.chromium.org/1518203003. This > way you don't need to do linear searching (indexOf). Done. https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/fake_settings_private.js (right): https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/fake_settings_private.js:40: for (var key in this.prefs) On 2015/12/16 21:18:07, dpapad wrote: > Drive by: That is fine, but as FYI you could also use Array#map as follows. > > var prefs = Object.keys(this.prefs).map(function(key) { > return deepCopy(this.prefs[key]; > }.bind(this)); Question: Is there an advantage to using map() here instead? To me this just seems like a slightly more convoluted way of doing exactly the same thing. Also, I threw together a quick fiddle and unless I messed something up it appears to be almost 10x slower: https://jsfiddle.net/stevenjb_chromium/s5sj6swn/
https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:20: addListener: function(listener) { Maybe: since this is now a Set and therefore should not allow a listener to be added twice, could we assert or 'expect' that here? assertFalse(this.listeners_.has(listener));
On 2015/12/16 at 22:20:57, stevenjb wrote: > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > File chrome/test/data/webui/fake_chrome_event.js (right): > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > chrome/test/data/webui/fake_chrome_event.js:26: var index = this.listeners_.indexOf(listener); > On 2015/12/16 21:18:07, dpapad wrote: > > Can you use a native Javascript Set instead of an array? I used it for the first > > time in Chromium's codebase at https://codereview.chromium.org/1518203003. This > > way you don't need to do linear searching (indexOf). > > Done. > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > File chrome/test/data/webui/settings/fake_settings_private.js (right): > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > chrome/test/data/webui/settings/fake_settings_private.js:40: for (var key in this.prefs) > On 2015/12/16 21:18:07, dpapad wrote: > > Drive by: That is fine, but as FYI you could also use Array#map as follows. > > > > var prefs = Object.keys(this.prefs).map(function(key) { > > return deepCopy(this.prefs[key]; > > }.bind(this)); > > Question: Is there an advantage to using map() here instead? To me this just seems like a slightly more convoluted way of doing exactly the same thing. Also, I threw together a quick fiddle and unless I messed something up it appears to be almost 10x slower: https://jsfiddle.net/stevenjb_chromium/s5sj6swn/ Line 22 in the fiddle has a bug, it is just pushing deepCopy function itself, but it is not calling it. var o3 = []; for (var k in o1) o3.push(deepCopy(o1[k])); Regarding the advantage of using map(), in theory there is one. Allocating an empty array and growing it with push means that under the cover every time the preallocated length of the array is reached the whole array is copied elsewhere and the pre-allocation is doubled. When using map() the length of the array is known before hand so copying the array is not needed. It is the equivalent of the following var myArray = new Array(10); // 10 is the size of the empty array we just created. for (var i = 0; i < 10) { myArray[i] = 'foo'; // assigning directly instead of using push. }
On 2015/12/16 at 22:39:08, dpapad wrote: > On 2015/12/16 at 22:20:57, stevenjb wrote: > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > > File chrome/test/data/webui/fake_chrome_event.js (right): > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > > chrome/test/data/webui/fake_chrome_event.js:26: var index = this.listeners_.indexOf(listener); > > On 2015/12/16 21:18:07, dpapad wrote: > > > Can you use a native Javascript Set instead of an array? I used it for the first > > > time in Chromium's codebase at https://codereview.chromium.org/1518203003. This > > > way you don't need to do linear searching (indexOf). > > > > Done. > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > > File chrome/test/data/webui/settings/fake_settings_private.js (right): > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > > chrome/test/data/webui/settings/fake_settings_private.js:40: for (var key in this.prefs) > > On 2015/12/16 21:18:07, dpapad wrote: > > > Drive by: That is fine, but as FYI you could also use Array#map as follows. > > > > > > var prefs = Object.keys(this.prefs).map(function(key) { > > > return deepCopy(this.prefs[key]; > > > }.bind(this)); > > > > Question: Is there an advantage to using map() here instead? To me this just seems like a slightly more convoluted way of doing exactly the same thing. Also, I threw together a quick fiddle and unless I messed something up it appears to be almost 10x slower: https://jsfiddle.net/stevenjb_chromium/s5sj6swn/ > > > Line 22 in the fiddle has a bug, it is just pushing deepCopy function itself, but it is not calling it. > var o3 = []; > for (var k in o1) > o3.push(deepCopy(o1[k])); > > Regarding the advantage of using map(), in theory there is one. Allocating an empty array and growing it with push means that under the cover > every time the preallocated length of the array is reached the whole array is copied elsewhere and the pre-allocation is doubled. When using map() > the length of the array is known before hand so copying the array is not needed. It is the equivalent of the following > > var myArray = new Array(10); // 10 is the size of the empty array we just created. > for (var i = 0; i < 10) { > myArray[i] = 'foo'; // assigning directly instead of using push. > } Messed up the for loop above by forgetting i++. This is actually a fortunate coincidence, since it reminded me of the other advantage of forEach, map over regular for loops is that there is less chance to mess up things. So my practice has been, always use forEach/map, unless I want to iterate over a subset, break out of the loop on some condition.
You're right. That makes a lot more sense, i was pretty surprised by the result actually. I fixed the test and added some verification. Now pushing onto an array directly is about 40% faster, which is more of what I would expect. The reason I expected that appending to an array would be faster is that this is a common operation and most allocators are smarter than doing a copy every time. The overhead of calling a function for each element is much higher than the copy time. If we pre-allocate the Array (like with your example) we can get an additional 20% improvement. So, while I understand that forEach is a nice object oriented way to do things in JS, I'm not convinced it should be preferred in straightforward cases like this one. On Wed, Dec 16, 2015 at 2:39 PM, dpapad@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On 2015/12/16 at 22:20:57, stevenjb wrote: > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > >> File chrome/test/data/webui/fake_chrome_event.js (right): >> > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/fake... > >> chrome/test/data/webui/fake_chrome_event.js:26: var index = >> > this.listeners_.indexOf(listener); > >> On 2015/12/16 21:18:07, dpapad wrote: >> > Can you use a native Javascript Set instead of an array? I used it for >> the >> > first > >> > time in Chromium's codebase at >> https://codereview.chromium.org/1518203003. >> > This > >> > way you don't need to do linear searching (indexOf). >> > > Done. >> > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > >> File chrome/test/data/webui/settings/fake_settings_private.js (right): >> > > > > https://codereview.chromium.org/1532503003/diff/1/chrome/test/data/webui/sett... > >> chrome/test/data/webui/settings/fake_settings_private.js:40: for (var key >> in >> > this.prefs) > >> On 2015/12/16 21:18:07, dpapad wrote: >> > Drive by: That is fine, but as FYI you could also use Array#map as >> follows. >> > >> > var prefs = Object.keys(this.prefs).map(function(key) { >> > return deepCopy(this.prefs[key]; >> > }.bind(this)); >> > > Question: Is there an advantage to using map() here instead? To me this >> just >> > seems like a slightly more convoluted way of doing exactly the same thing. > Also, > I threw together a quick fiddle and unless I messed something up it > appears to > be almost 10x slower: https://jsfiddle.net/stevenjb_chromium/s5sj6swn/ > > > Line 22 in the fiddle has a bug, it is just pushing deepCopy function > itself, > but it is not calling it. > var o3 = []; > for (var k in o1) > o3.push(deepCopy(o1[k])); > > Regarding the advantage of using map(), in theory there is one. Allocating > an > empty array and growing it with push means that under the cover > every time the preallocated length of the array is reached the whole array > is > copied elsewhere and the pre-allocation is doubled. When using map() > the length of the array is known before hand so copying the array is not > needed. > It is the equivalent of the following > > var myArray = new Array(10); // 10 is the size of the empty array we just > created. > for (var i = 0; i < 10) { > myArray[i] = 'foo'; // assigning directly instead of using push. > } > > https://codereview.chromium.org/1532503003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm LGTM FWIW the most comprehensive comparison I have found is https://jsperf.com/for-vs-foreach/435. In any case, I don't think we need to have a hard rule on doing loops in a certain way. I prefer using map() when transforming one array to another, and using forEach when iterating over an entire array, but as said I don't think there should be a rule about it. https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:15: this.listeners_ = new Set; Nit: Can we use the following syntax? this.listeners_ = new Set(); I have not encountered the syntax with the omitted () in Chromium's codebase.
https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:15: this.listeners_ = new Set; On 2015/12/17 00:04:18, dpapad wrote: > Nit: Can we use the following syntax? > > this.listeners_ = new Set(); > I have not encountered the syntax with the omitted () in Chromium's codebase. Sure. That's just my C background (and newness to JS) showing through :) https://codereview.chromium.org/1532503003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:20: addListener: function(listener) { On 2015/12/16 22:27:03, dschuyler wrote: > Maybe: since this is now a Set and therefore should not allow a listener to be > added twice, could we assert or 'expect' that here? > assertFalse(this.listeners_.has(listener)); Done.
michaelpg@ - Please take a look when you have a moment.
lgtm
lgtm, thanks for doing this and removing my hack! https://codereview.chromium.org/1532503003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:32: callListeners: function(...args) { var_args
https://codereview.chromium.org/1532503003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/fake_chrome_event.js (right): https://codereview.chromium.org/1532503003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/fake_chrome_event.js:32: callListeners: function(...args) { On 2015/12/17 23:26:20, michaelpg wrote: > var_args Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, dschuyler@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1532503003/#ps80001 (title: "var_args")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532503003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532503003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, michaelpg@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1532503003/#ps100001 (title: "Fix extraLibraries")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532503003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532503003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1532503003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1532503003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add FakeChromeEvent This will be needed for bluetooth and networking tests (and any other tests that rely on extension APIs with events). Initially use it in FakeSettingsPrivate. BUG=425627 ========== to ========== Add FakeChromeEvent This will be needed for bluetooth and networking tests (and any other tests that rely on extension APIs with events). Initially use it in FakeSettingsPrivate. BUG=425627 Committed: https://crrev.com/5afbdd002bc714d522b28ed13fcbb61c3e1bbd5b Cr-Commit-Position: refs/heads/master@{#366181} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5afbdd002bc714d522b28ed13fcbb61c3e1bbd5b Cr-Commit-Position: refs/heads/master@{#366181} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
