Closed
Bug 912915
Opened 11 years ago
Closed 11 years ago
Implement a simple generic highlighter
Categories
(DevTools :: Inspector, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(5 files, 1 obsolete file)
26.79 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
12.64 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
24.82 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
2.92 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
66.47 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Comment 2•11 years ago
|
||
Maybe extra methods on the Inspector actor?
I don't mind either way, approach looks fine to me.
Assignee | ||
Updated•11 years ago
|
Summary: Implement a simple highlighter for B2G → Implement a simple generic highlighter
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #800160 -
Attachment is obsolete: true
Attachment #800160 -
Flags: feedback?(dcamp)
Attachment #800854 -
Flags: review?(jwalker)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #800855 -
Flags: review?(jwalker)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #800857 -
Flags: review?(jwalker)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #800858 -
Flags: review?(jwalker)
Comment 7•11 years ago
|
||
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.
Attachment #800854 -
Flags: review?(jwalker) → review+
Updated•11 years ago
|
Attachment #800855 -
Flags: review?(jwalker) → review+
Comment 8•11 years ago
|
||
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
Attachment #800857 -
Flags: review?(jwalker) → review+
Updated•11 years ago
|
Attachment #800858 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•