916443 - Move the highlighter to the toolbox level and make it remote

Status: VERIFIED FIXED

(DevTools :: General, defect)

Comment 1

[Luke]

So this would make it accessible from debugger's inspect-variable-elements, and references in console's inspector, for example?

Comment 2

[Paul Rouget [:paul]]

I won't be working on that.

Comment 3

[Paul Rouget [:paul]]

Moving the highlighter code and the selection code at the toolbox level is not hard.

There's one issue though: the infobar comes with a dropdown menu that is tight to the inspector. And the inspector is not always loaded.

Because we want to kill this menu anyway (highlighter v4 won't show the infobar when the highlighter is locked), I think we should just get rid of all the inspector-specific code from the highlighter.

Comment 4

[Patrick Brosset <:pbro>]

So here is what I think the required changes are:

- Move the highlighter and selection code at /browser/devtools/ level, probably in their own sub-folder.
- Remove the code in the inspector that instantiates them, and do this in the toolbox instead.
- Change all tools that rely on `inspector.highlighter` to instead get a reference via the toolbox.
- Move the `inspector.showNodeMenu` related code to the toolbox, unless we do as Paul said: use this bug to also kill the infobar drop down menu, in which case, we just remove the code.
- Update all tests that access `inspector.highlighter` (same for selection).
- Remove the highlighter icon from the inspector toolbox and put it instead one level up in the main toolbox toolbar (next to the scratchpad, paint flashing, ... icons): see here http://cl.ly/image/3a0v1V293Z0W

Comment 5

[Paul Rouget [:paul]]

(In reply to Patrick Brosset from comment #4)
> So here is what I think the required changes are:
> 
> - Move the highlighter and selection code at /browser/devtools/ level,
> probably in their own sub-folder.

/framework/ is usually where we put toolbox related stuff.

> - Remove the code in the inspector that instantiates them, and do this in
> the toolbox instead.

Yes. Instead of inspector -> highlighter, inspector -> toolbox -> highlighter (same for selection).

> - Change all tools that rely on `inspector.highlighter` to instead get a
> reference via the toolbox.

Yep.

> - Move the `inspector.showNodeMenu` related code to the toolbox, unless we
> do as Paul said: use this bug to also kill the infobar drop down menu, in
> which case, we just remove the code.

Not sure how easy it would be to move the code menu code at the toolbox level.

> - Update all tests that access `inspector.highlighter` (same for selection).
> - Remove the highlighter icon from the inspector toolbox and put it instead
> one level up in the main toolbox toolbar (next to the scratchpad, paint
> flashing, ... icons): see here http://cl.ly/image/3a0v1V293Z0W

Sounds good to me. Also - do you think any of this will require using inspector front outside of the inspector panel?

Comment 6

[Patrick Brosset <:pbro>]

Attachment: WIP patch

This is a WIP patch that shows what it would take to move both the highlighter and selection at toolbox level.

As you can see, it's not very hard. The highlighter requires the selection because it listens to its events to highlight nodes, selection, in turn, requires a walker for various things, which itself is retrieved via an inspectorFront.

This WIP was used as a basis for discussion with paul which led to the following conclusions:

- the highlighter should indeed be at toolbox level, so that any tool can highlight nodes without having to load the inspector
- the selection mechanism however should remain at inspector level though. Today it's only possible to highlight by selecting a node, which isn't always needed. And if selection is required, anyway, the inspector should be shown to display that selection, so inspector and selection are coupled.
- the highlighter should be remote and expose an API that lets any tool that has an highlighterFront highlight a node.
- that means the LocalHighlighter class needs to go
- the inspector-panel (client-side) should now retrieve a highlighterFront and use that to highlight nodes when the become selected.

So, overall, a bigger task than what was envisioned, and one that basically starts with remoting the highlighter first.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug916443-highlighter-in-toolbox.patch

Hey guys, this is a WIP patch (not to be reviewed or checked in) to make the highlighter remote.

- The old highlighter file has been deleted
- A new highlighter actor file was created
- The new actor implements a showBoxModel method (the only thing that works for now) which is used by the client-side to highlight a node's box model
- Most of the old highlighter code is now in a new BoxModelHighlighter class which is used by the actor to draw the outline + infobar thingies.
- Just for testing things out, the markup-view now listen for mousemove and mouseleave on its container element and uses that to request box model highlighting to the highlighterFront. The way it works is that it highlights nodes (only those that are more than 0px wide and high, to skip head, script, ...) on mousemove and remove the highlighter as soon as you leave the markup view.

This is just a work in progress patch with lots of rough edges. I just wanted to have your early feedback on it as this is something I've never done so far and might missing important things.

Please take a quick look at it when you have some time.

Next steps are:
- Make the inspector button work again. Now if you click on it, nothing happens anymore. I have added an "inspect" method to the actor which does what the old highlighter was doing: listening for mousemove events (mainly) on the content browser to highlight hovered elements.
- Move the inspector button at toolbox level so it's always available.
- When inspecting the page, a click should switch to the inspector (loading it if not done so far) and select the element in the markup-view.

Comment 9

[Patrick Brosset <:pbro>]

Darrin, in order to move the inspect command to the toolbox toolbar, I'd need a 4-states icon for it, just like we have for the tilt, responsive mode or paint flashing. You can see these 3 icons here: https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/

As for the inspect icon, for now we only a 2 states one : https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/inspect-button.png
so we're missing the 2, slightly brighter, intermediary states which, I think, correspond to when the icon is :hover and :active.

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 832903 [details] [diff] [review]
bug916443-highlighter-in-toolbox.patch

BoxModelHighlighter needs to work for targets that are not Firefox tabs (Firefox OS, browser.xul, Fennec, …). You'll need to move the pick and highlight methods from the walker code to highlighter.js.

You need to implement stopInspecting(). The user might want to cancel inspection by clicking the inspect button again.

What about implementing showContentOutline too? Not sure if it's needed or not (I haven't followed the discussion with UX about highlighter v4).

Swap events.emit("inspector-mousemove") and "inspector-mouseclick".

We'll need to figure out how to make that compatible with the basic highlighter (we will use Firefox 29 to debug Firefox OS 1.2 (Gecko 26)).

I'd like to add that getting the API right is very important. It won't be easy to change it (I keep thinking about Firefox OS).

There's still a lot of work, but that's the right direction. Thank you for looking at this. Let me know if you need any help.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 832903 [details] [diff] [review]
bug916443-highlighter-in-toolbox.patch

Review of attachment 832903 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anything that Paul hasn't already noticed.

I take it that clicking on inspect is not supposed to work yet, just highlight on hover?

Comment 12

[Patrick Brosset <:pbro>]

Thanks Paul and Mike for the feedback! That helps.
I had no idea that for fennec and fxos we had no way to append our current XUL-based highlighter to some parent node of the browser. That means we must use the simple highlighter you did, Paul, for fxos with the pseudo-class.
As you said, though, I will move it to the new remote highlighter.

Comment 13

[Patrick Brosset <:pbro>]

I'm progressing on this bug and expect to attach a new WIP patch by the end of the week.
I managed to make things work for remote targets, both for desktop firefox and fxos simulator, and across versions. Meaning that the highlighter now works with fxos simulator 1.2.

I realized one thing, highlighting is one thing, and picking a node (inspecting) is another thing. While highlighting has no dependency on other devtools actors, inspector does, indeed, when a node is clicked on, we need to wrap it into a NodeActor, and for this, we need the WalkerActor that is attached to the InspectorActor.

So, my next step now is to separate these 2 things by keeping the simple show/hide highlighter logic in the new highlighter actor, but keeping the picking logic in the walker actor, meaning that the pick button would still be dependent on the inspector panel (anyway, it makes no sense to allow picking elements without switching to the inspector panel). And the walker actor would depend on the highlighter actor, but not the other way around.

This way, we'd still allow other tools to highlight elements without depending on the inspector.

@paul: I'd like your point of view on this. Let's chat over IRC if I didn't make myself clear enough here.

Comment 14

[Paul Rouget [:paul]]

> it makes no sense to allow picking elements without switching to the inspector panel

I think this is not true. We want to be able to pick nodes from the webconsole for example.

(we talked on IRC)

Comment 15

[Patrick Brosset <:pbro>]

> (we talked on IRC)

Here is what we discussed:

it would be useful to still be able to pick/select nodes outside of the inspector. Future new tools may need this. The webconsole is a good example, it may be useful to set the value of the $0 shortcut for instance.

- This means the pick button must stay in the toolbox and should not be used to switch to the inspector ui
- Both the inspector front and walker front instantiation should move up to the toolbox, just like the highlighter.
- The toggle picker functions should be at toolbox level too.
- It could make sense to lazily load the fronts on the first button click

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug916443-highlighter-in-toolbox-WIP-for-dcamp.patch

Dave, as discussed on IRC, here is the WIP patch for you to take a look.

Comment 17

[Patrick Brosset <:pbro>]

For info, here are some screencast of the current state: http://people.mozilla.org/~pbrosset/inspector/

Read before you watch:
- "old" means remote debugging an older version of firefox desktop (one that doesn't have the new highlighter actor).

- "new" means remote debugging the same version of firefox desktop

- The highlighter is now shown on hover of the markup-view. If debugging an "old" target, the pink dashed line is used. If debugging a "new" target, the normal highlighter is used.

- The inspect button is now at toolbox level. If debugging an "old" target, it allows to simply click (or touch) an element in the page to select it, but nothing is shown on mouseover. Debugging a "new" target also shows the highlighter on mouseover.

- Clicking the inspect button doesn't switch to the inspector, as shown in the webconsole screencast.

Comment 18

[Paul Rouget [:paul]]

\o/ Ça déchire :)

Comment 19

[Darrin Henein [:darrin]]

Attachment: Icon Graphics

This icon set should match what we have right now, if I could see some screenshots of it once it's in there, I can +1 the UI.

Comment 20

[Paul Rouget [:paul]]

(In reply to Darrin Henein [:darrin] from comment #19)
> Created attachment 8336823 [details]
> Icon Graphics
> 
> This icon set should match what we have right now, if I could see some
> screenshots of it once it's in there, I can +1 the UI.

Can we get a 2x version? Or should we wait until all the icons have a 2x version?

Comment 21

[Patrick Brosset <:pbro>]

Attachment: normal/hover/active/checked states of the new icon provided by Darrin

Darrin, here is your icon, in the toolbox.
From top to bottom: normal state, hovered, clicked, checked.

I'll try to provide a usable try build soon, so you can play with it yourself.

Comment 22

[Patrick Brosset <:pbro>]

Also, I just checked on my retina display, we do not have 2x versions for our toolbox icons, so they don't look as good as they could. Perhaps this is something to be done within bug 916766 along with the other design changes?

Comment 23

[Paul Rouget [:paul]]

See bug 837188 (for retina icons)

Comment 24

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V1 (NO TESTS)

Comment 25

[Patrick Brosset <:pbro>]

The previous patch deserves some explanations:

- There's now a new remote highlighter actor/front
- The highlighter is responsible for showing the box model on any node
- The markup-view uses the highlighter to show the box model when hovering the nodes in the markup tree view
- The markup-view uses the old walker.highlight method if the new highlighter actor isn't present on the debuggee
- There's no locked state anymore.
- The various "locked" event listeners in inspector's panels were removed.
- The inspect button is gone from the inspector and moved to the toolbox toolbar. When clicked, it doesn't switch to the inspector, so other tools can use it for their own purpose.
- The inspector actor/front, walker actor/front and selection class are loaded and lazy-init at toolbox level now (Inspector/walker/selection are only instantiated the first time the inspector panel is shown or the first time the pick button is used).
- When the inspect button is clicked, this triggers the "pick" mode in the toolbox which calls the walker's pick method. This method was here before and was just re-written here, so it will be backward compatible with older debuggees.
- The new walker's pick method basically listens for mousemmove and clicks on the page and tells the front via events and a response when nodes are hovered and clicked. The toolbox, in turn, uses this to highlight the box mode and set the current node selection.

A few details:

- Removed 2 promises rejections in rule and computed views as they cause console errors when picking and moving the mouse quickly on the page + these errors aren't really necessary it seems since we can quietly ignore them.
- Fixed the $0 in the webconsole so that it is set when inspecting from the console
- Picking an element from the page leaves the highlighter for 1 second before hiding it

Comment 26

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V2 (NO TESTS)

Tiny last minute change to remove unneeded code

Comment 27

[Patrick Brosset <:pbro>]

What is missing now:

- There's a remaining bug that occurs when, in pick mode, hovering over the page : The events fired by the walker actor seem to have missing parents, ending up breaking the markup view and breadcrumbs widget. dcamp is needinfo'd on the bug for this.
- No new tests have been written yet and many existing tests will fail. I will work on the tests in another patch file to ease review.

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Patrick Brosset from comment #25)
> - Fixed the $0 in the webconsole so that it is set when inspecting from the
> console

Does that mean that it will fix bug 787975?

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Panos Astithas [:past] from comment #28)
> (In reply to Patrick Brosset from comment #25)
> > - Fixed the $0 in the webconsole so that it is set when inspecting from the
> > console
> 
> Does that mean that it will fix bug 787975?

Unfortunately not with my patch. I could work on this though.

If I'm not mistaken, the problem is that $0 access gDevTools.getToolbox(target) which basically gets the client-side toolbox instance. And then from there, it access selection.node which isn't remote either (it references the actual DOM node currently selected). $0 should instead be accessing the currently selected node on the server-side only. However, as of now, this notion does not exist. Only the client-side knows about the selected node.

Comment 30

[Patrick Brosset <:pbro>]

If you want to play with a build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3b947768906f/try-macosx64/
https://tbpl.mozilla.org/?tree=Try&rev=3b947768906f

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8335340 [details] [diff] [review]
bug916443-highlighter-in-toolbox-WIP-for-dcamp.patch

No need for dcamp's advice on this anymore, I found what was the problem and it wasn't really directly linked to the client-server protocol. It happened that concurrent requests were sent to the server: 
while a request was still being processed (not responded to yet), another request was coming in and, of course, couldn't be responded to since the first one wasn't done.
Fixed now.

Comment 32

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V3 (NO TESTS)

Starting to work on the tests showed me a few more bugs that are now fixed in this v3 patch.

This doesn't mean tests are part of this patch, I'll attach a new one soon.

Comment 33

[Paul Rouget [:paul]]

Comment on attachment 8338550 [details] [diff] [review]
bug 916443 - remote highlighter and toolbox-level picker V3 (NO TESTS)

This is a big patch.

Before reviewing the code, here are some comments about the new workflow:

- when a node is selected from the markup view, we should scroll the page to make sure it's in the viewport
- the infobar needs a min-width
- when a node is selected but not from the markup view (breadcrumbs, searchbox or page context menu), we should show the highlighter for ~1s (just get some feedback, like we do on mobile)
- I get this error a lot in the console: `this.rootDoc is null inspector.js:1016`
- sometimes, I get 2 highlighters: http://i.imgur.com/0kFac3s.jpg (I think it happens when I used the Cmd-Shift-I shortcut multiple times)
- Cmd-Shift-C doesn't work anymore
- when the "inspector" button is pressed, hovering a node should not select it, but just highlight it (and use the dark blue color in the markup view). It should behave like if you were hovering nodes in the markup view. Mostly because selecting on hover will update the Rule View & co, and this is very slow
- not sure if just me, but having this "inspect" button on the far right when it was on the far left is very very disturbing

Comment 34

[Patrick Brosset <:pbro>]

Thanks. Some replies inline:

> - when a node is selected from the markup view, we should scroll the page to
> make sure it's in the viewport
Indeed, that works with the current highlighter, so I must have broken the code that did that.

> - the infobar needs a min-width
It does have one. If you select a node that has no class or ID, the min-widget makes sure it still looks ok.
I will attach a screenshot. Do you have a screenshot where this looked wrong?

> - when a node is selected but not from the markup view (breadcrumbs,
> searchbox or page context menu), we should show the highlighter for ~1s
> (just get some feedback, like we do on mobile)
Good point, since we don't have the lock state anymore, I guess it makes sense to give some visual feedback indeed.

> - I get this error a lot in the console: `this.rootDoc is null
> inspector.js:1016`
I don't recall having had that error. I'll check.

> - sometimes, I get 2 highlighters: http://i.imgur.com/0kFac3s.jpg (I think
> it happens when I used the Cmd-Shift-I shortcut multiple times)
Good catch. I need to make sure the highlighter is destroyed.

> - Cmd-Shift-C doesn't work anymore
Works now in a slightly newer version of the patch I have locally. Will attach soon.

> - when the "inspector" button is pressed, hovering a node should not select
> it, but just highlight it (and use the dark blue color in the markup view).
> It should behave like if you were hovering nodes in the markup view. Mostly
> because selecting on hover will update the Rule View & co, and this is very
> slow
I kind of liked the fact that the whole inspector was refreshing as you hovered nodes. That makes it pretty useful, and I haven't seen problems with it being slow so far, but it's a good point I guess.
I'll try as you said.

> - not sure if just me, but having this "inspect" button on the far right
> when it was on the far left is very very disturbing
Will need some UX feedback on this. I will needinfo Darrin on the bug.

Comment 35

[Patrick Brosset <:pbro>]

Attachment: node-info-bar-min-width.png

Here is what the nodeinfo bar looks like on a simple <a> tag with no attributes.
The current min-width is around 25px if I recall well. This value was chosen to avoid cases where the nodeinfo bar would be thinner than the arrow.

Comment 36

[Patrick Brosset <:pbro>]

> - when the "inspector" button is pressed, hovering a node should not select
> it, but just highlight it (and use the dark blue color in the markup view).
> It should behave like if you were hovering nodes in the markup view. Mostly
> because selecting on hover will update the Rule View & co, and this is very
> slow
Actually, I just realized it already works this way today. When unlocked, both the markup-view and style inspector will refresh as you hover nodes.

Comment 37

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V4 (NO TESTS)

Addresses points raised by Paul in comment 33.

- The page is scrolled when nodes are selected in the markup view or breadcrumb
- The box model stays visible for 1s after selection
- The multiple highlighter bug is gone
- Cmd-Shift-C works now
- When in "pick" mode, hovering over nodes in the page just shows these nodes in the markup view as if they were hovered, but does not select them and does not refresh the rule/computed views or breadcrumbs.

Comment 38

[Darrin Henein [:darrin]]

> > - not sure if just me, but having this "inspect" button on the far right
> > when it was on the far left is very very disturbing
> Will need some UX feedback on this. I will needinfo Darrin on the bug.

I agree, this is a significant change for those who are used to it, but the right side toolbar is the right place for it. It is now a 'global' tool and as such should live with the rest of the similar tools. I think aside from the 'its a change' reaction, the decision is sound.

Comment 39

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V5 (NO TESTS)

More fixes!
Mostly fixing small functional issues like showing the highlighter for 1 second when the current selection changes, scrolling the node into view, making sure the highlighter is animated, ...

Comment 40

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V1 (TESTS)

Here is the first version for the TEST patch.
There are still many problems to be addressed, but most failing tests are now resolved.

A couple of them still fail, I'm investigating this now.
And a lot of JS errors are thrown in the console which I think is due to a non-finished server-side request when the toolbox is destroyed (when the test ends).

When these 2 points are done with, I'll also add new test cases to the mix.

Comment 41

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V6 (NO TESTS)

Some more fixes mainly to make tests pass.

Comment 42

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V7 (NO TESTS)

Final fixes. Again due to fixing tests.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V2 (TESTS)

@Heather: Do you think you'd have time to apply these 2 patches and help me out on the test part?
So far, all tests pass except one: browser_inspector_pseudoclass_lock.js, and only when run within the inspector test suite (it passes when run individually).

I'm asking because it's related to the rule-view and I thought you might be able to help me with this.

The test toggles a :hover pseudo-class a few times on an element and checks if there are the right number of rules shown in the rule-view panel.
When I run the test on its own, it goes fine. When running the whole inspector test suite, the applied styles retrieved from the server contain too many entries.

I've been trying to find the problem for some time now with no luck so far, and I don't see how this can be related to this bug.

Thanks!

Comment 44

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V8 (NO TESTS)

Rebased and fixed a minor bug after testing with remote fennec

Comment 45

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V3 (TESTS)

Rebased test patch too.

Comment 46

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V9 (NO TESTS) bug916443-highlighter-in-toolbox.patch

Rebased once again.

Comment 47

[Patrick Brosset <:pbro>]

Comment on attachment 8343739 [details] [diff] [review]
bug 916443 - remote highlighter and toolbox-level picker V3 (TESTS)

I finally was able to find the reason for the remaining test failure.
I'm therefore cleaning up the inspector tests a bit, and will be attaching a new version of the test patch soon.

Comment 48

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V4 (TESTS)

Should fix all remaining failing tests.
First try build push: https://tbpl.mozilla.org/?tree=Try&rev=d7548cb4ecc0

Comment 49

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V5 (TESTS)

Fixed typo in the tests patch ...
And pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=b8dbef74034b

Comment 50

[Paul Rouget [:paul]]

Comment on attachment 8345255 [details] [diff] [review]
bug 916443 - remote highlighter and toolbox-level picker V9 (NO TESTS) bug916443-highlighter-in-toolbox.patch

Still looking…

2 comments:

The infobar min-width needs to be 3 times larger.

(In reply to Darrin Henein [:darrin] from comment #38)
> > > - not sure if just me, but having this "inspect" button on the far right
> > > when it was on the far left is very very disturbing
> > Will need some UX feedback on this. I will needinfo Darrin on the bug.
> 
> I agree, this is a significant change for those who are used to it, but the
> right side toolbar is the right place for it. It is now a 'global' tool and
> as such should live with the rest of the similar tools. I think aside from
> the 'its a change' reaction, the decision is sound.

I understand the theory, but…

How many people use the button on the top right corner? A minority. How many people use the inspect button? A majority. Firebug inspect button is on the left. Chrome inspect button too (but bottom left).

Let's find a way to move it to the left. Maybe we can move ALL the buttons to the left.

Comment 51

[Paul Rouget [:paul]]

After F5, only the infobar is showed, not the outline.

Comment 52

[Paul Rouget [:paul]]

I see these errors in the terminal (nothing in the console):

> console.error:
>   noSuchActor
> console.error:
>   noSuchActor

Comment 53

[Paul Rouget [:paul]]

I think the transition-timing property of the infobar is not the same as the outline (infobar:ease outline:linear. I'm not sure, didn't look at the CSS code).

Comment 54

[Paul Rouget [:paul]]

(In reply to Paul Rouget [:paul] from comment #52)
> I see these errors in the terminal (nothing in the console):
> 
> > console.error:
> >   noSuchActor
> > console.error:
> >   noSuchActor

Happens when closing Firefox.

Comment 55

[Paul Rouget [:paul]]

Comment on attachment 8345255 [details] [diff] [review]
bug 916443 - remote highlighter and toolbox-level picker V9 (NO TESTS) bug916443-highlighter-in-toolbox.patch

Still reviewing…

>-.highlighter-nodeinfobar-button > .toolbarbutton-text {
>-  display: none;
>-}
>-
>-.highlighter-nodeinfobar-container:not([locked]):not(:hover) > .highlighter-nodeinfobar > .highlighter-nodeinfobar-button {
>-  visibility: hidden;
>-}
>-
>-.highlighter-nodeinfobar-container[locked] > .highlighter-nodeinfobar,
>-.highlighter-nodeinfobar-container:not([locked]):hover > .highlighter-nodeinfobar {
>+.highlighter-nodeinfobar-container > .highlighter-nodeinfobar {
>   pointer-events: auto;
> }

Why?

>+    // Destroying the highlighter. Note that if the server connection has
>+    // already been cut (closing browser), this will send a noSuchActor message
>+    // but we don't want this to prevent the rest of the destroy process to be
>+    // carried out

How can we avoid that?


>diff --git a/browser/themes/linux/devtools/command-pick.png b/browser/themes/linux/devtools/command-pick.png

move command-pick.png in /shared/

Comment 56

[Paul Rouget [:paul]]

>-.highlighter-nodeinfobar-button > .toolbarbutton-text {
>-  display: none;
>-}
>-
>-.highlighter-nodeinfobar-container:not([locked]):not(:hover) > .highlighter-nodeinfobar > .highlighter-nodeinfobar-button {
>-  visibility: hidden;
>-}
>-
>-.highlighter-nodeinfobar-container[locked] > .highlighter-nodeinfobar,
>-.highlighter-nodeinfobar-container:not([locked]):hover > .highlighter-nodeinfobar {
>+.highlighter-nodeinfobar-container > .highlighter-nodeinfobar {
>   pointer-events: auto;
> }

(a line was missing)

Why?
All the highlighter should be pointer-events:none now.

Comment 57

[Paul Rouget [:paul]]

Patrick, can I ask you to share some more details about: isHighlightable, pick() and pick2()? I'm not sure to understand that correctly. isHighlightable mean is highlightable with the new code, pick() will use the older highlighter, pick2 will use the new highlighter?

Do I get it right?

Comment 58

[Paul Rouget [:paul]]

Patrick, can you look at how much work it would require to support e10s?

We don't have to support e10s in this specific patch, but I think we should at least be aware of how we will build a cross-process highlighter. I don't want to invalidate all this work in 2 weeks because we didn't consider e10s.

I you need any help with e10s, ask me, or read this:
https://developer.mozilla.org/en-US/docs/The_message_manager
http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/
http://billmccloskey.wordpress.com/2013/12/05/multiprocess-firefox/

Also, see: https://mail.mozilla.org/pipermail/firefox-dev/2013-December/001284.html

Comment 59

[Patrick Brosset <:pbro>]

Paul and I discussed e10s' impacts over IRC:
- It seems like there's no parent XUL node to append the current highlighter to in the child process (where the highlighter actor lives).
- So we can either change course and implement the highlighter at gecko level entirely (which would be really nice as it would make it look the same on Fennec and FxOS too).
- Or make the highlighter actor send messages via the MsgManager to the parent process which will, in turn, attach the highlighter in the <deck> of the <tabbrowser> as today. This, however, will require the highlighter actor to constantly send messages to inform the parent process of position changes (on paint events).

Comment 60

[Girish Sharma [:Optimizer]]

Just wanted to put it as a note here that the style editor css autocompletion is also dependent on this bug. I t would be really nice if this can get landed in 29th release only :)

Comment 61

[Patrick Brosset <:pbro>]

(In reply to Paul Rouget [:paul] from comment #50) 
> The infobar min-width needs to be 3 times larger.
Changed. It looks indeed better.

(In reply to Paul Rouget [:paul] from comment #51)
> After F5, only the infobar is showed, not the outline.
Fixed.

(In reply to Paul Rouget [:paul] from comment #55)
> >+    // Destroying the highlighter. Note that if the server connection has
> >+    // already been cut (closing browser), this will send a noSuchActor message
> >+    // but we don't want this to prevent the rest of the destroy process to be
> >+    // carried out
> 
> How can we avoid that?
The noSuchActor errors in the console are thrown by the server when a packet that has no associated actor (anymore) is received. Only one of them is caused by the highlighter actor (the other one might be due to the walker).
This is the case when the toolbox is open and you close Firefox. This causes the toolbox's destroy function to be called, which tries to send packets to the server which is, apparently, already partly destroyed.

If however you keep Firefox open but close the toolbox, the same function will be called and will work since the server isn't being destroyed.

I use this to remove the markup of the highlighter. So if I stop doing it to avoid these errors, toggling the devtools over and over again in the same tab will append more and more highlighter to the <deck>. This might not be a big problem after all.

(In reply to Paul Rouget [:paul] from comment #53)
> I think the transition-timing property of the infobar is not the same as the
> outline (infobar:ease outline:linear. I'm not sure, didn't look at the CSS
> code).
Both the infobar and outline share the same transitions:
  transition-property: opacity, top, left, width, height;
  transition-duration: 0.1s;
  transition-timing-function: linear;
Did you have the impression that they looked different? I wouldn't focus too much on this in this bug since this will most probably change with bug 663778.

> >diff --git a/browser/themes/linux/devtools/command-pick.png b/browser/themes/linux/devtools/command-pick.png
> 
> move command-pick.png in /shared/
Ok, I put it there because all other command-*.png icons are there. Plus, /shared/ doesn't have a jar.mn file. How am I supposed to reference the image so I can access it in the css files?

(In reply to Paul Rouget [:paul] from comment #56)
> >-.highlighter-nodeinfobar-button > .toolbarbutton-text {
> >-  display: none;
> >-}
> >-
> >-.highlighter-nodeinfobar-container:not([locked]):not(:hover) > .highlighter-nodeinfobar > .highlighter-nodeinfobar-button {
> >-  visibility: hidden;
> >-}
> >-
> >-.highlighter-nodeinfobar-container[locked] > .highlighter-nodeinfobar,
> >-.highlighter-nodeinfobar-container:not([locked]):hover > .highlighter-nodeinfobar {
> >+.highlighter-nodeinfobar-container > .highlighter-nodeinfobar {
> >   pointer-events: auto;
> > }
> 
> (a line was missing)
> 
> Why?
> All the highlighter should be pointer-events:none now.
Sure, fixed now.

(In reply to Paul Rouget [:paul] from comment #57)
> Patrick, can I ask you to share some more details about: isHighlightable,
> pick() and pick2()? I'm not sure to understand that correctly.
> isHighlightable mean is highlightable with the new code, pick() will use the
> older highlighter, pick2 will use the new highlighter?
> 
> Do I get it right?
Your understanding is correct. isHighlightable is a new server trait to let the client know if the current server supports the new remote highlighter. I probably need to find a better name though. Maybe isRemoteHighligtable? Doesn't sound good.
As for pick and pick2, there's a reasoning behind this name. pick and pick2 are doing the same thing, and I first wanted them to share the same API and therefore just use the same name, so that a new toolbox would be able to call either the old (current) pick or the new one by using the same actor method. But since the functionality is different, I wasn't able to make them have the same API/behavior, that's why I decided to call the new one pick2, as a new "version" of pick.
I kept the older pick method signature in the inspector actor so that it would be in the front, and a toolbox UI would be able to call it when connected to an older server (say on FxOS 1.2 for instance).

(In reply to Paul Rouget [:paul] from comment #58)
> Patrick, can you look at how much work it would require to support e10s?
> 
> We don't have to support e10s in this specific patch, but I think we should
> at least be aware of how we will build a cross-process highlighter. I don't
> want to invalidate all this work in 2 weeks because we didn't consider e10s.
> 
> I you need any help with e10s, ask me, or read this:
> https://developer.mozilla.org/en-US/docs/The_message_manager
> http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/
> http://billmccloskey.wordpress.com/2013/12/05/multiprocess-firefox/
> 
> Also, see:
> https://mail.mozilla.org/pipermail/firefox-dev/2013-December/001284.html
As written in comment 59, we've started a discussion that needs to be continued.

Comment 62

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] from comment #61)
> > >diff --git a/browser/themes/linux/devtools/command-pick.png b/browser/themes/linux/devtools/command-pick.png
> > 
> > move command-pick.png in /shared/
> Ok, I put it there because all other command-*.png icons are there. Plus,
> /shared/ doesn't have a jar.mn file. How am I supposed to reference the
> image so I can access it in the css files?
Oh I got it, sorry, ignore my comment.

Comment 63

[Paul Rouget [:paul]]

Looking at the highlighter actor:

>     // See if we can create the BoxModelHighlighter, that means we can reach the
>     // browser parentNode to attach the new XUL markup.
>     // If that fails, revert to the simpler outline highlighter
>     try {
>       this._boxModelHighlighter = new BoxModelHighlighter(this._browser);
>     } catch (e) {
>       this._boxModelHighlighter = new SimpleOutlineHighlighter(this._browser);
>     }

Can you find a more explicit way to figure out which one to use?

~

Is highlighter-controls still useful? At least, update the comment:

>    // The controlsBox will host the different interactive
>    // elements of the highlighter (buttons, toolbars, ...).

~

>     this.boundCloseEventHandler = null;

Not used anymore.

~

> this.highlighterContainer.parentNode.removeChild(this.highlighterContainer);

this.highlighterContainer.remove();

~

I think the pseudoClassesBox is not really necessary anymore. The infobar used to be the main UI for that, but now it's the markup view. For code simplicity, maybe we should remove all the pseudo-class related code?

Comment 64

[Paul Rouget [:paul]]

Looking at inspector.js:

>   /**
>    * This is kept for backward-compatibility reasons with older remote targets.
>    * Targets prior to bug 916443.
>    * […]
>    * inspect fxos 1.2 remote targets or older firefox desktop remote targets.
>    */
>   pick: method(function() {}, {request: {}, response: RetVal("disconnectedNode")}),
>   cancelPick: method(function() {}),
>   highlight: method(function(node) {}, {request: {node: Arg(0, "nullable:domnode")}}),

Can you ask dcamp to see if there's not a better a better way to work around that?

~

I'm not sure if that makes any sense, but could we move the picking mechanism in the highlighter actor?

Comment 65

[Paul Rouget [:paul]]

Comment on attachment 8345255 [details] [diff] [review]
bug 916443 - remote highlighter and toolbox-level picker V9 (NO TESTS) bug916443-highlighter-in-toolbox.patch

Alright, this is good. I'm happy with these changes. The only thing I'm not sure about is the location of the picking API.

Comment 66

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Paul Rouget [:paul] from comment #65)
...
> Alright, this is good. I'm happy with these changes. The only thing I'm not
> sure about is the location of the picking API.

Where should it be? I guess the alternatives might be:

* somewhere in the top left, in the same row as the gear icon?
* where it currently is, but persistently visible across all tools?

Comment 67

[Paul Rouget [:paul]]

(In reply to Jeff Griffiths (:canuckistani) from comment #66)
> (In reply to Paul Rouget [:paul] from comment #65)
> ...
> > Alright, this is good. I'm happy with these changes. The only thing I'm not
> > sure about is the location of the picking API.
> 
> Where should it be?

I was talking about the code location (in walker or highlighter).

About the button see comment 50.

Comment 68

[Patrick Brosset <:pbro>]

(In reply to Paul Rouget [:paul] from comment #63)
Thanks

> Looking at the highlighter actor:
> 
> >     // See if we can create the BoxModelHighlighter, that means we can reach the
> >     // browser parentNode to attach the new XUL markup.
> >     // If that fails, revert to the simpler outline highlighter
> >     try {
> >       this._boxModelHighlighter = new BoxModelHighlighter(this._browser);
> >     } catch (e) {
> >       this._boxModelHighlighter = new SimpleOutlineHighlighter(this._browser);
> >     }
> 
> Can you find a more explicit way to figure out which one to use?
Replaced the try/catch by a simple check that this._browser.parentNode exists, and moved this code to an other function.

> Is highlighter-controls still useful? At least, update the comment:
> 
> >    // The controlsBox will host the different interactive
> >    // elements of the highlighter (buttons, toolbars, ...).
Updated the code, renamed a few things.

> >     this.boundCloseEventHandler = null;
> 
> Not used anymore.
Removed

> > this.highlighterContainer.parentNode.removeChild(this.highlighterContainer);
> 
> this.highlighterContainer.remove();
Done

> I think the pseudoClassesBox is not really necessary anymore. The infobar
> used to be the main UI for that, but now it's the markup view. For code
> simplicity, maybe we should remove all the pseudo-class related code?
I think there's a use case where it's nice to have still. If you switch a node to :hover state in the markup-view for instance, and then you start the pick mode, when you hover over that node, it's nice to see that has the :hover pseudo class.

(In reply to Paul Rouget [:paul] from comment #64)
> Looking at inspector.js:
> 
> >   /**
> >    * This is kept for backward-compatibility reasons with older remote targets.
> >    * Targets prior to bug 916443.
> >    * […]
> >    * inspect fxos 1.2 remote targets or older firefox desktop remote targets.
> >    */
> >   pick: method(function() {}, {request: {}, response: RetVal("disconnectedNode")}),
> >   cancelPick: method(function() {}),
> >   highlight: method(function(node) {}, {request: {node: Arg(0, "nullable:domnode")}}),
> 
> Can you ask dcamp to see if there's not a better a better way to work around
> that?
Ok, I'll get in touch and verify if there's a way to skip this.

> I'm not sure if that makes any sense, but could we move the picking
> mechanism in the highlighter actor?
Yes that may in fact make a lot of sense. It will end up coupling the highlighter more to the inspector/walker than it is today, but I don't think this is a big problem.
I'll spend some time experimenting.

Comment 69

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - remote highlighter and toolbox-level picker V10 (NO TESTS)

Simple changes as per Paul's feedback + instantiation of the highlighter is now done via the inspector actor, just like the pageStyleActor (for instance). All the rest stays the same.

Comment 70

[Patrick Brosset <:pbro>]

Attachment: bug 916443 - pick API in remote highlighter V1

Paul, this is the move of the pick API to the highlighter.
This simplifies the client code because there's no need anymore for an extra round trip to highlight the hovered nodes.

Comment 71

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] from comment #59)
> Paul and I discussed e10s' impacts over IRC:
> - It seems like there's no parent XUL node to append the current highlighter
> to in the child process (where the highlighter actor lives).
> - So we can either change course and implement the highlighter at gecko
> level entirely (which would be really nice as it would make it look the same
> on Fennec and FxOS too).
> - Or make the highlighter actor send messages via the MsgManager to the
> parent process which will, in turn, attach the highlighter in the <deck> of
> the <tabbrowser> as today. This, however, will require the highlighter actor
> to constantly send messages to inform the parent process of position changes
> (on paint events).

I spent some more time looking into this and created bug 950712 as a result.
The changes required will be better done in a new bug.

Comment 72

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part1 V11.patch

Comment 73

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part2 V2.patch

Comment 74

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part3 V6.patch

Comment 75

[Patrick Brosset <:pbro>]

Joe, as discussed, it would be great if you could take a look at these patches.
I've made quite a bit of progress on the test part, fewer and fewer of them fail, but there are still some that fail on try but not locally (even in debug).

The last try build was: https://tbpl.mozilla.org/?tree=Try&rev=a901c9d3c3b3

And I just re-launched a new one after I fixed something in test browser_ruleview_override.js: https://tbpl.mozilla.org/?tree=Try&rev=e470dc673978
(the 3 patches just attached contain this fix).

Comment 76

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part1 V12.patch

Changes since last R+ on this patch are:

- rebased since there was a change made to the destroy inspector part which wasn't executed when walker.release failed (my patch moved that part of the code at toolbox level)
- simplified the highlighter destroy mechanism since the highlighter is now instantiated by the inspector actor
- fixed a couple of bugs in the SimpleOutlineHighlighter class (storing the currently outlined element so that it can easily be unhighlighted, instead of using querySelector which failed when iframes got in the way)

Comment 77

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part2 V3.patch

Part 2.
Changes made to this patch since the last R+:

- just hiding the box model in the highlighterActor.cancelPick method
- and refactored a bit the mutationobserver part in separate functions

Comment 78

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part3 V7.patch

part 3, tests.
Changes since last R+: fixed a few more tests that were failing on try, mostly due to them not waiting for the right events.

Comment 79

[Patrick Brosset <:pbro>]

The previous try build was almost green: https://tbpl.mozilla.org/?tree=Try&rev=e470dc673978
Apart from some unrelated failures, there was one test that failed, which hopefully should not be fixed with the new test patch.
New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=88b19214f704

Comment 80

[Paul Rouget [:paul]]

Tested with Firefox OS, I get this error:

I/Gecko   ( 1996): console.error:
E/GeckoConsole( 1996): Content JS ERROR at resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:818 in Actor<.writeError: TypeError: aTopLevelWindow is undefined
I/Gecko   ( 1996):   Message: TypeError: aTopLevelWindow is undefined
I/Gecko   ( 1996):   Stack:
I/Gecko   ( 1996):     LayoutHelpers@resource://gre/modules/devtools/LayoutHelpers.jsm:19
I/Gecko   ( 1996): HighlighterActor<.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:44

Comment 81

[Paul Rouget [:paul]]

Comment on attachment 8350547 [details] [diff] [review]
bug916443-remote-highlighter-part1 V12.patch

>+let HighlighterActor = protocol.ActorClass({
>+  typeName: "highlighter",
>+
>+  initialize: function(inspector) {
>+    protocol.Actor.prototype.initialize.call(this, null);
>+
>+    this._inspector = inspector;
>+    this._walker = this._inspector.walker;
>+    this._browser = this._inspector.tabActor.browser;


No browser in Firefox OS.

>+    this._layoutHelpers = new LayoutHelpers(this._browser.contentWindow);
>+
>+    if (this._supportsBoxModelHighlighter()) {
>+      this._boxModelHighlighter = new BoxModelHighlighter(this._browser);
>+    } else {
>+      this._boxModelHighlighter = new SimpleOutlineHighlighter(this._browser);
>+    }
>+  },


The simple highlighter depends on browser, and there's no browser on Firefox OS. Just the content window. You can only rely on the the window.

Comment 82

[Patrick Brosset <:pbro>]

Thanks Paul for the review. Since I was mostly focused on making sure the new highlighter was backward compatible with older targets, I didn't think of testing with a patched fxos and hence didn't see this problem.
I'll do that now and hopefully submit a new patch for review soon.

Comment 83

[Patrick Brosset <:pbro>]

Darrin, we now use @2x images for the toolbox, so I'd need this image: https://bug916443.bugzilla.mozilla.org/attachment.cgi?id=8336823 but in @2x
Thanks a lot!

Comment 84

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part1 V13.patch

This new patch is rebased against the latest fx-team and is also the result of folding the previously named part1 and part2 patches.
The boundary between these 2 patches wasn't clear anymore, so I merged them.

Other than that, this patch is the same as before except for one important change:
- The simpleOutlineHighlighter is not compatible with FxOS in that it doesn't rely on a browser object anymore. I've tested this by patching omni.ja on the device as discussed with Paul.

I have tested this patch in a number of use cases:

- local debugging in Fx desktop
- remote debugging another Fx desktop running the same code
- remote debugging another Fx desktop running an older code base (without the new highlighter)
- remote debugging Fennec
- remote debugging FxOS with the patched omni.ja containing the new highlighter
- remote debugging FxOS without the new highlighter.

Everything seems to work fine.
And here is the latest try build: https://tbpl.mozilla.org/?tree=Try&rev=313d1a7bd836

The failures seem unrelated, and there's a very annoying failure of test browser_UITour.js that constantly happens on a couple of platforms.

Attaching the part2 patch (the tests) now.

Comment 85

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part2 V8.patch

Tests.

Comment 86

[Paul Rouget [:paul]]

Playing with the patch on B2G. It works great! One thing: don't scroll on highlight in B2G (this will break some apps).

Comment 87

[Paul Rouget [:paul]]

Comment on attachment 8356522 [details] [diff] [review]
bug916443-remote-highlighter-part1 V13.patch

>+++ b/toolkit/devtools/server/actors/highlighter.js
>@@ -0,0 +1,747 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+const {Cu, Cc, Ci} = require("chrome");
>+const protocol = require("devtools/server/protocol");
>+const {Arg, Option, method, RetVal, types} = protocol;


RetVal and types are not used.

>+const events = require("sdk/event/core");

Why not the devtools event emitter?

Comment 88

[Darrin Henein [:darrin]]

Attachment: icon @1x

Comment 89

[Darrin Henein [:darrin]]

Attachment: icon @2x

Comment 90

[Brian Grinstead [:bgrins]]

Comment on attachment 8356522 [details] [diff] [review]
bug916443-remote-highlighter-part1 V13.patch

Review of attachment 8356522 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/jar.mn
@@ +259,5 @@
>    skin/classic/browser/devtools/[e-mail] (../shared/devtools/images/[e-mail])
>    skin/classic/browser/devtools/[e-mail] (../shared/devtools/images/[e-mail])
>    skin/classic/browser/devtools/[e-mail]   (../shared/devtools/images/[e-mail])
>    skin/classic/browser/devtools/[e-mail]         (../shared/devtools/images/[e-mail])
> +  skin/classic/browser/devtools/command-pick.png            (../shared/devtools/command-pick.png)

Can you move the command icon into the ../shared/devtools/images folder?

Comment 91

[Patrick Brosset <:pbro>]

Thanks for the fast review Paul!

(In reply to Paul Rouget [:paul] from comment #86)
> Playing with the patch on B2G. It works great! One thing: don't scroll on
> highlight in B2G (this will break some apps).
You're right, I'll do this.

(In reply to Paul Rouget [:paul] from comment #87)
> Comment on attachment 8356522 [details] [diff] [review]
> bug916443-remote-highlighter-part1 V13.patch
> 
> >+++ b/toolkit/devtools/server/actors/highlighter.js
> >@@ -0,0 +1,747 @@
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this
> >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+"use strict";
> >+
> >+const {Cu, Cc, Ci} = require("chrome");
> >+const protocol = require("devtools/server/protocol");
> >+const {Arg, Option, method, RetVal, types} = protocol;
> 
> 
> RetVal and types are not used.
Thanks. Done.

> >+const events = require("sdk/event/core");
> 
> Why not the devtools event emitter?
Well, I was reading dcamp's documentation of protocol.js (https://gist.github.com/campd/5460401) and that's what it says about actor events.

Thanks Darrin and Brian for the icons and advice. I'll add the new image now.

Comment 92

[Patrick Brosset <:pbro>]

Attachment: bug916443-remote-highlighter-part1 V14.patch

This new patch contains:
- The new icon provided by Darrin, at the place pointed out by Brian.
- The removal of scrollIntoView for the simpleOutlineHighlighter, as discussed with Paul
- Removal of unused RetVal and types

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=e39ea28f07bb
Seems green enough! Apart from unrelated failures.

Paul, care for a final quick review :) ?

Comment 93

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/bc5a874afdf7
https://hg.mozilla.org/integration/fx-team/rev/c3145720f680

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc5a874afdf7
https://hg.mozilla.org/mozilla-central/rev/c3145720f680

Comment 95

[Tim Nguyen :ntim]

Hey, sorry to needinfo you like this, but are those 2 things expected ? :
- Inspect button on node infobar is missing, it used to be more practical to me when I had the quick inspect button on that infobar.

- The selected node dotted border and the infobar that comes with it fades out after 1/2 seconds

Comment 96

[Tim Nguyen :ntim]

Also, the remote highlighter isn't working with the browser toolbox.

Comment 97

[Paul Rouget [:paul]]

(In reply to Tim Nguyen [:ntim] from comment #95)
> Hey, sorry to needinfo you like this, but are those 2 things expected ? :
> - Inspect button on node infobar is missing, it used to be more practical to
> me when I had the quick inspect button on that infobar.
> 
> - The selected node dotted border and the infobar that comes with it fades
> out after 1/2 seconds

Yes. We think it's better that way. Feel free to discuss that on the dev-developer-tools mailing list if you think it's not the right way to do it (see https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/O73OPMII2Bw )

> Also, the remote highlighter isn't working with the browser toolbox

It should. File a bug if it doesn't work for you (you should see a red rectangle around the node).

> Hey, sorry to needinfo you like this

No worries. For inspector related stuff, I encourage you to needinfo :pbrosset in the future.

Comment 98

[Patrick Brosset <:pbro>]

(In reply to Paul Rouget [:paul] from comment #97)
> (In reply to Tim Nguyen [:ntim] from comment #95)
> > Hey, sorry to needinfo you like this, but are those 2 things expected ? :
> > - Inspect button on node infobar is missing, it used to be more practical to
> > me when I had the quick inspect button on that infobar.
> > 
> > - The selected node dotted border and the infobar that comes with it fades
> > out after 1/2 seconds
> 
> Yes. We think it's better that way. Feel free to discuss that on the
> dev-developer-tools mailing list if you think it's not the right way to do
> it (see
> https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/
> O73OPMII2Bw )
Indeed, one the main changes is that there's no highlighter lock mode anymore, instead, it shows as you hover over nodes in the markup-view. It also shows quickly (1 sec) when you select a node by clicking in the breadcrumbs widget for example.
With this new behavior in mind it makes no sense having the inspect button in the nodeinfo bar anymore.

Having said this, you're not the first to give feedback about this change, and I think there could be a way to mix this new highlight-on-hover mode with some kind of a lock mode. I'll file a bug to discuss this.

> > Also, the remote highlighter isn't working with the browser toolbox
> 
> It should. File a bug if it doesn't work for you (you should see a red
> rectangle around the node).
The highlight on hover does work, but it seems the inspect button doesn't.
I will file a bug for this.

Comment 99

[Patrick Brosset <:pbro>]

> Having said this, you're not the first to give feedback about this change,
> and I think there could be a way to mix this new highlight-on-hover mode
> with some kind of a lock mode. I'll file a bug to discuss this.
bug 959073
> > > Also, the remote highlighter isn't working with the browser toolbox
bug 959076

Comment 100

[Will Bamberg [:wbamberg]]

Patrick: I've updated the Page Inspector docs, especially: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Selecting_elements, to explain this change. Please let me know if it works for you.

Thanks!

Comment 101

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #100)
> Patrick: I've updated the Page Inspector docs, especially:
> https://developer.mozilla.org/en-US/docs/Tools/
> Page_Inspector#Selecting_elements, to explain this change. Please let me
> know if it works for you.
> 
> Thanks!
Sounds good, thanks for updating the doc!
Just one comment though: "as you move the mouse around the markup in the HTML pane, the selected element changes accordingly and the dotted border is briefly shown" --> In fact the border is shown for as long as you hover over a given element in the HTML pane. The video shows the border jumping around from node to node quickly because the mouse moves quickly around the HTML pane. This is, for me, the best improvement of this bug because it allows very quickly to see what part of the page corresponds to what node in the HTML pane, just by hovering over it, and I think the "is briefly shown" part may not be saying this clear enough.

Comment 102

[Patrick Brosset <:pbro>]

(In reply to Will Bamberg [:wbamberg] from comment #100)
> Patrick: I've updated the Page Inspector docs, especially:
> https://developer.mozilla.org/en-US/docs/Tools/
> Page_Inspector#Selecting_elements, to explain this change. Please let me
> know if it works for you.
> 
> Thanks!
I have made a change to the MDN page. Please let me know if it looks good.

Comment 103

[Will Bamberg [:wbamberg]]

Looks great. Thanks. It's really hard to explain this in words, the screencast helps a lot.

Comment 104

[Ioana (away)]

I'm not seeing any request associated with the qawanted keyword. Is qa still needed here?

Comment 106

[Bogdan Maris, Desktop Test Engineering]

Verified as fixed on Firefox 29 beta 2 using Ubuntu 12.04 32bit, Mac OS X 10.8.5 and Windows 7 64bit.