Closed Bug 663778 Opened 13 years ago Closed 10 years ago

Implement box model highlighter

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox30 fixed)

RESOLVED FIXED
Firefox 30
Tracking Status
firefox30 --- fixed

People

(Reporter: paul, Assigned: miker)

References

(Depends on 1 open bug)

Details

Attachments

(11 files, 41 obsolete files)

26.85 KB, image/png
Details
576.45 KB, image/png
Details
6.47 MB, video/quicktime
Details
40.22 KB, image/png
Details
32.69 KB, image/png
Details
11.68 KB, patch
miker
: review+
Details | Diff | Splinter Review
4.99 KB, patch
miker
: review+
Details | Diff | Splinter Review
36.66 KB, patch
miker
: review+
Details | Diff | Splinter Review
21.14 KB, patch
miker
: review+
Details | Diff | Splinter Review
58.18 KB, patch
miker
: review+
Details | Diff | Splinter Review
25.41 KB, patch
miker
: review+
Details | Diff | Splinter Review
Once a node is locked, we should draw the different layout regions:
× margins
× padding
× borders / scrollbars
Attached image screenshot firebug (obsolete) —
Attached image screenshot google-chrome (obsolete) —
Blocks: 663830
Attached patch patch v1 (obsolete) — Splinter Review
The layout boxes are drawn as soon as the user lock the node.
In the future, the behavior will probably different (only if the node is hovered? A toggle button?).
Attachment #538959 - Flags: review?(mihai.sucan)
Depends on: 663781
Comment on attachment 538959 [details] [diff] [review]
patch v1

need tests.
Attachment #538959 - Flags: review?(mihai.sucan)
Comment on attachment 538959 [details] [diff] [review]
patch v1

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

Patch looks good!

Please address the comments and include a test. It would also be nice to include a screenshot of how this looks (you included screenshots from chrome and firebug ;) ).

::: browser/base/content/inspector.js
@@ +354,4 @@
>     */
>    lock: function IFH_lock()
>    {
> +    let bbox_rect = this._highlightOriginalRect;

bbox_rect is not really a usual variable name in the Mozilla coding style.

I would recommend something like bboxRect.

@@ +378,5 @@
> +   *        The bounding box of the selected node.
> +   * @param CSSStyleDeclaration style
> +   *        Computed-style of the selected node.
> +   */
> +  drawSelectionBox: function IFH_drawSelectionBox(bbox_rect, style)

Please use the "a" prefix for arguments.

aBoxRect, aStyle.

Also use a similar style for the rest of the variable names and function arguments.
Status: NEW → ASSIGNED
Whiteboard: minotaur
Priority: -- → P1
Whiteboard: minotaur → [minotaur][has-patch]
Attached patch patch v2 (obsolete) — Splinter Review
Only asking for feedback right now. I need to rebase this patch (WIP).

The abstract view lie in a doorhanger. This is temporary. We want to host this tool in a sidebar (bug 674476).

About the UI, the only thing I want to change is the color of the node in the blueprint. I think we should re-use the purple design we have in the highlighter.
Attachment #538959 - Attachment is obsolete: true
Attachment #548886 - Flags: feedback?(mihai.sucan)
Attached patch patch v2.1 (obsolete) — Splinter Review
Updated with tests.
Attachment #548886 - Attachment is obsolete: true
Attachment #548886 - Flags: feedback?(mihai.sucan)
Attachment #549074 - Flags: review?(mihai.sucan)
Depends on: 674871, 666650
No longer depends on: 663781
Comment on attachment 549074 [details] [diff] [review]
patch v2.1

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

Patch looks fine, but there are still some problems that need to be addressed. The layout view loks awesome, and the text is readable on my system, yay!

Screenshot with some comments:
http://img.i7m.de/show/87p15.png

(sorry for the ugly callouts :) )

1. The doorhanger button is no longer clickable once the Layout view opens. I cannot close the view once it's open.

2. There's a regression that causes links I click in the page to be followed, when I click on a node that I want to lock on. All click-related events need to be canceled. The code that does this is now broken, unfortunately.

3. Margin indicators when the node is locked are barely visible. Please see the screenshot.

4. When I scroll the page the element position (coordinates info) also change. The padding and margin highlights do not properly update as well. There seem to be some problems with the code that calculates the window-relative coords of the element.

5. When I lock on a different element the layout view does not update.

6. Strings need to be localized.

7. Most of the Inspector tests fail to run with the patch applied.

Some of the problems might be caused by the minimal rebase I did. Am I missing some patches that would fix (some of) these problems?

Please also see the review comments below. Thank you!

::: browser/base/content/inspector.js
@@ +258,5 @@
> +                     left: clientRect.left,
> +                     width: clientRect.width,
> +                     height: clientRect.height};
> +
> +    let orgRect = {top: clientRect.top,

Please add a comment here that explains the reason you want two rect objects. Also please explain what is the purpose of the two properties, offsetX and offsetY.

Also please rename "orgRect" to something more clear.

@@ +933,5 @@
> +      document.getElementById("inspector-inspect-toolbutton").checked = false;
> +      this.detachPageListeners();
> +      this.inspecting = false;
> +      if (this.highlighter.node) {
> +        this.select(this.highlighter.node, true, !aPreventScroll);

This line throws. aPreventScroll is undefined.

@@ +1516,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +//// Layout
> +
> +/**
> + * Layout View

Please add a comment here explaining what is Layout View.

@@ +1521,5 @@
> + *
> + * @param nsIDOMNode aBrowser
> + *        The xul:browser object for the content window being highlighted.
> + */
> +function LayoutView(aBrowser)

I would suggest renaming this to InspectorLayoutView, or ElementLayoutView.

@@ +1538,5 @@
> +
> +  /**
> +   * Open the panel, build the layout boxes and add observers.
> +   */
> +  onShow: function Layout_onShow(aNode)

aNode is unused.

@@ +1547,5 @@
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.LOCKED, false);
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.UNLOCKED, false);
> +
> +    let btn = document.getElementById("inspector-layout-toolbutton");
> +    let node = InspectorUI.selection;

This is unused.

@@ +1548,5 @@
> +    Services.obs.addObserver(this, INSPECTOR_NOTIFICATIONS.UNLOCKED, false);
> +
> +    let btn = document.getElementById("inspector-layout-toolbutton");
> +    let node = InspectorUI.selection;
> +    this.panel.openPopup(btn, "overlap", 0, 0, false, false);

"overlap" is the problem here with the doorhanger covering the Layout button.

Please see:

https://developer.mozilla.org/en/XUL/Attribute/popup.position

I think you want "after_start" here.

@@ +1583,5 @@
> +   */
> +  onLocked: function Layout_onLocked()
> +  {
> +    let node = InspectorUI.selection;
> +    let layout = this.compute(node,

node is not used in other places in this method.

Just do this.compute(InspectorUI.selection, ...).

@@ +1605,5 @@
> +   * When a node is selected, we do nothing.
> +   */
> +  onSelect: function Layout_onSelect(aNode)
> +  {
> +  },

This is empty, please cleanup.

@@ +1609,5 @@
> +  },
> +
> +
> +  /**
> +   * Event handler.

This is not a useful comment. Please explain what the event handler does.

@@ +1628,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Observers.

Same case as with handleEvent().

@@ +1643,5 @@
> +     }
> +   },
> +
> +  /**
> +   * Register the Layout tool

Please elaborate. Something like "Register the Layout tool into the Inspector".

@@ +1650,5 @@
> +  {
> +    InspectorUI.registerTool({
> +      id: "layout",
> +      context: this,
> +      label: "Layout",

Please make all of the new strings localizable.

Please see the patch from bug 566084. You need:

+XPCOMUtils.defineLazyGetter(InspectorUI, "strings", function () {
+  return Services.strings.
+         createBundle("chrome://browser/locale/inspector.properties");
+});

... and a new file:

browser/locales/en-US/chrome/browser/inspector.properties

... where you add your strings. For how to use the string bundle, see the patch from the aforementioned bug.

@@ +1799,5 @@
> +    layout.node.top = Math.round(rect.top + win.scrollY);
> +    layout.node.left = Math.round(rect.left + win.scrollX);
> +
> +    layout.offset.left = rect.offsetX - win.scrollX;
> +    layout.offset.top = rect.offsetY - win.scrollY;

This might be wrong.

In highlight() you compute rect.offsetX/Y (which there is orgRect) by adding the x/y values from all of the iframes. Here you use win.scrollY where win is a reference only to the window of the highlighted element. This is surprising because it seems to be mixing window.top coords with element iframe win.scrollX/Y. That is, rect.offsetX/Y coords are relative to the window.top, while win.scrollX/Y ... are just relative to the iframe where the highlighted element happens to be.

Maybe I am wrong, but this did immediately caught my eye when reading the code, before I even tested the scrolling issue. After reading the code I tested scrolling and saw the problems with the coords. ;)

@@ +1834,5 @@
> +
> +    mapLayout(this.layoutBordersBox, layout.borders);
> +    mapLayout(this.layoutPaddingBox, layout.padding);
> +
> +    let offsetY = layout.node.top + layout.offset.top;

Notice that:
layout.node.top = rect.top + win.scrollY;
layout.offset.top = rect.offsetY - win.scrollY;
(taken from compute())

So here offsetY is actually:
offsetY = rect.top + win.scrollY + rect.offsetY - win.scrollY;

(scrollY is canceled)

Is that on purpose?

::: browser/base/content/layout-view.html
@@ +36,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK ***** -->

This does not follow the correct boilerplate template. Please update according to:

http://www.mozilla.org/MPL/boilerplate-1.1/

@@ +39,5 @@
> +#
> +# ***** END LICENSE BLOCK ***** -->
> +
> +<meta charset="utf8">
> +<title>Layout View</title>

This needs to be an XHTML file with an associated DTD file for the localization of the strings used in the file.
Attachment #549074 - Flags: review?(mihai.sucan) → review-
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][best:1d; likely: 3d; worst: 5d]
Attached image Firebug layout view (obsolete) —
For what it's worth, I'm attaching a screenshot of Firebug's layout view (not the info presented around the node, but the info presented within Firebug's inspection interface). This is the info for the same node at limi.net as in the screenshot for the layout tool that Paul has built here.
Not for later:
we could have 3 different view:
x square (like the current view)
x horizontal rect
x vertical rect

The ratio of the rect doesnt have to be big (1.5).

Could help to understand the abstract view.
I like that idea. I think Kyle mentioned it last week too, suggesting we should mimic the aspect ratio of the selected node to make it apparent that the layout box is connected to the selection.

+1
Summary: [highlighter] Draw layout information of the selected node → [layout] Draw layout information of the selected node
Depends on: 683954
No longer depends on: 674871
The Layout Panel will be implemented in bug 683954. This bug now depends on it.
Should be able to get by with the Layout Panel for minotaur.
Whiteboard: [minotaur][has-patch][best:1d; likely: 3d; worst: 5d] → [has-patch][best:1d; likely: 3d; worst: 5d]
We're doing developer tool prioritization, filter on 'brontozaur' to ignore the spam.
Priority: P1 → P3
triage, filter on centaur
Assignee: paul → nobody
Status: ASSIGNED → NEW
Whiteboard: [has-patch][best:1d; likely: 3d; worst: 5d] → [has-patch]
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Assignee: mratcliffe → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
Assignee: nobody → mratcliffe
Priority: P3 → P2
shorlander: Before I start this we need to decide on the UI. Personally, I like the way Firebug displays the box model on the page when a HTML node is highlighted in the HTML panel or the layout panel is hovered.

How do you think we should show the box model in the highlighter?
Questions:

Can we get a summary of the different possible options (how it works in Chrome and Opera)?

Is there any alternative (because the existing implementation don't look very great)?
We can probably improve the design, but I was wondering if could "re-think" the concept of
highlighting regions (one region is either the padding, the margin or the borders).

Do we want to show the margins, the padding and the border at the same time?
Do we want to only show one region at a time?

Do we want to differentiate the regions with colors? Patterns?
Maybe there's no need to differentiate them if we want to show one region at a time.

When do we want to highlight the region? On mouseover on the layoutview?

Do we want the regions to "live" (be stuck) with the highlighter and follow the scroll/zoom/updates?

What happens if there is a CSS Animation on the margin?

We are planning to hide the veil when using the Rule View (bug 735575), how does it affect this bug?

Can we imagine having "grids/outlines" to extend the regions? (inspiration: http://dribbble.com/shots/463243-3-Column-Grid http://dribbble.com/shots/289030-Christowski-Blog-Info)
Attached image screenshot of opera (obsolete) —
Screenshot of Opera. Rule lines are an interesting idea.
Attached image screenshot of opera (obsolete) —
real screenshot.
Attachment #613978 - Attachment is obsolete: true
The current Firebug screenshots miss a lot of features. Here is an updated one that shows the difference between hovering a node in the HTML panel and hovering the various areas of the layout panel.
Attachment #538849 - Attachment is obsolete: true
Attachment #549773 - Attachment is obsolete: true
(In reply to Paul Rouget [:paul] from comment #19)
> Questions:
> 
> Can we get a summary of the different possible options (how it works in
> Chrome and Opera)?
> 

When hovering a node in the HTML panel Chrome, Opera & Firebug all use the same method, a simple box model highlighter overlays the node on the page. Internet Explorer does nothing when a node is hovered in the HTML panel.

In addition:
Chrome:
Hovering the content, padding, border and margin in the layout panel highlights only the thing that is hovered. Hovering position (offsets) shows a doorhanger with e.g. "div 70px x 70px" inside pointing to the element. Double-clicking a number in the layout view allows each value to be edited.

Opera:
When hovering elements in the webpage itself guidelines are displayed showing the edges of the node's border along with a box model style highlight. In the layout panel hovering the content, padding, border and margin highlights only the thing that is hovered. Double-clicking a value allows it to be edited.

Internet Explorer:
Does nothing when the layout panel is hovered. Single-clicking a value allows for it to be edited.

Firebug:
Hovering anything in the layout view shows the simple box model highlighter. The nodes positioned parent is outlined in a dotted red line with rulers (px) on the top and left borders (an absolutely positioned node's positioned parent is the closest parent that is absolutely or relatively positioned).

Guide lines are displayed denoting the content, padding, border and margin depending on which is hovered in the layout view. Single-clicking any value allows for it to be edited.

The ultimate:
Hovering anything in the layout view shows the simple box model highlighter. The nodes positioned parent is outlined in a dotted red line with rulers (px) on the top and left borders (an absolutely positioned node's positioned parent is the closest parent that is absolutely or relatively positioned).

Depending on the users preference 1. Guide lines are displayed denoting the content, padding, border and margin depending on which is hovered in the layout view. Single-clicking any value allows for it to be edited or 2. Hovering the content, padding, border and margin in the layout panel highlights only the thing that is hovered ... guide lines denote the bounds of each as they are highlighted.

Hovering position (offsets) shows a doorhanger with e.g. "div 70px x 70px" inside pointing to the element.

Double-clicking a number in the layout view allows each value to be edited.

> Is there any alternative (because the existing implementation don't look
> very great)?
> We can probably improve the design, but I was wondering if could "re-think"
> the concept of
> highlighting regions (one region is either the padding, the margin or the
> borders).
> 
> Do we want to show the margins, the padding and the border at the same time?
> Do we want to only show one region at a time?
> 
> Do we want to differentiate the regions with colors? Patterns?
> Maybe there's no need to differentiate them if we want to show one region at
> a time.
> 
> When do we want to highlight the region? On mouseover on the layoutview?
> 

Personally, I think the "ultimate" implementation from above would be best.

> Do we want the regions to "live" (be stuck) with the highlighter and follow
> the scroll/zoom/updates?
> 

That would make sense.

> What happens if there is a CSS Animation on the margin?
> 

The highlighter should be updated.

> We are planning to hide the veil when using the Rule View (bug 735575), how
> does it affect this bug?
> 

When hovering nodes or the layout view I would say that we should show it again during the time that they are hovered.

> Can we imagine having "grids/outlines" to extend the regions? (inspiration:
> http://dribbble.com/shots/463243-3-Column-Grid
> http://dribbble.com/shots/289030-Christowski-Blog-Info)

Do you mean allowing the edges of a highlight to be dragged to change these things for a node? Maybe show guides when a node is highlighted and allow the guides to be dragged to change the elements size ... this would be very awkward to use to change border width etc. Using text input also allows different units to be used %, px, pt etc.
Brian wrote, in bug 770818 comment 3:
> #4 When the user is hovering over the various parts of the element in layout view, such as
> margin, border, etc the corresponding areas on the live element on the page are
> highlighted while the rest of the page is dimmed.

Brian,

if we remove the veil (as discussed in bug 770818), do you think just changing the size of the dashed border is enough (nothing is dimmed)?
Whiteboard: [has-patch]
Attachment #549074 - Attachment is obsolete: true
Attachment #548882 - Attachment is obsolete: true
Changing the size of the dashed border (changing the size of the highlighted region) might not be enough as it only shows a bounding box, not a region.
Assignee: mratcliffe → paul
Depends on: 770818
Attached patch v3 (obsolete) — Splinter Review
Todo:
- get a visual design + ui review
Todo:
- get a visual design + ui review
- tests
- what to do for margin:auto (expand to parent padding box?)
- use a canvas for complex border design (stips)
Interaction proposal:
- while inspecting (node not locked) we show the regions (margin/padding/borders/content)
- once locked, we hide the regions
- once locked and the mouse is overring the layout view, we show the targeted region (as in the screencast in comment 27)
Todo:
- get a visual design + ui review
- tests
- what to do for margin:auto (expand to parent padding box?) (can happen in a follow-up bug)
- use a canvas for complex border design (strips) (can happen in a follow-up bug)
- show the regions when node not locked (can happen in a follow-up bug)
Hi guys,
Will this or bug 770818 be ready to go ahead in FF 15 release on Tuesday august 28?
Depends on: 381328
Depends on: 653545
Depends on: 626359
Attached image DarkTheme-Inspector-Locked@2x.png (obsolete) —
Shorlander recently looked into this and provided this design.

Basically, he wants us to always use the box model in the same way that Chrome does.

For transformed elements we should be faithful to the box model so we should show the content and border boxes transformed but margin and padding should remain untransformed.

Hovering border, margin and padding in the layout view should show guides for content, border, padding or margin. The layout view's border needs to be made thicker in order to make this possible.

I believe getQuads() should give us the correct information.
Assignee: paul → mratcliffe
Attachment #538850 - Attachment is obsolete: true
Attachment #613979 - Attachment is obsolete: true
Attachment #613999 - Attachment is obsolete: true
Attachment #650859 - Attachment is obsolete: true
Depends on: 917755
No longer depends on: 381328, 626359
When it comes to css transformed elements, I'd like to suggest that we draw something like this: http://codepen.io/captainbrosset/full/lHpnK (I'll attach a screenshot too).
The idea is that you should easily see where the original box was supposed to be, and where the transformed box is now, possibly with 4 arrows, one for each corner.
(In reply to Patrick Brosset from comment #38)
> Created attachment 815841 [details]
> display-normal-and-transformed-boxes.png

When an image is transformed only the border and content are moved. The margin, padding, border and content still take the same space on the page.

It would make sense to show the box model as normal but with the border and content transformed in this way. This is probably the next step as we need to get the box model in first.

Will you log a new bug for handling transformed elements in the box model highlighter?
Blocks: 925744
Blocks: 935956
Attached image highlighter-2.png
We have decided on SVG for the highlighter due to it's capability to highlight complex shapes etc. We need to check for any performance issues when graphics are not accelerated.

The highlighter should also show guides.

Here is the latest mock-up.
Attachment #807164 - Attachment is obsolete: true
I think anyway something like SVG or canvas is mandatory for this. Two reasons I see are:
- if the element is css-transformed
- if the element has rounded corners
In both these cases, the highlighter will need to show a complex shape.

btw, Mike, are we going to be able to show border-radius? And do we even want to? I know chrome doesn't, and it feels a bit strange to be drawing rectangles on top of a rounded shape.
(In reply to Patrick Brosset from comment #42)
> 
> btw, Mike, are we going to be able to show border-radius? And do we even
> want to? I know chrome doesn't, and it feels a bit strange to be drawing
> rectangles on top of a rounded shape.

The box model never has rounded corners. If our highlighter is showing the box model then it shouldn't have rounded corners.
Right, silly question.

Another point though, what about animated elements? Should the box model "follow" them around? If, say, the element currently selected has an animation assigned to it that increases its height or margin.

I would say yes, but that will require to re-draw the regions every now and then (maybe throttling it to 100ms or so).
The current highlighter does follow animated elements today using, I think, "MozAfterPaint" events.
Highlighter Overlay Colors

Content (blue)  :  #80d4ff @ 0.4 alpha
Border (yellow) :  #ffe431 @ 0.4 alpha
Padding (green) :  #66cc52 @ 0.4 alpha
Margin (orange) :  #d89b28 @ 0.4 alpha
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> The box model never has rounded corners.

Why?
(In reply to Joe Walker [:jwalker] from comment #47)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> > The box model never has rounded corners.
> 
> Why?

I agree with Mike, it'd be weird to show the rounded borders. Only borders have rounded borders, and they don't impact the box model.

Also, rounded borders "eat" the content, so it'd be weird to show a rounded border region overlapping with the content region.
Is this still valid?

"The CSS box model describes the rectangular boxes that are generated for elements in the document tree and laid out according to the visual formatting model."
- http://www.w3.org/TR/CSS2/box.html
Any update on this?
Yes, I am finishing off disable cache (moved into the TabActor) and then working on this.
For the time being I will base the patch on roc's patch from bug 917755 as that will give him more time. In the worst case scenario I could create a polyfill.
Summary: [layout] Draw layout information of the selected node → Implement box model highlighter
This is WIP. I still need is to use getQuads to position the NodeInfobar and write tests.
Attached patch Rebased WIP (obsolete) — Splinter Review
... and create a getBoxQuads polyfill.

getQuads does take scrolling into account but does not take frames into account (probably a bug). This means that we will need two methods:
- getBoxQuads polyfill
- getBoxQuadsWithFrameOffsets() method in highlighter.js
Attachment #8358519 - Attachment is obsolete: true
Hi Mike, here's a polyfill for the getBoxQuads API.
It's inside LayoutHelpers.jsm.
I've also added a function called getAbsoluteBoxQuads which is what the highlighter uses. Indeed, getBoxQuads returns quads relative to the node's viewport, so this getAbsoluteBoxQuads function will loop up to get the quads relative to the top level window.
It's late so I might have forgotten stuff but (and I don't fully know why), it seems to work pretty well with respect to iframes and scrolling.
I'll attach a short videos demonstrating how it works on a simple test page.
Flags: needinfo?(mratcliffe)
Oh by the way, this patch was built on top of attachment 8358519 [details] [diff] [review], so not your latest patch, as I didn't noticed you added a new one. I hope it'll be fine.
Just to show how the polyfill behaves on a simple test page, with scrolling and iframes.
Comment on attachment 8368224 [details] [diff] [review]
bug663778-node.getBoxQuads-polyfill.patch

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

r+ for the polyfill
Attachment #8368224 - Flags: review+
Clearing needinfo request
Flags: needinfo?(mratcliffe)
Can you make a first pass over this... nothing complicated.

Please grab the dependencies from:
http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter

It will save you having to rebase them.

This is based on the native getBoxQuads implementation although the polyfill code is included (I will move that to a separate patch). When we have this fully working with the native implementation we can switch to the polyfill and make it more compatible.

Note that it is broken on zoomed and retina displays so run tests using --jsd and drag the browser window to a normal display before running any tests.

The only failing test I know of is browser_inspector_markup_navigation.js.
Attachment #8367260 - Attachment is obsolete: true
Attachment #8368224 - Attachment is obsolete: true
Attachment #8370144 - Flags: review?(pbrosset)
Comment on attachment 8370144 [details] [diff] [review]
box-model-highlighter-663778.patch WIP

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

Ok, this is a preliminary review just looking at the code.
Indeed, I haven't had the time to play with the new highlighter (with the patch applied) yet.

Mike: can you provide a new patch that uses the polyfill whenever you're ready, just so I can test with the code that will actually be used (if we manage to get this in Aurora).

I have a few comments below, but I'm globally pretty happy with the patch so far.

::: browser/base/content/browser.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  @namespace html url("http://www.w3.org/1999/xhtml");
> +@namespace svg url("http://www.w3.org/2000/svg");

You've defined this @namespace in all of the browser.css files (this one and the themed ones), but you're actually using this namespace in highlighter.inc.css

Can the namespace be defined there instead? Maybe there's something about *.inc.css files that make this impossible, I don't know but I think these files are pre-processed in some ways.

::: browser/devtools/framework/selection.js
@@ +165,5 @@
> +    // set the node even if it is already set otherwise it is not possible to
> +    // e.g. highlight the same node twice.
> +    let rawValue = null;
> +    if (value && value.isLocal_toBeDeprecated()) {
> +      rawValue = value.rawNode();

Can you elaborate a bit more here?
Why did you need to do that change? Did the outline highlighter suffered from the same problem?

I see one potential problem with doing this: the markup-view, rule-view, computed-view, font-inspector and layout-view will refresh again even if the same node is reselected.
Indeed, most things in the inspector listen to selection.on("new-node-front") to refresh.
Maybe it's not an issue but I'm afraid we have code that sets the selection with the same node without checking.

::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
@@ +53,5 @@
>  
>    function assertH1Highlighted() {
> +    let deferred = promise.defer();
> +
> +    executeSoon(() => {

Why is this executeSoon needed? Can you add a comment?

::: browser/devtools/inspector/test/browser_inspector_bug_674871.js
@@ -49,5 @@
>      openInspector(aInspector => {
>        inspector = aInspector;
>        // Make sure the highlighter is shown so we can disable transitions
>        inspector.toolbox.highlighter.showBoxModel(getNodeFront(doc.body)).then(() => {
> -        getHighlighterOutline().setAttribute("disable-transitions", "true");

It's a good thing we can get rid of these.
And I do agree that transitions are not needed anymore with your patch, it would feel too "heavy" to have transitions on the box-model regions.

::: browser/devtools/inspector/test/browser_inspector_highlighter.js
@@ +65,5 @@
>  
>    EventUtils.synthesizeMouse(h1, 2, 2, {type: "mousemove"}, content);
>  }
>  
>  function testOutlineDimensions() {

I think the name of the function should be changed since we don't have an outline anymore.

@@ +99,4 @@
>      let outlineWidth = Math.floor(outlineDims.width);
>      let outlineHeight = Math.floor(outlineDims.height);
>  
> +    // TODO: Fix this when getBoxQuads() properly deals with zoom.

Since we're going to try and fix this bug with our polyfill, shouldn't we make it handle zoom too? In which case the 2 following asserts should stay.

::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
@@ +54,5 @@
>      aElement.ownerDocument.defaultView);
>  }
>  
>  function runTests() {
> +  executeSoon(testDiv1Highlighter);

Why is executeSoon needed here too? Can you add a comment if there's no other way.
Maybe the promise returned by inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?

::: browser/devtools/inspector/test/browser_inspector_scrolling.js
@@ +33,5 @@
>  {
>    inspector = aInspector;
>  
> +  inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(() => {
> +    performScrollingTest();

nit: please remove the arrow function:
inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(performScrollingTest);

@@ +54,5 @@
>    }, false);
> +
> +  executeSoon(function() {
> +    // FIXME: this will fail on retina displays. EventUtils will only scroll
> +    // 25px down instead of 50.

Could you try using content.devicePixelRatio to make the test work on all types of screens?
I know this isn't actually part of the bug, but it's been a problem for a long time and since you're touching the file, might as well try and put it in.

::: browser/devtools/inspector/test/head.js
@@ +74,3 @@
>  
> +function getBoxModelStatus()
> +{

nit: please use the prevailing formatting style for the function opening brace.

::: browser/devtools/layoutview/view.css
@@ +69,5 @@
>    border-width: 25px;
>  }
>  
>  #borders {
> +  padding: 25px;

I noticed that with this change, the border region in the layout-view sidebar has changed, it used to be a thin solid line with box-shadow, now it looks exactly like the padding and margin regions.
I like the fact that it's wider than before, it was hard to position the mouse over the thin (2px?) line before. But I liked the way it looked before, the fact that it was a plain line with shadow made it quicker to understand what you were looking it.

Could you try making it look like a bit more like it used to look while preserving the padding: 25px you added?
Adding these rules to #borders {} could work:
border-width: 2px;
border-style: solid;
box-shadow: 0 0 16px #000;
But it's not perfect, better ask Darrin perhaps.

::: browser/devtools/layoutview/view.js
@@ +240,5 @@
>      this._lastRequest = lastRequest;
>      return this._lastRequest;
> +  },
> +
> +  showBoxModel: function(options={}) {

showBoxModel and hideBoxModel can now be supplanted by:
this.inspector.toolbox.highlighterUtils.highlightNodeFront()
and
this.inspector.toolbox.highlighterUtils.unhighlight()
I've moved this code from markup-view to toolbox recently so that it avoids duplicating this nasty if/else logic all over the place.

Having said this, I don't see where these methods are being called. They don't seem to be!
Did you intend to toggle the box model on mouse-over of the layout-view regions? I think this is an excellent idea (and it should only highlight the corresponding region, not the whole box-model), but if this isn't going to be part of this patch, better remove this code altogether (to keep the patch small).
Since we're trying to land this so it can be uplifted, better file a separate bug to deal with this.

::: browser/devtools/layoutview/view.xhtml
@@ +29,5 @@
>      </p>
>  
>      <div id="main">
>  
> +      <div id="margins" data-box="margin" tooltip="&margins.tooltip;">

These data attributes have probably been added to wire the showboxmodel-on-hover mechanism, and should be either used or removed (see my other comment in view.js).

::: toolkit/devtools/LayoutHelpers.jsm
@@ +74,5 @@
> +      return this._getBoxQuadsFromRect(offsetRect(paddingRect, "padding"));
> +    }
> +  },
> +
> +  getAbsoluteBoxQuads: function(node, options) {

I see you've moved the frame-offset logic to highlighter.js instead, so this function should be removed.

::: toolkit/devtools/server/actors/highlighter.js
@@ +315,5 @@
> +      padding: this._createSVGNode("padding", "polygon", this._boxModelContainer),
> +      content: this._createSVGNode("content", "polygon", this._boxModelContainer)
> +    };
> +
> +    this._guides = {

nit: please make the _boxModelNodes and _guides names consistent, so _guideNodes would be better maybe.

@@ +397,5 @@
>        barHeight: barHeight,
>      };
>    },
>  
> +  _createSVGNode: function(boxType, svgNodeType, parent) {

nit: please rename boxType into className, since that's what it is. It's only a box type some of the times. Also, I don't think the svg part is needed in svgNodeType, just nodeType would be enough.

@@ +551,5 @@
> +
> +    options.box = options.box || "content";
> +
> +    // We do not cache the rectangle purely because comparing all 16 points
> +    // unnecessarily complicates things.

I feel ok with not caching it anymore. If we do have perf problems in the future, this will be an easy change to make.
However, for someone looking at this code and not knowing that we did have caching before, this comment might seem strange. So, it's only a detail, but I would remove it.

@@ +552,5 @@
> +    options.box = options.box || "content";
> +
> +    // We do not cache the rectangle purely because comparing all 16 points
> +    // unnecessarily complicates things.
> +    let rect = this._getBounds("border");

I have to say it seems strange to me that the boxQuads API offers both bounds and points since it's basically the exact same information (or am I missing something?).
I would rather use the points only throughout this class.

Also, you're calling getBounds here which, in turn, calls getAdjustedQuads (which loops through frames), knowing that getAdjustedQuads will be called again 4 times below in the for..of loop.
You might want to rewrite this to reuse the result.

In fact, what we should do here is call getBoxQuads(boxType) 4 times, one for each type of box, and only then traverse the frames to offset all 4 boxes (to traverse only once). Walking up the DOM isn't an expensive thing, but getting offsets on each element as we go is, so better minimize the number of times we do this (especially knowing this will be done on mouse-over, so, potentially very fast)

@@ +592,5 @@
> +                }
> +                arr.splice(arr.lastIndexOf(val), 1);
> +              }
> +            }
> +          }

Looking at these last 20 lines, it's really to get what's being done here.
I would do 2 things:
- extract these 20 lines to another (private) function, and give it a name and comment that explain what this is about
- change the validX and validY names to something more self-explanatory.

If I understand this correctly, you're trying to get the min X/Y and max X/Y, but only if the box hasn't been transformed in a way that the guides would not be parallel to the page's edges, right?
For instance, if allY is [15, 10, 25, 30] (which could happen if the element is skewed), then there won't be any validY, which would end up hiding the top and bottom guides. Is my understanding correct?

If yes, do we really want to hide the guides when the element is transformed?
Indeed, they may not be super useful in this case. What was the reasoning behind this?

@@ +625,5 @@
> +   *         The guide to update
> +   * @param  {Integer} point
> +   *         x or y co-ordinate
> +   */
> +  _updateGuide: function(guide, point=-1) {

Please add more comments to this function about the fact that this will hide if no point is passed.

@@ +685,5 @@
>    /**
>     * Move the Infobar to the right place in the highlighter.
>     */
>    _moveInfobar: function() {
> +    const offset = 5;

Can you move this const to the top of the file and give it a more explicit name?
Something like INFO_BAR_OFFSET

@@ +687,5 @@
>     */
>    _moveInfobar: function() {
> +    const offset = 5;
> +
> +    let rect = this._getBounds("margin");

I'm again a little bit worried that we're calling getBounds again here, especially because _moveInfobar is called during _update, right after _highlightBoxModel is called.

The only other place where we call _moveInfobar is in _showInfobar, called itself in show, which also calls _update!

So basically, moveInfobar is called twice everytime we show the box model, and it is always called at the same time as highlightBoxModel, so we can probably make it accept a rect argument to avoid having it recompute it.

@@ +797,5 @@
> +        x: quads.p4.x + xOffset,
> +        y: quads.p4.y + yOffset,
> +        z: quads.p4.z
> +      },
> +      bounds: {

As I said in one of the comments in this file, if we decide not to use bounds and only rely on the points, we won't need this part.
Attachment #8370144 - Flags: review?(pbrosset)
I still need to address pbrosset's review comments from comment 61 but this is the current patch.
Attachment #8370144 - Attachment is obsolete: true
This switches to using the polyfill that now plays well with zoom and Retina displays.
Comment on attachment 8370832 [details] [diff] [review]
Patch 1: box-model-highlighter-663778.patch WIP

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature
User impact if declined: The devtools highlighter changes make little sense with the simple frame that we currently use. This patch adds a box model highlighter and makes a large difference to the user experience.
Testing completed (on m-c, etc.): It will be
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8370832 - Flags: approval-mozilla-aurora?
Comment on attachment 8370835 [details] [diff] [review]
Patch 2: box-model-polyfill-663778.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature
User impact if declined: The devtools highlighter changes make little sense with the simple frame that we currently use. This patch adds a box model highlighter and makes a large difference to the user experience.
Testing completed (on m-c, etc.): It will be
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None
Attachment #8370835 - Flags: approval-mozilla-aurora?
The patches need a little tidying but they are almost done.

At the moment you are best getting the prerequisites for this bug from http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter/summary unless your hobby is rebasing.

The biggest issue at the moment is that we have broken the markup navigation test:
./mach mochitest-browser browser/devtools/markupview/test/browser_inspector_markup_navigation.js

Seems like some kind of timing issue we have uncovered.
(In reply to Michael Ratcliffe - PTO until 28 JAN [:miker] [:mratcliffe] from comment #66)
> The patches need a little tidying but they are almost done.
> 
> At the moment you are best getting the prerequisites for this bug from
> http://hg.mozilla.org/users/mratcliffe_mozilla.com/box-model-highlighter/
> summary unless your hobby is rebasing.

Actually, the two patches apply cleanly without any prerequisites.

> 
> The biggest issue at the moment is that we have broken the markup navigation
> test:
> ./mach mochitest-browser
> browser/devtools/markupview/test/browser_inspector_markup_navigation.js
> 
> Seems like some kind of timing issue we have uncovered.

This test passes locally for me, I've also pushed to try to check: https://tbpl.mozilla.org/?tree=Try&rev=d0581b6c93cf
Seems like the C++ side of getBoxQuads() must be causing the issue.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #68)
> Seems like the C++ side of getBoxQuads() must be causing the issue.

There do still seem to be some issues on the bc test, though at least for me they aren't showing up locally - https://tbpl.mozilla.org/php/getParsedLog.php?id=34156658&tree=Try
Why did we change the layout view too? To match the highlighter?

I think it's much harder to understand which regions is which.

Can we do that in a followup bug?
(In reply to Paul Rouget [:paul] from comment #70)
> Why did we change the layout view too? To match the highlighter?
> 
> I think it's much harder to understand which regions is which.
> 
> Can we do that in a followup bug?

Because hovering each region moves the guides around that region and having a 2px border makes it difficult to hover the border.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #71)
> (In reply to Paul Rouget [:paul] from comment #70)
> > Why did we change the layout view too? To match the highlighter?
> > 
> > I think it's much harder to understand which regions is which.
> > 
> > Can we do that in a followup bug?
> 
> Because hovering each region moves the guides around that region and having
> a 2px border makes it difficult to hover the border.

hoooo! Nice. But still, the new look is confusing. What is content, border, margin, …?
I somewhat agree with Paul. This was one of my comments in the review.
I think Paul's idea to keep this for a follow-up bug is very good, especially since we want to uplift this to Aurora. Let's keep it as small as possible. What do you think?
Attachment #8370832 - Flags: approval-mozilla-aurora?
Attachment #8370835 - Flags: approval-mozilla-aurora?
We have decided to make the box model panel colors the same match the box model highlighter colors.

We also decided not to attempt to uplift this as it is a new feature and also because it is too large.
No longer depends on: 917755
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #74)
> We have decided to make the box model panel colors the same match the box
> model highlighter colors.

Sweet! Mockup?
(In reply to Paul Rouget [:paul] from comment #75)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #74)
> > We have decided to make the box model panel colors the same match the box
> > model highlighter colors.
> 
> Sweet! Mockup?

Why have a mockup when you can have the real thing? The layout view is now fixed so the guides now move around the hovered region. I have also given it the highlighter colors for easier identification of each region.

If you demo it then please use this patch as a lot of issues have been fixed.
Attachment #8370832 - Attachment is obsolete: true
Attachment #8370835 - Attachment is obsolete: true
Attached image Layout view.PNG
Darrin: Would you like me to change anything in the new layout view design? The colors now match the highlighted regions and the guides move around those regions when they are hovered in the layout view.
Attachment #8372221 - Flags: feedback?(dhenein)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #77)
> Created attachment 8372221 [details]
> Layout view.PNG


In my opinion, the layout view should stay as it is now, then the colors should show up when hovering the layout view.
Comment on attachment 8372221 [details]
Layout view.PNG

Looks great, thanks! 2 questions: 

1. Where did the current border UI go? Right now we have a bold(er) line representing the border and 4 labels to indicate the width of each border. Could we not have just colored that to match the highlighter?

2. Can we decrease the box shadow intensity by around 50% on the yellow box? I know we have that right now but its less visible on our current blue theme.
Attachment #8372221 - Flags: feedback?(dhenein)
Try:
https://tbpl.mozilla.org/?tree=Try&rev=c7fd883ced02

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #61)
> Comment on attachment 8370144 [details] [diff] [review]
> box-model-highlighter-663778.patch WIP
> 
> Review of attachment 8370144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, this is a preliminary review just looking at the code.
> Indeed, I haven't had the time to play with the new highlighter (with the
> patch applied) yet.
> 
> Mike: can you provide a new patch that uses the polyfill whenever you're
> ready, just so I can test with the code that will actually be used (if we
> manage to get this in Aurora).
> 

Done

> I have a few comments below, but I'm globally pretty happy with the patch so
> far.
> 
> ::: browser/base/content/browser.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> > +@namespace svg url("http://www.w3.org/2000/svg");
> 
> You've defined this @namespace in all of the browser.css files (this one and
> the themed ones), but you're actually using this namespace in
> highlighter.inc.css
> 
> Can the namespace be defined there instead? Maybe there's something about
> *.inc.css files that make this impossible, I don't know but I think these
> files are pre-processed in some ways.
> 

Unfortunately not. @namespace must come after any @charset and @import
rules and before any style rules. Because he .inc.css file literally becomes
part of browser.css, @namespace has to be declared at the top of that file.

> ::: browser/devtools/framework/selection.js
> @@ +165,5 @@
> > +    // set the node even if it is already set otherwise it is not possible to
> > +    // e.g. highlight the same node twice.
> > +    let rawValue = null;
> > +    if (value && value.isLocal_toBeDeprecated()) {
> > +      rawValue = value.rawNode();
> 
> Can you elaborate a bit more here?
> Why did you need to do that change? Did the outline highlighter suffered
> from the same problem?
> 
> I see one potential problem with doing this: the markup-view, rule-view,
> computed-view, font-inspector and layout-view will refresh again even if the
> same node is reselected.
> Indeed, most things in the inspector listen to
> selection.on("new-node-front") to refresh.
> Maybe it's not an issue but I'm afraid we have code that sets the selection
> with the same node without checking.
> 

Yes, the outline inspector suffers from the same issue. Without this change:
- Right-click a node and inspect element.
- Right-click the same node and try to inspect it again. Nothing happens, the
highlighter is not shown so to the user it seems broken.

> ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> @@ +53,5 @@
> >  
> >    function assertH1Highlighted() {
> > +    let deferred = promise.defer();
> > +
> > +    executeSoon(() => {
> 
> Why is this executeSoon needed? Can you add a comment?
> 

You are right. It turns out that the inspector is fully initialized before the
highlighter so doing this within openInspector() will cause highlighter tests to
fail:
  inspector.once("inspector-updated", runTests);
We have to use this instead:
  inspector.toolbox.once("node-highlight", runTests);

> ::: browser/devtools/inspector/test/browser_inspector_bug_674871.js
> @@ -49,5 @@
> >      openInspector(aInspector => {
> >        inspector = aInspector;
> >        // Make sure the highlighter is shown so we can disable transitions
> >        inspector.toolbox.highlighter.showBoxModel(getNodeFront(doc.body)).then(() => {
> > -        getHighlighterOutline().setAttribute("disable-transitions", "true");
> 
> It's a good thing we can get rid of these.
> And I do agree that transitions are not needed anymore with your patch, it
> would feel too "heavy" to have transitions on the box-model regions.
> 

Agreed.

> ::: browser/devtools/inspector/test/browser_inspector_highlighter.js
> @@ +65,5 @@
> >  
> >    EventUtils.synthesizeMouse(h1, 2, 2, {type: "mousemove"}, content);
> >  }
> >  
> >  function testOutlineDimensions() {
> 
> I think the name of the function should be changed since we don't have an
> outline anymore.
> 

Fixed

> @@ +99,4 @@
> >      let outlineWidth = Math.floor(outlineDims.width);
> >      let outlineHeight = Math.floor(outlineDims.height);
> >  
> > +    // TODO: Fix this when getBoxQuads() properly deals with zoom.
> 
> Since we're going to try and fix this bug with our polyfill, shouldn't we
> make it handle zoom too? In which case the 2 following asserts should stay.
> 

Yes, that is what I planned

> ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> @@ +54,5 @@
> >      aElement.ownerDocument.defaultView);
> >  }
> >  
> >  function runTests() {
> > +  executeSoon(testDiv1Highlighter);
> 
> Why is executeSoon needed here too? Can you add a comment if there's no
> other way.
> Maybe the promise returned by
> inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> 

We need to use executeSoon there to strike a balance between performance and
testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
used for the "picker-node-picked" event. Using this timer for
"picker-node-hover" causes the UI to become very sluggish. As to why pick has to
wait I have no idea.

> ::: browser/devtools/inspector/test/browser_inspector_scrolling.js
> @@ +33,5 @@
> >  {
> >    inspector = aInspector;
> >  
> > +  inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).then(() => {
> > +    performScrollingTest();
> 
> nit: please remove the arrow function:
> inspector.toolbox.highlighter.showBoxModel(getNodeFront(div)).
> then(performScrollingTest);
> 

Done

> @@ +54,5 @@
> >    }, false);
> > +
> > +  executeSoon(function() {
> > +    // FIXME: this will fail on retina displays. EventUtils will only scroll
> > +    // 25px down instead of 50.
> 
> Could you try using content.devicePixelRatio to make the test work on all
> types of screens?
> I know this isn't actually part of the bug, but it's been a problem for a
> long time and since you're touching the file, might as well try and put it
> in.
> 

I have added a simple patch to bug Bug 884796 to fix this. Because the fix is
in EventUtils robcee will need to review it as he is a testing peer.

> ::: browser/devtools/inspector/test/head.js
> @@ +74,3 @@
> >  
> > +function getBoxModelStatus()
> > +{
> 
> nit: please use the prevailing formatting style for the function opening
> brace.
> 

Done

> ::: browser/devtools/layoutview/view.css
> @@ +69,5 @@
> >    border-width: 25px;
> >  }
> >  
> >  #borders {
> > +  padding: 25px;
> 
> I noticed that with this change, the border region in the layout-view
> sidebar has changed, it used to be a thin solid line with box-shadow, now it
> looks exactly like the padding and margin regions.
> I like the fact that it's wider than before, it was hard to position the
> mouse over the thin (2px?) line before. But I liked the way it looked
> before, the fact that it was a plain line with shadow made it quicker to
> understand what you were looking it.
> 
> Could you try making it look like a bit more like it used to look while
> preserving the padding: 25px you added?
> Adding these rules to #borders {} could work:
> border-width: 2px;
> border-style: solid;
> box-shadow: 0 0 16px #000;
> But it's not perfect, better ask Darrin perhaps.
>

This is now colored the same as the highlighter regions. I have also pinged
Darrin for feedback.

> ::: browser/devtools/layoutview/view.js
> @@ +240,5 @@
> >      this._lastRequest = lastRequest;
> >      return this._lastRequest;
> > +  },
> > +
> > +  showBoxModel: function(options={}) {
> 
> showBoxModel and hideBoxModel can now be supplanted by:
> this.inspector.toolbox.highlighterUtils.highlightNodeFront()
> and
> this.inspector.toolbox.highlighterUtils.unhighlight()
> I've moved this code from markup-view to toolbox recently so that it avoids
> duplicating this nasty if/else logic all over the place.
> 
> Having said this, I don't see where these methods are being called. They
> don't seem to be!
> Did you intend to toggle the box model on mouse-over of the layout-view
> regions? I think this is an excellent idea (and it should only highlight the
> corresponding region, not the whole box-model), but if this isn't going to
> be part of this patch, better remove this code altogether (to keep the patch
> small).
> Since we're trying to land this so it can be uplifted, better file a
> separate bug to deal with this.
> 

Yes, I must have broken this on rebase. It is now fixed again.

> ::: browser/devtools/layoutview/view.xhtml
> @@ +29,5 @@
> >      </p>
> >  
> >      <div id="main">
> >  
> > +      <div id="margins" data-box="margin" tooltip="&margins.tooltip;">
> 
> These data attributes have probably been added to wire the
> showboxmodel-on-hover mechanism, and should be either used or removed (see
> my other comment in view.js).
> 

They are now used

> ::: toolkit/devtools/LayoutHelpers.jsm
> @@ +74,5 @@
> > +      return this._getBoxQuadsFromRect(offsetRect(paddingRect, "padding"));
> > +    }
> > +  },
> > +
> > +  getAbsoluteBoxQuads: function(node, options) {
> 
> I see you've moved the frame-offset logic to highlighter.js instead, so this
> function should be removed.
> 

Done

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +315,5 @@
> > +      padding: this._createSVGNode("padding", "polygon", this._boxModelContainer),
> > +      content: this._createSVGNode("content", "polygon", this._boxModelContainer)
> > +    };
> > +
> > +    this._guides = {
> 
> nit: please make the _boxModelNodes and _guides names consistent, so
> _guideNodes would be better maybe.
> 

Changed

> @@ +397,5 @@
> >        barHeight: barHeight,
> >      };
> >    },
> >  
> > +  _createSVGNode: function(boxType, svgNodeType, parent) {
> 
> nit: please rename boxType into className, since that's what it is. It's
> only a box type some of the times. Also, I don't think the svg part is
> needed in svgNodeType, just nodeType would be enough.
> 

Agreed... done.

> @@ +551,5 @@
> > +
> > +    options.box = options.box || "content";
> > +
> > +    // We do not cache the rectangle purely because comparing all 16 points
> > +    // unnecessarily complicates things.
> 
> I feel ok with not caching it anymore. If we do have perf problems in the
> future, this will be an easy change to make.
> However, for someone looking at this code and not knowing that we did have
> caching before, this comment might seem strange. So, it's only a detail, but
> I would remove it.
> 

Removed

> @@ +552,5 @@
> > +    options.box = options.box || "content";
> > +
> > +    // We do not cache the rectangle purely because comparing all 16 points
> > +    // unnecessarily complicates things.
> > +    let rect = this._getBounds("border");
> 
> I have to say it seems strange to me that the boxQuads API offers both
> bounds and points since it's basically the exact same information (or am I
> missing something?).
> I would rather use the points only throughout this class.
> 
> Also, you're calling getBounds here which, in turn, calls getAdjustedQuads
> (which loops through frames), knowing that getAdjustedQuads will be called
> again 4 times below in the for..of loop.
> You might want to rewrite this to reuse the result.
> 
> In fact, what we should do here is call getBoxQuads(boxType) 4 times, one
> for each type of box, and only then traverse the frames to offset all 4
> boxes (to traverse only once). Walking up the DOM isn't an expensive thing,
> but getting offsets on each element as we go is, so better minimize the
> number of times we do this (especially knowing this will be done on
> mouse-over, so, potentially very fast)
> 

Yeah, we now cache the quads and reuse when possible.

> @@ +592,5 @@
> > +                }
> > +                arr.splice(arr.lastIndexOf(val), 1);
> > +              }
> > +            }
> > +          }
> 
> Looking at these last 20 lines, it's really to get what's being done here.
> I would do 2 things:
> - extract these 20 lines to another (private) function, and give it a name
> and comment that explain what this is about
> - change the validX and validY names to something more self-explanatory.
> 

Done

> If I understand this correctly, you're trying to get the min X/Y and max
> X/Y, but only if the box hasn't been transformed in a way that the guides
> would not be parallel to the page's edges, right?

Yup

> For instance, if allY is [15, 10, 25, 30] (which could happen if the element
> is skewed), then there won't be any validY, which would end up hiding the
> top and bottom guides. Is my understanding correct?
> 

Spot on

> If yes, do we really want to hide the guides when the element is transformed?
> Indeed, they may not be super useful in this case. What was the reasoning
> behind this?
> 

We discussed this at length in a daily meeting and decided that the guides are
very useful for lining up transformed elements, especially considering that
hovering the different layout regions in the layout panel will move the guides
around that region.

> @@ +625,5 @@
> > +   *         The guide to update
> > +   * @param  {Integer} point
> > +   *         x or y co-ordinate
> > +   */
> > +  _updateGuide: function(guide, point=-1) {
> 
> Please add more comments to this function about the fact that this will hide
> if no point is passed.
> 

Done

> @@ +685,5 @@
> >    /**
> >     * Move the Infobar to the right place in the highlighter.
> >     */
> >    _moveInfobar: function() {
> > +    const offset = 5;
> 
> Can you move this const to the top of the file and give it a more explicit
> name?
> Something like INFO_BAR_OFFSET
> 

Done

> @@ +687,5 @@
> >     */
> >    _moveInfobar: function() {
> > +    const offset = 5;
> > +
> > +    let rect = this._getBounds("margin");
> 
> I'm again a little bit worried that we're calling getBounds again here,
> especially because _moveInfobar is called during _update, right after
> _highlightBoxModel is called.
> 
> The only other place where we call _moveInfobar is in _showInfobar, called
> itself in show, which also calls _update!
> 
> So basically, moveInfobar is called twice everytime we show the box model,
> and it is always called at the same time as highlightBoxModel, so we can
> probably make it accept a rect argument to avoid having it recompute it.
> 

Fixed

> @@ +797,5 @@
> > +        x: quads.p4.x + xOffset,
> > +        y: quads.p4.y + yOffset,
> > +        z: quads.p4.z
> > +      },
> > +      bounds: {
> 
> As I said in one of the comments in this file, if we decide not to use
> bounds and only rely on the points, we won't need this part.

getBounds has been removed
Attachment #8372220 - Attachment is obsolete: true
Attachment #8373569 - Flags: review?(pbrosset)
(In reply to Darrin Henein [:darrin] from comment #79)
> Comment on attachment 8372221 [details]
> Layout view.PNG
> 
> Looks great, thanks! 2 questions: 
> 
> 1. Where did the current border UI go? Right now we have a bold(er) line
> representing the border and 4 labels to indicate the width of each border.
> Could we not have just colored that to match the highlighter?
> 

We needed to make the border wider in order to give an area for the layout view
region mouseover behavior (the guides move to surround the area that you are
mousing over). If the border is only a couple of pixels wide it is difficult to
hover it.

See https://www.youtube.com/watch?v=1D_nwI46n0o

> 2. Can we decrease the box shadow intensity by around 50% on the yellow box?
> I know we have that right now but its less visible on our current blue theme.

Done
Comment on attachment 8373569 [details] [diff] [review]
Addressed review comments - tests pass locally

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

Thanks Mike for the new patch. I've applied it and played with it, it's looking really great.
The mouseover on the layout-view is pretty cool too.

Thanks for having addressed my comments and for your answers.
I just have a few remaining comments below, mostly about tests (indeed, I think the patch is functional and works great, we just need to focus on tests now).

r=me as soon as we manage to get it passing on try.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #80)
> Created attachment 8373569 [details] [diff] [review]
> Addressed review comments - tests pass locally
> 
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=c7fd883ced02
Seems to fail in debug builds with some highlighter tests.
I can help you on these since I spent quite a bit of time making them pass when working on the remote highlighter bug. Give me a shout if/when you need help.

> > ::: browser/devtools/framework/selection.js
> > @@ +165,5 @@
> > > +    // set the node even if it is already set otherwise it is not possible to
> > > +    // e.g. highlight the same node twice.
> > > +    let rawValue = null;
> > > +    if (value && value.isLocal_toBeDeprecated()) {
> > > +      rawValue = value.rawNode();
> > 
> > Can you elaborate a bit more here?
> > Why did you need to do that change? Did the outline highlighter suffered
> > from the same problem?
> > 
> > I see one potential problem with doing this: the markup-view, rule-view,
> > computed-view, font-inspector and layout-view will refresh again even if the
> > same node is reselected.
> > Indeed, most things in the inspector listen to
> > selection.on("new-node-front") to refresh.
> > Maybe it's not an issue but I'm afraid we have code that sets the selection
> > with the same node without checking.
> > 
> 
> Yes, the outline inspector suffers from the same issue. Without this change:
> - Right-click a node and inspect element.
> - Right-click the same node and try to inspect it again. Nothing happens, the
> highlighter is not shown so to the user it seems broken.
Ok got it. I was afraid this might cause mutliple refresh problems but as it turns out, the inspector sidebar tools don't refresh when the same node is selected again and it's nice to be able to click again (in the breadcrumbs for instance) and have the node highlight again briefly.

> > ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> > @@ +53,5 @@
> > >  
> > >    function assertH1Highlighted() {
> > > +    let deferred = promise.defer();
> > > +
> > > +    executeSoon(() => {
> > 
> > Why is this executeSoon needed? Can you add a comment?
> > 
> 
> You are right. It turns out that the inspector is fully initialized before
> the
> highlighter so doing this within openInspector() will cause highlighter
> tests to
> fail:
>   inspector.once("inspector-updated", runTests);
> We have to use this instead:
>   inspector.toolbox.once("node-highlight", runTests);
I don't understand why this can even work. Just before this line you do:
inspector.selection.setNode(doc.querySelector("h2"), null);
Passing |null| as a reason should not briefly highlight the node (by the way, can you change null into "test" instead? This is a new reason I've introduced specifically for tests), and therefore the node-highlight event shouldn't be fired.
In many tests I've worked on recently, the pattern I used that works well is:

inspector.once("inspector-updated", runTests);
inspector.selection.setNode(doc.querySelector("h2"), "test");

This should work fine normally.

> > ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> > @@ +54,5 @@
> > >      aElement.ownerDocument.defaultView);
> > >  }
> > >  
> > >  function runTests() {
> > > +  executeSoon(testDiv1Highlighter);
> > 
> > Why is executeSoon needed here too? Can you add a comment if there's no
> > other way.
> > Maybe the promise returned by
> > inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> > 
> 
> We need to use executeSoon there to strike a balance between performance and
> testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
> used for the "picker-node-picked" event. Using this timer for
> "picker-node-hover" causes the UI to become very sluggish. As to why pick
> has to
> wait I have no idea.
No, HIGHLIGHTER_PICKED_TIMER isn't used for the "picker-node-picked" event, the event is fired as soon as the node is picked, the timer is only used to wait for a bit before the highlighter goes away (so when you click on a node, it stays up for 1 second and then disappears).
For sure we shouldn't use this timer for any of the events.
So the comment you added in the file about this is wrong and should be removed.
So I still don't understand why executeSoon would be needed here, but TBH, I had a lot of problems related to timings with the highlighter myself when remoting it.
Let's leave it there for now, remove the comment, and anyway, since tests are still failing on try debug builds, I'll try and take a look with you.

::: browser/devtools/inspector/test/browser_inspector_bug_699308_iframe_navigation.js
@@ +18,5 @@
>      inspector.toolbox.highlighterUtils.startPicker().then(() => {
>        EventUtils.synthesizeMouse(content.document.body, 1, 1,
>          {type: "mousemove"}, content);
>        inspector.toolbox.once("picker-node-hovered", () => {
> +        cb();

nit: this can be rewritten:
inspector.toolbox.once("picker-node-hovered", cb);

::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
@@ +37,5 @@
> +        // We need to use executeSoon here to strike a balance between
> +        // performance and testability. highlighter.js::pick() contains
> +        // HIGHLIGHTER_PICKED_TIMER that is used for the "picker-node-picked"
> +        // event. Using this timer for the "picker-node-hover" causes the UI
> +        // to become very sluggish.

As explained in my comments, this should be removed.

@@ +54,5 @@
>  }
>  
>  function moveMouseOver(aElement, cb) {
>    inspector.toolbox.once("picker-node-hovered", () => {
> +    cb();

nit: can be rewritten:
inspector.toolbox.once("picker-node-hovered", cb);
Attachment #8373569 - Flags: review?(pbrosset)
Blocks: 971662
Attached patch patch: Fixed some tests (obsolete) — Splinter Review
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #82)
> Comment on attachment 8373569 [details] [diff] [review]
> Addressed review comments - tests pass locally
> 
> Review of attachment 8373569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Mike for the new patch. I've applied it and played with it, it's
> looking really great.
> The mouseover on the layout-view is pretty cool too.
> 
> Thanks for having addressed my comments and for your answers.
> I just have a few remaining comments below, mostly about tests (indeed, I
> think the patch is functional and works great, we just need to focus on
> tests now).
> 
> r=me as soon as we manage to get it passing on try.
> 
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #80)
> > Created attachment 8373569 [details] [diff] [review]
> > Addressed review comments - tests pass locally
> > 
> > Try:
> > https://tbpl.mozilla.org/?tree=Try&rev=c7fd883ced02
> Seems to fail in debug builds with some highlighter tests.
> I can help you on these since I spent quite a bit of time making them pass
> when working on the remote highlighter bug. Give me a shout if/when you need
> help.
> 

If you have time:
https://etherpad.mozilla.org/box-model-highlighter

> > > ::: browser/devtools/framework/selection.js
> > > @@ +165,5 @@
> > > > +    // set the node even if it is already set otherwise it is not possible to
> > > > +    // e.g. highlight the same node twice.
> > > > +    let rawValue = null;
> > > > +    if (value && value.isLocal_toBeDeprecated()) {
> > > > +      rawValue = value.rawNode();
> > > 
> > > Can you elaborate a bit more here?
> > > Why did you need to do that change? Did the outline highlighter suffered
> > > from the same problem?
> > > 
> > > I see one potential problem with doing this: the markup-view, rule-view,
> > > computed-view, font-inspector and layout-view will refresh again even if the
> > > same node is reselected.
> > > Indeed, most things in the inspector listen to
> > > selection.on("new-node-front") to refresh.
> > > Maybe it's not an issue but I'm afraid we have code that sets the selection
> > > with the same node without checking.
> > > 
> > 
> > Yes, the outline inspector suffers from the same issue. Without this change:
> > - Right-click a node and inspect element.
> > - Right-click the same node and try to inspect it again. Nothing happens, the
> > highlighter is not shown so to the user it seems broken.
> Ok got it. I was afraid this might cause mutliple refresh problems but as it
> turns out, the inspector sidebar tools don't refresh when the same node is
> selected again and it's nice to be able to click again (in the breadcrumbs
> for instance) and have the node highlight again briefly.
> 

Exactly.

> > > ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> > > @@ +53,5 @@
> > > >  
> > > >    function assertH1Highlighted() {
> > > > +    let deferred = promise.defer();
> > > > +
> > > > +    executeSoon(() => {
> > > 
> > > Why is this executeSoon needed? Can you add a comment?
> > > 
> > 
> > You are right. It turns out that the inspector is fully initialized before
> > the
> > highlighter so doing this within openInspector() will cause highlighter
> > tests to
> > fail:
> >   inspector.once("inspector-updated", runTests);
> > We have to use this instead:
> >   inspector.toolbox.once("node-highlight", runTests);
> I don't understand why this can even work. Just before this line you do:
> inspector.selection.setNode(doc.querySelector("h2"), null);
> Passing |null| as a reason should not briefly highlight the node (by the
> way, can you change null into "test" instead? This is a new reason I've
> introduced specifically for tests), and therefore the node-highlight event
> shouldn't be fired.
> In many tests I've worked on recently, the pattern I used that works well is:
> 
> inspector.once("inspector-updated", runTests);
> inspector.selection.setNode(doc.querySelector("h2"), "test");
> 
> This should work fine normally.
> 

Done

> > > ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> > > @@ +54,5 @@
> > > >      aElement.ownerDocument.defaultView);
> > > >  }
> > > >  
> > > >  function runTests() {
> > > > +  executeSoon(testDiv1Highlighter);
> > > 
> > > Why is executeSoon needed here too? Can you add a comment if there's no
> > > other way.
> > > Maybe the promise returned by
> > > inspector.toolbox.highlighterUtils.startPicker() somehow resolves too soon?
> > > 
> > 
> > We need to use executeSoon there to strike a balance between performance and
> > testability. highlighter.js::pick() contains HIGHLIGHTER_PICKED_TIMER that is
> > used for the "picker-node-picked" event. Using this timer for
> > "picker-node-hover" causes the UI to become very sluggish. As to why pick
> > has to
> > wait I have no idea.
> No, HIGHLIGHTER_PICKED_TIMER isn't used for the "picker-node-picked" event,
> the event is fired as soon as the node is picked, the timer is only used to
> wait for a bit before the highlighter goes away (so when you click on a
> node, it stays up for 1 second and then disappears).
> For sure we shouldn't use this timer for any of the events.
> So the comment you added in the file about this is wrong and should be
> removed.
> So I still don't understand why executeSoon would be needed here, but TBH, I
> had a lot of problems related to timings with the highlighter myself when
> remoting it.
> Let's leave it there for now, remove the comment, and anyway, since tests
> are still failing on try debug builds, I'll try and take a look with you.
> 

Okay, Mihai has logged bug 971785, which I suspect will fix a bunch of timing
issues in the highlighter. I will probably absorb that into this patch.

> :::
> browser/devtools/inspector/test/
> browser_inspector_bug_699308_iframe_navigation.js
> @@ +18,5 @@
> >      inspector.toolbox.highlighterUtils.startPicker().then(() => {
> >        EventUtils.synthesizeMouse(content.document.body, 1, 1,
> >          {type: "mousemove"}, content);
> >        inspector.toolbox.once("picker-node-hovered", () => {
> > +        cb();
> 
> nit: this can be rewritten:
> inspector.toolbox.once("picker-node-hovered", cb);
> 

Yeah, I have already changed that.

> ::: browser/devtools/inspector/test/browser_inspector_iframeTest.js
> @@ +37,5 @@
> > +        // We need to use executeSoon here to strike a balance between
> > +        // performance and testability. highlighter.js::pick() contains
> > +        // HIGHLIGHTER_PICKED_TIMER that is used for the "picker-node-picked"
> > +        // event. Using this timer for the "picker-node-hover" causes the UI
> > +        // to become very sluggish.
> 
> As explained in my comments, this should be removed.
> 

Agreed

> @@ +54,5 @@
> >  }
> >  
> >  function moveMouseOver(aElement, cb) {
> >    inspector.toolbox.once("picker-node-hovered", () => {
> > +    cb();
> 
> nit: can be rewritten:
> inspector.toolbox.once("picker-node-hovered", cb);

Done
Attachment #8373569 - Attachment is obsolete: true
Attached patch patch 1: Fixed more tests (obsolete) — Splinter Review
Merged fix from bug 971785
Attachment #8375516 - Attachment is obsolete: true
Attached image inline-boxes.png
I have a remark regarding inline boxes. In the screenshot, you see a link (display:inline <a> tag) that wraps on 2 lines.

In this case, the padding, border, margin apply to each of the line boxes, not the outer container.

I've added a red border on the link and a padding to illustrate the point.
You see that our current highlighter displays wrong information. Or, not really wrong, but not correctly positioned along the line boxes.

I checked in firebug and chrome, turns out they do the same as us.
So sticking to this representation may be less disturbing to users, but it's definitely not correct, and I think a highlighter that would correctly highlight wrapping inline boxes would help better understand how things work in CSS.

Mike: this comes from the polyfilled version of getBoxQuads that we use, for sure, but do you know if the final version will behave the same?
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #86)
> Created attachment 8376132 [details]
> inline-boxes.png
> 
> I have a remark regarding inline boxes. In the screenshot, you see a link
> (display:inline <a> tag) that wraps on 2 lines.
> 
> In this case, the padding, border, margin apply to each of the line boxes,
> not the outer container.
> 
> I've added a red border on the link and a padding to illustrate the point.
> You see that our current highlighter displays wrong information. Or, not
> really wrong, but not correctly positioned along the line boxes.
> 
> I checked in firebug and chrome, turns out they do the same as us.
> So sticking to this representation may be less disturbing to users, but it's
> definitely not correct, and I think a highlighter that would correctly
> highlight wrapping inline boxes would help better understand how things work
> in CSS.
> 
> Mike: this comes from the polyfilled version of getBoxQuads that we use, for
> sure, but do you know if the final version will behave the same?

Completely agree... all tools currently do this wrong. I remember researching this for Firebug but I never got around to implementing inline highlighting. Basically, *left and *right are honored by inlines but *top and *bottom are ignored. Also, the style is only applied to the start and end of the inline element.

The only way I could find was:
let range = document.createRange();
range.selectNodeContents(textNode);
let rects = range.getClientRects();
let textNodeLeft = rects[0].left;
...
And then building the correct shape out of a combination of the rects and the nodes computed style. This enhancement should be part of a new bug.
Attached patch patch 1: Fixed more tests (obsolete) — Splinter Review
Hopefully fixed debugger tests.
Attachment #8375555 - Attachment is obsolete: true
Added some logging to EventEmitter.emit in order to nail down try failures.
Attachment #8376282 - Attachment is obsolete: true
Comment on attachment 8376353 [details] [diff] [review]
patch 1: Drastically improved logging

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

As discussed in the meeting, I'm going through the code, to see if I can spot potential race conditions.

toolbox.js startPicker() and stopPicker() seem to also inconsistently resolve the promises they return. I dont know well this code, but here are my comments about it:

* startPicker() returns a promise that is resolved when done() is invoked - that happens after initInspector(), after selectTool(), and after highlighter.pick() (when isRemoteHighlightable).

* if !isRemoteHighlightable, the code does walker.pick(), but it doesnt wait for it, and it calls done(). Why? Not saying this is wrong, just making sure this isnt an error.

* a quick successive call to startPicker() starts the whole process again (before _isPicking=true). Is this safe/expected? Should it return the same promise? What is the expected behavior in such cases?

* if this._isPicking=true returns a resolved promise, which is inconsistent with the first call. The first call resolves *after* highlighter.pick(), but _isPicking is set to true *before* pick() happens. So any call during this time will have unexpected behavior. Why _isPicking set to true after initInspector()+selectTool()? I would've expected it to be set in done().

* stopPicker() is similar: it sets _isPicking=false after initInspector(), but the promise resolves when done() is invoked. The done() function is called when highlighter/walker.cancelPick() resolves. Maybe I am missing something, but these appear to be potential inconsistencies causing intermittent failures.

See below for an additional comments.

::: browser/devtools/framework/toolbox.js
@@ +1109,1 @@
>      return walker.then(outstanding, outstanding);

This doesnt look quite right. this._destroying should be:

this._destroying = walker.then(outstanding, outstanding);

so you can always return this._destroying. The current return is not the same as the _destroying promise you return at the beginning of the function.
Blocks: 850336
Please have the colors for border/padding/margin areas match the colors already used in firebug (preferably) or chrome dev tools. It makes it easier to switch back and forth between browsers while building pages.
Please don't make it "different" just for the sake of being different. Us developers don't want different. We wan't a complete equivalent to firebug that's fully integrated into Firefox and fully supported by Mozilla.
(In reply to thinsoldier from comment #93)
> Please have the colors for border/padding/margin areas match the colors
> already used in firebug (preferably) or chrome dev tools. It makes it easier
> to switch back and forth between browsers while building pages.

FWIW, while the exact hues vary, the colors used are consistent with what other browsers/tools are using: "orange" for margin, "yellow" for borders, "green" for padding and "blue" for content.
Blocks: 976505
Currently based on bug 974056 simply because it makes it easier to debug tests by uncommenting devtools.dump.emit in head.js
Attachment #8376353 - Attachment is obsolete: true
Fixes for a bunch of exceptions on shutdown. These fixes appear to improve the accuracy of the leak detector during test runs.
Attachment #8384688 - Attachment is obsolete: true
Attachment #8386692 - Flags: review?(pbrosset)
Changes to the layout view. This includes new styling and the ability to move highlighter guides as each region is hovered.
Attachment #8386693 - Flags: review?(pbrosset)
A few misc fixes:
1. Remove mouseover and mouseout removeEventListeners from the markup view as they are never added in the first place.
2. Remove reference to foldgutter.css from codemirror's editor.js as this missing file was causing hundreds of errors in test runs.
3. A couple of missing semicolons (I just can't help fixing them).
4. Fix querySelectorAll in the NodeListActor. Input from the markup view's search box is used as a querySelector in this method so this change swallows the exception in the case that a bad selector is entered.
Attachment #8386703 - Flags: review?(pbrosset)
Changes to tests.

I have disabled dom.send_after_paint_to_content for all devtools tests except tilt (just in case it Victor needs it). This pref was causing an infinite loop when the highlighter is displayed e.g. Show highlighter -> MozAfterPaint -> Show highlighter etc.

The rest of the changes are simple test fixes.
Attachment #8386704 - Flags: review?(pbrosset)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #100)
> Created attachment 8386704 [details] [diff] [review]
> 4-box-model-highlighter-663778.patch
> 
> Changes to tests.
> 
> I have disabled dom.send_after_paint_to_content for all devtools tests
> except tilt (just in case it Victor needs it). This pref was causing an
> infinite loop when the highlighter is displayed e.g. Show highlighter ->
> MozAfterPaint -> Show highlighter etc.
> 
> The rest of the changes are simple test fixes.

Actually, strike that, I had reordered the patches. This is the box model highlighter implementation.
Changes to tests.

I have disabled dom.send_after_paint_to_content for all devtools tests except tilt (just in case it Victor needs it). This pref was causing an infinite loop when the highlighter is displayed e.g. Show highlighter -> MozAfterPaint -> Show highlighter etc.

The rest of the changes are simple test fixes.
Attachment #8386708 - Flags: review?(pbrosset)
Some tests are still failing but this shouldn't hold up reviews any longer. See https://tbpl.mozilla.org/?tree=Try&rev=716cbf08fd6b for an example. I think that "ReferenceError: ok is not defined" is quite a classic.
Mihai just fixed something that this patch addressed, rebased.
Attachment #8386692 - Attachment is obsolete: true
Attachment #8386692 - Flags: review?(pbrosset)
Attachment #8386712 - Flags: review?(pbrosset)
Note that devtools.dump.emit is set to true at the moment for markup and inspector tests. This is just a temporary measure as it can be used to work out why tests are failing.
Comment on attachment 8386704 [details] [diff] [review]
4-box-model-highlighter-663778.patch

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

Thanks, I see that all my comments raised in the last review have been taken into account.
This patch seems general ok to me, I just have one somewhat important comment about the new inspector-panel update process. See my comments below.
Happy to R+ this patch with this resolved.

::: browser/devtools/inspector/inspector-panel.js
@@ +443,5 @@
>      progress.outstanding.add(done);
>      return done;
>    },
>  
> +  _onHighlighterReady: function() {

If I'm not mistaken, you've introduced this to make sure the "inspector-updated" event is fired only after the highlighter has moved to the node currently being inspected.

Several comments about this:

- it took me some time to understand the logic here. I think it would make it a lot easier to understand if the client-side part of the highlighter (HighlighterUtils in toolbox.js) would signal its process just like any other inspector tool:

let doneUpdating = this.inspector.updating("highlighter");

and then later, when the `highlighter-ready` event has been received:

doneUpdating();

This would make it consistent with the rule-view, breadcrumb, ...
It would also simplify the `checkDone` function (would be back to what it was before).

- another thing is (and I haven't looked into this too much so I might be wrong): what about when the highlighter isn't used? The update process will then never be done. Indeed, `this._highlighterReady` is only set to true when the `highlighter-ready` event is received, but if we're not highlighting the node when selecting it (I think this happens when we navigate to a new page and auto-select the default node), then it means `inspector-updated` will never be emitted, which may be bad.

::: toolkit/devtools/LayoutHelpers.jsm
@@ +420,5 @@
> +
> +
> +
> +  /********************************************************************
> +   * GetBoxQuads POLYFILL START

Can you add a comment explaining why we need it and giving the number of the bug we need fixed before removing this.

::: toolkit/devtools/server/actors/highlighter.js
@@ +498,5 @@
>        } else {
>          // Nothing to highlight (0px rectangle like a <script> tag for instance)
>          this.hide();
>        }
> +      events.emit(this._inspector.walker, "highlighter-ready");

Not a big deal but I think it would be better for the highlighter to emit its own events and then have the HighlighterActor do the bridge to the inspector. This would make the highlighter API more self-contained and would remove the need for the second constructor parameter.

this.emit("ready");

and then in the HighlighterActor:

this._boxModelHighlighter.on("ready", () => {
  events.emit(this._inspector.walker, "highlighter-ready");
});

Same comment for the `highlighter-hide` event.
Attachment #8386704 - Flags: review?(pbrosset)
Comment on attachment 8386712 [details] [diff] [review]
1-box-model-shutdown-fixes-changes-663778.patch

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

I have to admit I'm not very comfortable with these changes.

The goal is to make sure we don't throw various errors when the toolbox is closed.
I've seen this happen during tests a lot, but for this case, I'm pretty confident we can fix the tests themselves instead of adding defensive if conditions in certain functions. It especially feels strange when we're adding an `if(isDestroyed){return}` to one function only. Either we find a global way of making destroyed class instances safe to call, or we don't call their methods at all.
Again, if this is only a problem during tests, this should be easy to fix in each test (I'm fixing some of those in bug 968727).

It also seems to happen when quickly opening and closing the toolbox (via the keyboard shortcuts for instance), but fixing this should probably be done in a separate bug.

Checking that things still exist in a callback or promise handler seems fine because these are called asynchronously and an object may have been destroyed in the meantime, but in other cases, I think it would be better solved by tracing the issue back to the root cause rather than making some functions more defensive.

If I'm not mistaken, a lot of the errors are due to the inspector trying to select a new node and the toolbox being closed before everything can be updated accordingly. If this is indeed the case, it should be relatively easy to come up with something at inspector-panel level that tells us if we are being destroyed, and if yes, then selection.js shouldn't try to fire `new-node-front` events.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +251,5 @@
>      this.panel.hidePopup();
>    },
>  
>    isShown: function() {
> +    if (!this.panel) {

I see that most changes in this patch are essentially making the code more defensive so that we can avoid errors in certain situations like when closing a window or ending tests.
I'm not against adding this condition here, but it feels pretty strange to have it here and in no other function of this class. Why not in `show`, `hide`, `setSize`, `empty`, .... ?
I think the best solution would be to find out why `isShown` is being called *after* the panel has been removed.
Does this only happen when running tests?
If yes, I'm pretty sure it's easy to avoid. I've been working recently on refactoring tests, and most of time, just waiting for events solve these kinds of problems.

::: browser/devtools/webconsole/webconsole.js
@@ +408,5 @@
>      };
>  
> +    if (!this.webConsoleClient) {
> +      // Toolbox is in the process of shutting down.
> +      return deferred.resolve();

Can you move this `if` to the top of the function and return instead `promise.resolve()`.
Also maybe check with msucan why/when does `this.webConsoleClient` is falsy?

::: toolkit/devtools/server/actors/inspector.js
@@ +1191,5 @@
>      if (options.center && options.start) {
>        throw Error("Can't specify both 'center' and 'start' options.");
>      }
> +    if (!node) {
> +      return;

The return type of this protocol method is `array:domnode`.
I'm guessing it should be made nullable too, or maybe you should be returning an empty array here.

::: toolkit/devtools/server/actors/styles.js
@@ +446,5 @@
>      }
>    },
>  
>    getLayout: method(function(node, options) {
> +    if (!node) {

I think a better pattern for protocol methods is to return a rejected promise.
It avoids having to come up with a new json object return type.

::: toolkit/devtools/server/protocol.js
@@ +545,5 @@
>     * @returns a request packet.
>     */
>    write: function(fnArgs, ctx) {
>      let str = JSON.stringify(this.template, (key, value) => {
> +      if (value instanceof Arg && fnArgs) {

This seems harmless enough, but I guess worth asking dcamp's feedback, no?

::: toolkit/devtools/styleinspector/css-logic.js
@@ +170,5 @@
>  
>      this._matchedRules = null;
>      this._matchedSelectors = null;
>      let win = this.viewedDocument.defaultView;
> +    if (win) {

I think this check would be better at the top of the function, coupled with the already existing `if (!aViewedElement) {`
This if should both check if the the element exists and if it has a parent window.
Attachment #8386712 - Flags: review?(pbrosset)
Note that I haven't entirely reviewed attachment 8386712 [details] [diff] [review], I'd like your point of view on my general comment first, and maybe hear what other thinks.
Comment on attachment 8386693 [details] [diff] [review]
2-box-model-layout-view-changes-663778.patch

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

::: browser/devtools/layoutview/view.js
@@ +261,5 @@
>  let elts;
>  let tooltip;
>  
> +let onmouseover = function(e) {
> +  let region = e.target.getAttribute("data-box");

This should work: `e.target.dataset.box`
Attachment #8386693 - Flags: review?(pbrosset) → review+
Comment on attachment 8386693 [details] [diff] [review]
2-box-model-layout-view-changes-663778.patch

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

::: browser/devtools/layoutview/view.js
@@ +261,5 @@
>  let elts;
>  let tooltip;
>  
> +let onmouseover = function(e) {
> +  let region = e.target.getAttribute("data-box");

Note: if this is a XUL element, dataset will be undefined
Comment on attachment 8386703 [details] [diff] [review]
3-box-model-misc-fixes-changes-663778.patch

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

LGTM
Attachment #8386703 - Flags: review?(pbrosset) → review+
Comment on attachment 8386703 [details] [diff] [review]
3-box-model-misc-fixes-changes-663778.patch

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

LGTM

::: toolkit/devtools/server/actors/inspector.js
@@ +1405,5 @@
> +
> +    try {
> +      nodeList = baseNode.rawNode.querySelectorAll(selector);
> +    } catch(e) {
> +      // Bad selector. Do nothing as the selector can come from a searchbox.

Actually, I had originally done the same change in another bug but finally removed the try/catch because of this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=962647#c15
I think it makes more sense to keep the exception on the server side as it may be useful to client code to know that a selector was invalid.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #106)
> Comment on attachment 8386704 [details] [diff] [review]
> 4-box-model-highlighter-663778.patch
> 
> Review of attachment 8386704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, I see that all my comments raised in the last review have been taken
> into account.
> This patch seems general ok to me, I just have one somewhat important
> comment about the new inspector-panel update process. See my comments below.
> Happy to R+ this patch with this resolved.
> 
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +443,5 @@
> >      progress.outstanding.add(done);
> >      return done;
> >    },
> >  
> > +  _onHighlighterReady: function() {
> 
> If I'm not mistaken, you've introduced this to make sure the
> "inspector-updated" event is fired only after the highlighter has moved to
> the node currently being inspected.
> 
> Several comments about this:
> 
> - it took me some time to understand the logic here. I think it would make
> it a lot easier to understand if the client-side part of the highlighter
> (HighlighterUtils in toolbox.js) would signal its process just like any
> other inspector tool:
> 
> let doneUpdating = this.inspector.updating("highlighter");
> 
> and then later, when the `highlighter-ready` event has been received:
> 
> doneUpdating();
> 
> This would make it consistent with the rule-view, breadcrumb, ...
> It would also simplify the `checkDone` function (would be back to what it
> was before).
> 
> - another thing is (and I haven't looked into this too much so I might be
> wrong): what about when the highlighter isn't used? The update process will
> then never be done. Indeed, `this._highlighterReady` is only set to true
> when the `highlighter-ready` event is received, but if we're not
> highlighting the node when selecting it (I think this happens when we
> navigate to a new page and auto-select the default node), then it means
> `inspector-updated` will never be emitted, which may be bad.
> 

I have removed highlighter-ready from this.inspector.updating. We just don't
need it. If a tool only needs to wait for the highlighter it can listen for the
highlighter-ready event directly. This also addresses all of your comments
above.

> ::: toolkit/devtools/LayoutHelpers.jsm
> @@ +420,5 @@
> > +
> > +
> > +
> > +  /********************************************************************
> > +   * GetBoxQuads POLYFILL START
> 
> Can you add a comment explaining why we need it and giving the number of the
> bug we need fixed before removing this.
> 

Done

> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +498,5 @@
> >        } else {
> >          // Nothing to highlight (0px rectangle like a <script> tag for instance)
> >          this.hide();
> >        }
> > +      events.emit(this._inspector.walker, "highlighter-ready");
> 
> Not a big deal but I think it would be better for the highlighter to emit
> its own events and then have the HighlighterActor do the bridge to the
> inspector. This would make the highlighter API more self-contained and would
> remove the need for the second constructor parameter.
> 
> this.emit("ready");
> 
> and then in the HighlighterActor:
> 
> this._boxModelHighlighter.on("ready", () => {
>   events.emit(this._inspector.walker, "highlighter-ready");
> });
> 
> Same comment for the `highlighter-hide` event.

Agreed, done.

I also needed to add a pref (devtools.inspector.highlighter.autohide) to prevent the highlighter from hiding during tests. This is because it uses a timeout of 1000ms before hiding. If a server is under load and it takes e.g. markup-view more than 1000ms to send it's ready event then the highlighter is already hidden. That means that on a line directly after highlighting a node the highlighter may no longer be visible.
Attachment #8386704 - Attachment is obsolete: true
Attachment #8387657 - Flags: review?(jwalker)
Attached patch 1 Addressed comments (obsolete) — Splinter Review
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #107)
> Comment on attachment 8386712 [details] [diff] [review]
> 1-box-model-shutdown-fixes-changes-663778.patch
> 
> Review of attachment 8386712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have to admit I'm not very comfortable with these changes.
> 
> The goal is to make sure we don't throw various errors when the toolbox is
> closed.
> I've seen this happen during tests a lot, but for this case, I'm pretty
> confident we can fix the tests themselves instead of adding defensive if
> conditions in certain functions. It especially feels strange when we're
> adding an `if(isDestroyed){return}` to one function only. Either we find a
> global way of making destroyed class instances safe to call, or we don't
> call their methods at all.
> Again, if this is only a problem during tests, this should be easy to fix in
> each test (I'm fixing some of those in bug 968727).
> 
> It also seems to happen when quickly opening and closing the toolbox (via
> the keyboard shortcuts for instance), but fixing this should probably be
> done in a separate bug.
> 
> Checking that things still exist in a callback or promise handler seems fine
> because these are called asynchronously and an object may have been
> destroyed in the meantime, but in other cases, I think it would be better
> solved by tracing the issue back to the root cause rather than making some
> functions more defensive.
> 
> If I'm not mistaken, a lot of the errors are due to the inspector trying to
> select a new node and the toolbox being closed before everything can be
> updated accordingly. If this is indeed the case, it should be relatively
> easy to come up with something at inspector-panel level that tells us if we
> are being destroyed, and if yes, then selection.js shouldn't try to fire
> `new-node-front` events.
>

This is a good point and we should address it sometime but as discussed in the daily meeting this is caused by the toolbox being closed and opened again in between tests. We could ensure that we wait for all tools to finish dealing with all of their events before allowing the toolbox to close but that would result in a very slow response when closing the toolbox and when switching toolbox tabs, which is not good for users. For now let's just prevent the exceptions that are occurring... this is just a workaround for those issues to make real errors easy to find in test logs.
 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +251,5 @@
> >      this.panel.hidePopup();
> >    },
> >  
> >    isShown: function() {
> > +    if (!this.panel) {
> 
> I see that most changes in this patch are essentially making the code more
> defensive so that we can avoid errors in certain situations like when
> closing a window or ending tests.
> I'm not against adding this condition here, but it feels pretty strange to
> have it here and in no other function of this class. Why not in `show`,
> `hide`, `setSize`, `empty`, .... ?
> I think the best solution would be to find out why `isShown` is being called
> *after* the panel has been removed.
> Does this only happen when running tests?
> If yes, I'm pretty sure it's easy to avoid. I've been working recently on
> refactoring tests, and most of time, just waiting for events solve these
> kinds of problems.
> 

See previous comment.

> ::: browser/devtools/webconsole/webconsole.js
> @@ +408,5 @@
> >      };
> >  
> > +    if (!this.webConsoleClient) {
> > +      // Toolbox is in the process of shutting down.
> > +      return deferred.resolve();
> 
> Can you move this `if` to the top of the function and return instead
> `promise.resolve()`.
> Also maybe check with msucan why/when does `this.webConsoleClient` is falsy?
> 

Done

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1191,5 @@
> >      if (options.center && options.start) {
> >        throw Error("Can't specify both 'center' and 'start' options.");
> >      }
> > +    if (!node) {
> > +      return;
> 
> The return type of this protocol method is `array:domnode`.
> I'm guessing it should be made nullable too, or maybe you should be
> returning an empty array here.
> 

Yup, now returning an empty array.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +446,5 @@
> >      }
> >    },
> >  
> >    getLayout: method(function(node, options) {
> > +    if (!node) {
> 
> I think a better pattern for protocol methods is to return a rejected
> promise.
> It avoids having to come up with a new json object return type.
> 

Done

> ::: toolkit/devtools/server/protocol.js
> @@ +545,5 @@
> >     * @returns a request packet.
> >     */
> >    write: function(fnArgs, ctx) {
> >      let str = JSON.stringify(this.template, (key, value) => {
> > +      if (value instanceof Arg && fnArgs) {
> 
> This seems harmless enough, but I guess worth asking dcamp's feedback, no?
> 

Emailed Dave

> ::: toolkit/devtools/styleinspector/css-logic.js
> @@ +170,5 @@
> >  
> >      this._matchedRules = null;
> >      this._matchedSelectors = null;
> >      let win = this.viewedDocument.defaultView;
> > +    if (win) {
> 
> I think this check would be better at the top of the function, coupled with
> the already existing `if (!aViewedElement) {`
> This if should both check if the the element exists and if it has a parent
> window.

Not possible. To get win we need this.viewedDocument which is what the core of this method is doing.
Attachment #8386712 - Attachment is obsolete: true
Attachment #8387662 - Flags: review?(jwalker)
Comment on attachment 8386708 [details] [diff] [review]
5-box-model-test-changes-663778.patch

I will go over these tests again now that we have a green try as we probably don't need all changes.
Attachment #8386708 - Flags: review?(pbrosset)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #113)
> I also needed to add a pref (devtools.inspector.highlighter.autohide) to
> prevent the highlighter from hiding during tests. This is because it uses a
> timeout of 1000ms before hiding. If a server is under load and it takes e.g.
> markup-view more than 1000ms to send it's ready event then the highlighter
> is already hidden. That means that on a line directly after highlighting a
> node the highlighter may no longer be visible.

(I didn't look at the actual change so take this with a grain of salt, but) Keep in mind that new prefs make browser startup slower, so if this is something that only tests need, it might be better to use a property in the highlighter that the test preamble could set unconditionally.
(In reply to Panos Astithas [:past] from comment #116)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #113)
> > I also needed to add a pref (devtools.inspector.highlighter.autohide) to
> > prevent the highlighter from hiding during tests. This is because it uses a
> > timeout of 1000ms before hiding. If a server is under load and it takes e.g.
> > markup-view more than 1000ms to send it's ready event then the highlighter
> > is already hidden. That means that on a line directly after highlighting a
> > node the highlighter may no longer be visible.
> 
> (I didn't look at the actual change so take this with a grain of salt, but)
> Keep in mind that new prefs make browser startup slower, so if this is
> something that only tests need, it might be better to use a property in the
> highlighter that the test preamble could set unconditionally.

There have also been a few user requests for this feature so allowing them to set it via about:config is probably a win. Also, that change has already landed as part of another bug.
Attachment #8387662 - Attachment is obsolete: true
Attachment #8387662 - Flags: review?(jwalker)
pbrosset has already reviewed this but I have added the devtools.inspector.highlighter.autohide pref as the highlighter hiding itself was our last race condition.
Attachment #8387657 - Attachment is obsolete: true
Attachment #8387657 - Flags: review?(jwalker)
Attachment #8388263 - Flags: review?(jwalker)
These are the most repetitive changes to tests. I broke this out from the main test patch as it makes the review way easier.
Attachment #8386708 - Attachment is obsolete: true
Attachment #8388265 - Flags: review?(jwalker)
Main test changes.
Attachment #8388266 - Flags: review?(jwalker)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #117)
> There have also been a few user requests for this feature so allowing them
> to set devtools.inspector.highlighter.autohide via about:config is probably
> a win. Also, that change has already landed as part of another bug.

I am wrong, this did not land as part of another bug. Personally I think a pref makes the most sense but if jwalker thinks it should be a toggle I am happy to change it.
Comment on attachment 8388263 [details] [diff] [review]
3-box-model-highlighter-663778.patch

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

::: browser/devtools/framework/toolbox.js
@@ +1162,5 @@
>  
>      // Releasing the walker (if it has been created)
>      // This can fail, but in any case, we want to continue destroying the
>      // inspector/highlighter/selection
> +    let walker = this._destroying = this._walker ? this._walker.release() : promise.resolve();

It looks like we might have blown 80 columns and there's a confusion as to if the double equals is double assignment.
So how about:

let walker = (this._destroying = this._walker) ?
             this._walker.release() :
             promise.resolve();
Attachment #8388263 - Flags: review?(jwalker) → review+
Comment on attachment 8388265 [details] [diff] [review]
4-box-model-tests-charset-mozafterpaint-changes-663778.patch

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

::: browser/devtools/markupview/test/head.js
@@ +14,5 @@
>  
> +// dom.send_after_paint_to_content is set to true (non-default) in
> +// testing/profiles/prefs_general.js so lets set it to the same as it is in
> +// a default browser profile for the duration of the test.
> +Services.prefs.setBoolPref("dom.send_after_paint_to_content", false);

Do we actually need to do this everywhere?
Attachment #8388265 - Flags: review?(jwalker) → review+
Comment on attachment 8388266 [details] [diff] [review]
5-box-model-test-changes-663778.patch

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

::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
@@ +33,5 @@
>      Task.spawn(function() {
>        yield hoverH1InMarkupView();
>        yield assertH1Highlighted();
> +
> +      // We no longer hide the highlighter during tests so no need to check that

In general we shouldn't be commenting on how it used to be unless that's vital to understanding now. We've got VCS to explain deletions.
Attachment #8388266 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #125)
> Comment on attachment 8388263 [details] [diff] [review]
> 3-box-model-highlighter-663778.patch
> 
> Review of attachment 8388263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +1162,5 @@
> >  
> >      // Releasing the walker (if it has been created)
> >      // This can fail, but in any case, we want to continue destroying the
> >      // inspector/highlighter/selection
> > +    let walker = this._destroying = this._walker ? this._walker.release() : promise.resolve();
> 
> It looks like we might have blown 80 columns and there's a confusion as to
> if the double equals is double assignment.
> So how about:
> 
> let walker = (this._destroying = this._walker) ?
>              this._walker.release() :
>              promise.resolve();

Makes sense, fixed.

(In reply to Joe Walker [:jwalker] from comment #127)
> Comment on attachment 8388266 [details] [diff] [review]
> 5-box-model-test-changes-663778.patch
> 
> Review of attachment 8388266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> @@ +33,5 @@
> >      Task.spawn(function() {
> >        yield hoverH1InMarkupView();
> >        yield assertH1Highlighted();
> > +
> > +      // We no longer hide the highlighter during tests so no need to check that
> 
> In general we shouldn't be commenting on how it used to be unless that's
> vital to understanding now. We've got VCS to explain deletions.

(In reply to Joe Walker [:jwalker] from comment #126)
> Comment on attachment 8388265 [details] [diff] [review]
> 4-box-model-tests-charset-mozafterpaint-changes-663778.patch
> 
> Review of attachment 8388265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/test/head.js
> @@ +14,5 @@
> >  
> > +// dom.send_after_paint_to_content is set to true (non-default) in
> > +// testing/profiles/prefs_general.js so lets set it to the same as it is in
> > +// a default browser profile for the duration of the test.
> > +Services.prefs.setBoolPref("dom.send_after_paint_to_content", false);
> 
> Do we actually need to do this everywhere?

We had discussed this in our daily meetings and the general consensus was that in the future tools will most likely involve tests that use the highlighter. Victor also wanted it enabled for Tilt tests because flooding things with useless MozAfterPaint stuff is not a good idea.
Rebased
Attachment #8388263 - Attachment is obsolete: true
Attachment #8389828 - Flags: review+
Moved mozafterpaint out to new patch as we are now using a flag instead of a pref.
Attachment #8388265 - Attachment is obsolete: true
Attachment #8389830 - Flags: review+
Rebased
Attachment #8388266 - Attachment is obsolete: true
Attachment #8389831 - Flags: review+
Attached patch 6-testing-flag-663778.patch (obsolete) — Splinter Review
Moved mozafterpaint fix out to new patch as we are now using a flag instead of a pref.

Our head.js files now contain gDevTools.testing = true which prevents the highlighter from hiding and disables dom.send_after_paint_to_content, which we agreed in a meeting should be false for all of our tools because they are all likely to interact with the highlighter at some point.

Try run to follow.
Attachment #8389835 - Flags: review?(jwalker)
Comment on attachment 8389835 [details] [diff] [review]
6-testing-flag-663778.patch

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

::: browser/devtools/profiler/test/head.js
@@ +28,3 @@
>  registerCleanupFunction(function () {
>    helpers = null;
> +  gDevTools.testing = false;

This bugged me before and it still bugs me...
Sometimes we integrate into existing clean-up functions, and sometimes we have a new one. I think it makes sense if we keep them the same system everywhere rather than merging them into existing clean-up function, unless there is some cost in a clean-up function that I'm not aware of. Using the same block everywhere will help us remove them all at the same time.

::: browser/devtools/styleinspector/test/head.js
@@ +30,5 @@
> +SimpleTest.registerCleanupFunction(() => {
> +  gDevTools.testing = false;
> +
> +  Services.prefs.clearUserPref("devtools.debugger.log");
> +  Services.prefs.clearUserPref("devtools.dump.emit");

This shouldn't be turned on by default?
Attachment #8389835 - Flags: review?(jwalker) → review+
Attached patch 6-testing-flag-663778.patch (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #133)
> Comment on attachment 8389835 [details] [diff] [review]
> 6-testing-flag-663778.patch
> 
> Review of attachment 8389835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/profiler/test/head.js
> @@ +28,3 @@
> >  registerCleanupFunction(function () {
> >    helpers = null;
> > +  gDevTools.testing = false;
> 
> This bugged me before and it still bugs me...
> Sometimes we integrate into existing clean-up functions, and sometimes we
> have a new one. I think it makes sense if we keep them the same system
> everywhere rather than merging them into existing clean-up function, unless
> there is some cost in a clean-up function that I'm not aware of. Using the
> same block everywhere will help us remove them all at the same time.
> 

I am happier with it in one block too... done.

> ::: browser/devtools/styleinspector/test/head.js
> @@ +30,5 @@
> > +SimpleTest.registerCleanupFunction(() => {
> > +  gDevTools.testing = false;
> > +
> > +  Services.prefs.clearUserPref("devtools.debugger.log");
> > +  Services.prefs.clearUserPref("devtools.dump.emit");
> 
> This shouldn't be turned on by default?

If you mean devtools.dump.emit then no, it is amazingly useful for debugging issues but it logs a lot of information that makes test logs difficult to read.

It is a simple matter of switching it on in head.js, pushing to try, reproducing the orange and then reading the log.
Attachment #8389835 - Attachment is obsolete: true
Attachment #8390574 - Flags: review+
Added a null check in a destructor to fix a webconsole test and return null from _highlightBoxModel to fix XPCShell test failures.
Attachment #8389830 - Attachment is obsolete: true
Attachment #8390576 - Flags: review+
Added null check to destructor.
Attachment #8389828 - Attachment is obsolete: true
Attachment #8390854 - Flags: review+
Fixed a number of tests.
Attachment #8390574 - Attachment is obsolete: true
Attachment #8390856 - Flags: review+
Because these changes uncover leaks in unrelated tests we can't wait for try runs to complete before pushing as new leaks are bound to appear (it turned out to be 3 different leaks that caused the previous set of patches to bounce).

bc and xpcshell tests are all green locally so all we can do is land when not many others are landing... fingers crossed.

https://hg.mozilla.org/integration/fx-team/rev/c20ad6e9c62e
https://hg.mozilla.org/integration/fx-team/rev/9b860b0a7282
https://hg.mozilla.org/integration/fx-team/rev/8dea188bf8a0
https://hg.mozilla.org/integration/fx-team/rev/fad8f0885e5b
https://hg.mozilla.org/integration/fx-team/rev/acf2659a7478
https://hg.mozilla.org/integration/fx-team/rev/436a9dfc6cb4
Whiteboard: [fixed-in-fx-team]
Depends on: 983933
Depends on: 983935
Depends on: 983948
Depends on: 984918
Blocks: 968727
Depends on: 984848
Blocks: 982822
Blocks: 988102
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.