Quick and dirty patch I made for the work week. Dave, does the approach sound good to you? No extra actor, just 3 new functions in the walker actor.

Maybe extra methods on the Inspector actor? I don't mind either way, approach looks fine to me.

Comment on attachment 800854 [details] [diff] [review] move LayoutHelpers to toolkit/ Review of attachment 800854 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming these are the same files.

Comment on attachment 800857 [details] [diff] [review] actual code Review of attachment 800857 [details] [diff] [review]: ----------------------------------------------------------------- Filter these comments with the level of rush that you're in. There's nothing hugely significant here. ::: toolkit/devtools/server/actors/inspector.js @@ +66,5 @@ > const PSEUDO_CLASSES = [":hover", ":active", ":focus"]; > > const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__"; > > +let HELPER_SHEET = "." + HIDDEN_CLASS + " { visibility: hidden !important } "; I look at this and think - "yuck, string concatenation in user interface code". I'm not going to ask you to change this, but I think that avoiding XSS comes far enough above DRY in the priority list that I'd inline HIDDEN_CLASS. But like I say not a required change - just what I'd do. @@ +846,5 @@ > > /** > + * Pick a node on click. > + */ > + _pick_deferred: null, _pickDeferred ? @@ +879,5 @@ > + let newParents = this.ensurePathToRoot(node); > + > + this._pick_deferred.resolve({ > + node: node, > + newParents: [parent for (parent of newParents)] We're supposed to be avoiding non-ES6-isms aren't we? @@ +927,5 @@ > + } > + > + LayoutHelpers.scrollIntoViewIfNeeded(node.rawNode); > + DOMUtils.addPseudoClassLock(node.rawNode, ":-moz-devtools-highlighted"); > + this._highlightTimeout = setTimeout(this._unhighlight.bind(this), 2000); Magic number

(In reply to Joe Walker [:jwalker] from comment #7) > Comment on attachment 800854 [details] [diff] [review] > move LayoutHelpers to toolkit/ > > Review of attachment 800854 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm assuming these are the same files. They are. Thanks a lot for the review Joe. I'll address your comments.