GNOME Bugzilla – Bug 778800
Don't for..in-traverse arrays
Last modified: 2017-03-06 20:08:54 UTC
These commits removes all for..in traversals of arrays since that can have unforseen results. Case in point: PlaceBubble._populate failed after the addition of the Array.prototype.last method since it was using a for..in-loop and the body didn't expect a function as one of the array members. While trying to fix the PlaceBubble._populate method I accidentally overwrote a couple of variables and decided that the method needed some love anyways so the first patch does a more wholesome refactor of that while the next patch fixes the rest of the for..in-array traversals in a much more straight forward way.
Created attachment 346032 [details] [review] PlaceBubble: Refactor content population The _populate method was doing a little too much of work for my taste. It was split up into two parts, one that created data structures for what to show followed by code that produced widgets out of the data. This commit... - splits _populate up into separate functions for creating the data and for adding it to the popover. - rearranges code to make it a bit shorter - makes use of destructuring when it makes sense - moves a an array traversal from a for .. in-loop to a forEach-loop instead since using for .. in-loops on arrays can have unforseen consequences
Created attachment 346033 [details] [review] Don't traverse arrays with for..in-loops Traversing arrays with for .. in-loops can have unexpected consequences. In short: never do that, use Array.prototype.forEach or for..of-loops instead.
Review of attachment 346032 [details] [review]: Thanks! Some nits below ::: src/placeBubble.js @@ +208,3 @@ + halign: Gtk.Align.START }); + + this._expandedContent.attach(widget, col, row, col == 0 ? 2 : 1, 1); this is a tricky line. maybe a comment or rewrite could be due. @@ +223,3 @@ + let expandedContent = this._createExpandedContent(place); + + _populate: function(place) { maybe just this._attachContent(this._createExpandedContent(place));
Review of attachment 346033 [details] [review]: lgtm, thanks
Review of attachment 346032 [details] [review]: ::: src/placeBubble.js @@ +208,3 @@ + halign: Gtk.Align.START }); + + this._expandedContent.attach(widget, col, row, col == 0 ? 2 : 1, 1); Yes I agree, I didn't touch it though except for info ⇒ widget. I'll look at a comment first. I could also easily rewrite this a lot more. I'm not fond of the PlaceFormatter class for example and if we're even to split up data- and widget-generation I'd prefer to do the data-stuff elsewhere even. @@ +223,3 @@ + let expandedContent = this._createExpandedContent(place); + + this._attachContent(content, expandedContent); Yeah I was going to do that first as well, unfortunately we need the length of the expandedContent array to figure out whether we should show the expand-button further down. @@ +228,1 @@ this._expandButton.visible = expandedContent.length > 0; Here :)
Review of attachment 346033 [details] [review]: Cool!
Created attachment 346110 [details] [review] fixup! PlaceBubble: Refactor content population Make the attach-code easier to read. Run `git rebase -i --autosquash HEAD~3` before pushing.
So, I guess this should be in a pushable state now, so that we can get this in for 3.23.91?
Attachment 346032 [details] pushed as f87904c - PlaceBubble: Refactor content population Attachment 346033 [details] pushed as 07cb8b1 - Don't traverse arrays with for..in-loops
Follow-up patch looked good to me, rebased and squashed before pushing. Also have tested logging in and out of OSM, searching for places to excersize the affected code paths.