Closed Bug 912915 Opened 7 years ago Closed 7 years ago

Implement a simple generic highlighter

Categories

(DevTools :: Inspector, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Blocks: appmgr_v1
Attached patch patch.diff (obsolete) — Splinter Review
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #800160 - Flags: feedback?(dcamp)
Maybe extra methods on the Inspector actor?

I don't mind either way, approach looks fine to me.
Depends on: 913440
Summary: Implement a simple highlighter for B2G → Implement a simple generic highlighter
Attachment #800160 - Attachment is obsolete: true
Attachment #800160 - Flags: feedback?(dcamp)
Attachment #800854 - Flags: review?(jwalker)
Attachment #800855 - Flags: review?(jwalker)
Attached patch actual codeSplinter Review
Attachment #800857 - Flags: review?(jwalker)
Attached patch testSplinter Review
Attachment #800858 - Flags: review?(jwalker)
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+
Attachment #800855 - Flags: review?(jwalker) → review+
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+
Attachment #800858 - Flags: review?(jwalker) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/7015fcdd43a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.