[Layout] Implement an abstract view of the layout of the selected node

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

({dev-doc-complete})

Trunk
Firefox 14
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 disabled, firefox15 fixed)

Details

(Whiteboard: [minotaur][firefox14-wanted])

Attachments

(2 attachments, 14 obsolete attachments)

312.82 KB, image/jpeg
Details
69.75 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
This view should live into a Panel.
(Assignee)

Updated

6 years ago
Whiteboard: [minotaur]
(Assignee)

Updated

6 years ago
Depends on: 674871, 663831
(Assignee)

Updated

6 years ago
Blocks: 663778
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

6 years ago
Created attachment 557568 [details]
Possible design
(Assignee)

Comment 2

6 years ago
See bug 663778 for a preliminary patch.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
Depends on bug 663831 because the tool will register itself in initTools.
(Assignee)

Comment 4

6 years ago
Created attachment 558438 [details] [diff] [review]
patch v0.1 WIP

The code lie in the devtools/ folder.
I use a floating panel.

The current implementation has 2 bugs I can't explain yet:
x The 2nd time the panel is select to be dragged, it becomes "click insensitive" (only on Mac). I believe we have the same problem with the HTML panel.
x Some times, the SVG document is not refresh correctly (screenshot to come)
(Assignee)

Comment 5

6 years ago
Created attachment 558439 [details]
panel bug (document not completly refreshed)
(Assignee)

Comment 6

6 years ago
Created attachment 559427 [details] [diff] [review]
patch v0.2 WIP rebase (new register tool API)
Attachment #558438 - Attachment is obsolete: true
I came across this CSS grid today: http://leaverou.me/css3patterns/#blueprint-grid
It doesn't look as good as your SVG one, but you might want to borrow some ideas for a simpler/faster solution.
(Assignee)

Updated

6 years ago
Blocks: 663830
(Assignee)

Comment 8

6 years ago
Created attachment 565927 [details] [diff] [review]
patch v0.3 WIP rebase
Attachment #559427 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 674887
(Assignee)

Comment 9

6 years ago
Created attachment 567764 [details] [diff] [review]
patch v1

The View is included in a <panel>. This is a temporary situtation. It will be
moved to a better place in bug 674887.
(Assignee)

Updated

6 years ago
Attachment #567764 - Flags: review?(rcampbell)
Comment on attachment 567764 [details] [diff] [review]
patch v1

In LayoutView.jsm:

+function LayoutView(aContentWindow) {
+  this._init(aContentWindow);
+}

Nit: for function definitions, we usually put the open bracket on the following line. (Not my style choice but, pretty prevalent across mozcode).

+  onIframeReady: function() {

should include the message signature here and everywhere.

function LayoutView_onIframeReady() {

One pattern we've found useful is to pass in the instance of the InspectorUI and pull out the chrome and/or content windows from that. Also allows us to move the registration inside the tool itself at initialization time which cuts down on the amount that the Inspector has to know about the tool itself.

+    this._iframeReady = false;
+    this._iframe.addEventListener("load", this.onIframeReady.bind(this), true);

You should cache the bound method onIframeReady with something like:

this.boundIframeReady = this.onIframeReady.bind(this);

so you can remove it in the listener.

nit:

+  /**
+    * Select a node to draw
+    *
+    * @param aNode The node to draw

don't need the extra gap between Select comment and the @param line. You should also use punctuation in your comment lines, e.g., end your sentences with '.'.

+
+  hide: function() {

needs a javadoc.

typo:

+    * Hide and destrou the popup
                destroy
+  unregister: function() {
+    this.hide();

do we know the popup is being shown?

+    this._iframe = null;

you should probably removeChild() this from the panel before deleting the reference. This caused problems in the style inspector before.

should include function LayoutView_unregister()

in _computeLayout:
+      l.top = parseInt(valuetop);
+      l.right = parseInt(valueright);
+      l.bottom = parseInt(valuebottom);
+      l.left = parseInt(valueleft);

do we want to use parseInt for these? We're potentially throwing away fractions here.

+    layout.boundingbox.width = Math.round(rect.width);
+    layout.boundingbox.height = Math.round(rect.height);
+    layout.boundingbox.top = Math.round(rect.top);
+    layout.boundingbox.left = Math.round(rect.left);

and here, except you're rounding up. Not sure if these are going to ever be relevant.

+   * Memonized lookup of a l10n string from a string bundle.

I think you mean "Memoized". (I'm correcting those strings in the styleinspector.jsm too).

in layoutview.xhtml:

+    const RATIO_THRESHOLD = 0.3;

what's this? (ok, I found it later on, but maybe a comment would help)

+      if (ratio < 1 - RATIO_THRESHOLD) {

ratio < 2/3 ?

+      } else {
+        if (ratio > 1 + RATIO_THRESHOLD) {

You could rewrite this as:

} else if (ratio > 1 + RATIO_THRESHOLD) {

and move:

+        } else {

out of the nested if.

...

no issues with css, but you should ask dao.

in browser/themes/winstripe/browser/jar.mn:

+        skin/classic/browser/devtools/layoutview.css              (devtools/layoutview.css)

should be skin/classic/aero/browser/devtools/layoutview.css

Great start, this is going to be awesome. R- for now because of the method signatures, missing comments and various nits.
Attachment #567764 - Flags: review?(rcampbell) → review-
(Assignee)

Comment 11

6 years ago
Created attachment 568023 [details] [diff] [review]
patch v1.1

Thank you Rob for the review. I addressed most of your comments.

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 567764 [details] [diff] [review] [diff] [details] [review]
> patch v1
> You should cache the bound method onIframeReady with something like:
> 
> this.boundIframeReady = this.onIframeReady.bind(this);
> 
> so you can remove it in the listener.

Ok, I now remove the listener. But is that really useful since the listener
won't hold any strong reference to the iframe (the iframe get destroyed at
the end)?

> in _computeLayout:
> +      l.top = parseInt(valuetop);
> +      l.right = parseInt(valueright);
> +      l.bottom = parseInt(valuebottom);
> +      l.left = parseInt(valueleft);
> 
> do we want to use parseInt for these? We're potentially throwing away
> fractions here.
> 
> +    layout.boundingbox.width = Math.round(rect.width);
> +    layout.boundingbox.height = Math.round(rect.height);
> +    layout.boundingbox.top = Math.round(rect.top);
> +    layout.boundingbox.left = Math.round(rect.left);
> 
> and here, except you're rounding up. Not sure if these are going to ever be
> relevant.

These values are only displayed (not involved in any computation), that is why
I don't want any fractionned values.

> +      if (ratio < 1 - RATIO_THRESHOLD) {
> 
> ratio < 2/3 ?

Sorry?
(Assignee)

Updated

6 years ago
Attachment #567764 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #565927 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #568023 - Flags: review?(rcampbell)
(Assignee)

Comment 12

6 years ago
Comment on attachment 568023 [details] [diff] [review]
patch v1.1

Dao, can you please take a look at the CSS (it's the same for Linux, Mac and Windows)? Thanks!
Attachment #568023 - Flags: review?(dao)
Comment on attachment 568023 [details] [diff] [review]
patch v1.1

>+  <meta charset="UTF-8" />

This is invalid html.

>+<!ENTITY nonodeselected     "No node selected">

noNodeSelected

>+<!ENTITY dimension          "dimension">
>+<!ENTITY margins            "margins">
>+<!ENTITY borders            "borders">
>+<!ENTITY padding            "padding">
>+<!ENTITY content            "content">

These could probably use some l10n instructions and/or clearer entity names.

>+++ b/browser/themes/gnomestripe/browser/devtools/layoutview.css

>+body {
>+  width: 300px;
>+  height: 300px;

As I understand it, these numbers are fixed, i.e. it wouldn't make sense for a theme to change them. If so, please move them to a content stylesheet.

>+body:hover > #marginbox:not(:hover) > #borderbox:not(:hover) > #paddingbox:not(:hover) > #contentbox:not(:hover) > #dimension-label {

When #marginbox is not hovered, it's impossible for its children to be hovered, is it?
Attachment #568023 - Flags: review?(dao) → review-

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 14

6 years ago
Created attachment 568706 [details] [diff] [review]
patch v1.2

Addressed Dao's comments.
(Assignee)

Updated

6 years ago
Attachment #568023 - Attachment is obsolete: true
Attachment #568023 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Attachment #568706 - Flags: review?(rcampbell)
Attachment #568706 - Flags: checkin?(dao)
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

removing checkin flag until review is complete.
Attachment #568706 - Flags: checkin?(dao)
(Assignee)

Updated

6 years ago
Attachment #568706 - Flags: review?(dao)
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

>--- a/browser/devtools/jar.mn
>+++ b/browser/devtools/jar.mn
>@@ -2,9 +2,10 @@ browser.jar:
> *   content/browser/inspector.html                (highlighter/inspector.html)
>     content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
> *   content/browser/scratchpad.xul                (scratchpad/scratchpad.xul)
> *   content/browser/scratchpad.js                 (scratchpad/scratchpad.js)
>     content/browser/csshtmltree.xhtml             (styleinspector/csshtmltree.xhtml)
>     content/browser/orion.js                      (sourceeditor/orion/orion.js)
>     content/browser/orion.css                     (sourceeditor/orion/orion.css)
>     content/browser/orion-mozilla.css             (sourceeditor/orion/mozilla.css)
>-
>+*   content/browser/layoutview.xhtml              (layoutview/layoutview.xhtml)
>+    content/browser/layoutview.css                (layoutview/layoutview.css)

Why are these folded into content/browser/ rather than content/browser/devtools/?
Comment on attachment 568706 [details] [diff] [review]
patch v1.2

>+++ b/browser/themes/gnomestripe/browser/devtools/layoutview.css

>+#textbox,
>+#arrowbox {
>+  pointer-events: none;
>+  position: absolute;
>+  top: 0; left: 0;

line break after ;

>+  width: 300px;
>+  height: 300px;

>+.square > #marginbox { margin: 39px 40px 40px 39px; }
>+.vertical > #marginbox { margin: 39px 60px 40px 59px; }
>+.horizontal > #marginbox { margin: 59px 40px 60px 39px; }

>+#arrowbox .vertical::before,
>+#arrowbox .vertical::after {
>+  content: "";
>+  display: block;
>+  position: absolute;
>+  width: 4px;
>+  height: 4px;

>+#arrowbox .vertical::before {
>+  top: 0px;
>+  -moz-transform: rotate(45deg);
>+}
>+
>+#arrowbox .vertical::after {
>+  bottom: 0px;
>+  -moz-transform: rotate(225deg);
>+}

>+.square > #arrowbox > .top {
>+  top: 8px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.square > #arrowbox > .left {
>+  top: 39px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.square > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.square > #arrowbox > .height {
>+  right: 27px;
>+  top: 70px;
>+  height: 160px;
>+}
>+
>+/* square */
>+
>+.square > #arrowbox > .top {
>+  top: 8px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.square > #arrowbox > .left {
>+  top: 39px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.square > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.square > #arrowbox > .height {
>+  right: 27px;
>+  top: 70px;
>+  height: 160px;
>+}
>+
>+/* horizontal */
>+
>+.horizontal > #arrowbox > .top {
>+  top: 28px;
>+  left: 39px;
>+  height: 24px;
>+}
>+
>+.horizontal > #arrowbox > .left {
>+  top: 59px;
>+  left: 8px;
>+  width: 24px;
>+}
>+
>+.horizontal > #arrowbox > .width {
>+  bottom: 47px;
>+  left: 70px;
>+  width: 160px;
>+}
>+
>+.horizontal > #arrowbox > .height {
>+  right: 27px;
>+  top: 90px;
>+  height: 120px;
>+}
>+
>+/* vertical */
>+
>+.vertical > #arrowbox > .top {
>+  top: 8px;
>+  left: 59px;
>+  height: 24px;
>+}
>+
>+.vertical > #arrowbox > .left {
>+  top: 39px;
>+  left: 28px;
>+  width: 24px;
>+}
>+
>+.vertical > #arrowbox > .width {
>+  bottom: 27px;
>+  left: 90px;
>+  width: 120px;
>+}
>+
>+.vertical > #arrowbox > .height {
>+  right: 47px;
>+  top: 70px;
>+  height: 160px;
>+}

>+body[nonode] #marginbox,
>+body[nonode] #arrowbox,
>+body[nonode] #textbox > p {
>+  visibility: hidden;
>+}
>+body[nonode] #textbox > #nonodemessage {
>+  visibility: visible;
>+}

>+.label {
>+  pointer-events: none;

>+#dimension-label {
>+  top: 4px;
>+  right: 4px;
>+}
>+
>+#margin-label {
>+  top: 40px;
>+  right: 45px;
>+}
>+
>+#border-label {
>+  top: 70px;
>+  right: 75px;
>+}
>+
>+#padding-label {
>+  top: 90px;
>+  right: 95px;
>+}
>+
>+#content-label {
>+  top: 110px;
>+  right: 115px;
>+}
>+
>+.horizontal .label:not(#dimension-label) {
>+  margin-top: 20px;
>+}
>+
>+.vertical .label:not(#dimension-label) {
>+  top: 4px;
>+  right: 4px;
>+}

>+#nonodemessage {
>+  width: 300px;
>+  line-height: 300px;
>+  visibility: hidden;
>+}
>+
>+/* Values */
>+
>+#boundingbox-top {
>+  top: 14px;
>+  left: 48px;
>+  width: 40px;
>+  text-align: left!important;
>+}
>+
>+#boundingbox-left {
>+  top: 48px;
>+  left: 0px;
>+  width: 40px;
>+}
>+
>+#boundingbox-height {
>+  width: 25px;
>+  right: 0px;
>+}
>+
>+#boundingbox-width {
>+  bottom: 8px;
>+}
>+
>+.vertical #boundingbox-height {
>+  right: 20px;
>+}
>+
>+.vertical #boundingbox-top {
>+  left: 68px;
>+}
>+
>+.vertical #boundingbox-left {
>+  left: 20px;
>+}
>+
>+.horizontal #boundingbox-width {
>+  bottom: 28px;
>+}
>+
>+.horizontal #boundingbox-top {
>+  top: 34px;
>+}
>+
>+.horizontal #boundingbox-left {
>+  top: 68px;
>+}
[etc.]

Is this application logic or theme specific stuff?

Comment 18

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P2

Updated

6 years ago
Attachment #568706 - Flags: review?(dao)
This is likely going to need an update.
(Assignee)

Comment 20

6 years ago
Created attachment 589869 [details] [diff] [review]
patch v1.3 - rebased
(Assignee)

Updated

6 years ago
Attachment #568706 - Attachment is obsolete: true
Attachment #568706 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Depends on: 707809
(Assignee)

Comment 21

6 years ago
Created attachment 590681 [details]
mockup
(Assignee)

Updated

5 years ago
No longer depends on: 707809
(Assignee)

Comment 22

5 years ago
Created attachment 601237 [details] [diff] [review]
patch 2

Todo:
- tests
- accesskey / tooltiptext
(Assignee)

Updated

5 years ago
Depends on: 717922
(Assignee)

Updated

5 years ago
Attachment #558439 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #557568 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #589869 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #601237 - Flags: feedback?(mratcliffe)
Comment on attachment 601237 [details] [diff] [review]
patch 2

I really like it, f+ with the external image addressed.

> background: url(http://i.imgur.com/Lyp1h.png), -moz-radial-gradient(50% 70%, circle cover, hsl(210,53%,45%) 0%, hsl(210,54%,33%) 100%);
We should probably keep external images to a minimum.

> this.iframe.setAttribute("style", "width: 240px; height: 155px");
Really, why not use a class? Or is this to keep our CSS reviewable within our module?
Attachment #601237 - Flags: feedback?(mratcliffe) → feedback+
(Assignee)

Comment 24

5 years ago
Created attachment 601956 [details] [diff] [review]
patch v2.1
(Assignee)

Updated

5 years ago
Attachment #601237 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #601956 - Flags: review?(jwalker)
Attachment #601956 - Flags: feedback?(fayearthur)
(Assignee)

Comment 25

5 years ago
Created attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)
(Assignee)

Updated

5 years ago
Attachment #601956 - Attachment is obsolete: true
Attachment #601956 - Flags: review?(jwalker)
Attachment #601956 - Flags: feedback?(fayearthur)
(Assignee)

Updated

5 years ago
Attachment #602360 - Flags: review?(jwalker)
Attachment #602360 - Flags: feedback?(fayearthur)
(Assignee)

Updated

5 years ago
Blocks: 718250
(Assignee)

Comment 26

5 years ago
review/feedback ping
Comment on attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)

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

::: browser/devtools/layoutview/LayoutView.jsm
@@ +67,5 @@
> +    this.highlighter.addListener("nodeselected", this.update);
> +    this.browser.addEventListener("MozAfterPaint", this.update, true);
> +
> +    this.map = {
> +      marginTop: [".margin.top > span", "margin-top"],

Please could you document the purpose of this map.
Also, wouldn't this be easier to understand?:

    marginTop: { selector: ".margin.top > span", property: "margin-top", value: undefined },
    ...

@@ +133,5 @@
> +
> +    popupset.appendChild(this.panel);
> +  },
> +
> +  onDocumentReady: function() {

Nit: onDocumentReady: function LV_onDocumentReady() {
and for the following 2 functions.

@@ +170,5 @@
> +    let node = this.highlighter.node;
> +    let clientRect = node.getBoundingClientRect();
> +
> +    let width = Math.round(clientRect.width);
> +    let height = Math.round(clientRect.height);

Hmm, if we've got fractional width/height, wouldn't we want to see it?

::: browser/devtools/shared/LayoutHelpers.jsm
@@ -212,5 @@
>  
>      return aRectScaled;
>    },
>  
> -

Did you mean to add this? The file is otherwise unchanged?
Attachment #602360 - Flags: review?(jwalker) → review+
Comment on attachment 602360 [details] [diff] [review]
patch v2.1 (rebased)

This looks really nice. One thing I found in practice is that it's hard to grok the values quickly. Maybe it's the font size or the background. One element I selected had a margin of 6 and it took a bit to see that it wasn't a 0.

Might be something worth iterating on later, but everything is correct, and looks nice.
Attachment #602360 - Flags: feedback?(fayearthur) → feedback+
(Assignee)

Comment 29

5 years ago
Created attachment 604908 [details] [diff] [review]
patch v2.2
(Assignee)

Updated

5 years ago
Attachment #602360 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Comment on attachment 604908 [details] [diff] [review]
patch v2.2

Tim, can you review this? We add a button to browser.xul, so we need browser review.
Attachment #604908 - Flags: review?(ttaubert)
(Assignee)

Comment 31

5 years ago
Thanks for the r+ Joe! I addressed your comments in attachment 604908 [details] [diff] [review].

(In reply to Joe Walker from comment #27)
> @@ +170,5 @@
> > +    let node = this.highlighter.node;
> > +    let clientRect = node.getBoundingClientRect();
> > +
> > +    let width = Math.round(clientRect.width);
> > +    let height = Math.round(clientRect.height);
> 
> Hmm, if we've got fractional width/height, wouldn't we want to see it?

I don't know about this yet. If needed, I'll fix that in a follow-up bug.
(In reply to Paul Rouget [:paul] from comment #30)
> Comment on attachment 604908 [details] [diff] [review]
> patch v2.2
> 
> Tim, can you review this? We add a button to browser.xul, so we need browser
> review.

What's the keyboard access story for this button?
(Assignee)

Comment 33

5 years ago
(In reply to Dão Gottwald [:dao] from comment #32)
> (In reply to Paul Rouget [:paul] from comment #30)
> > Comment on attachment 604908 [details] [diff] [review]
> > patch v2.2
> > 
> > Tim, can you review this? We add a button to browser.xul, so we need browser
> > review.
> 
> What's the keyboard access story for this button?

There's no accesskey for this button. There's no text to show the accesskey and we can't build an accurate tooltip dynamically. See bug 717923 comment 24. Let me know if you think this is a problem, because this also affects bug 717922.
Comment on attachment 604908 [details] [diff] [review]
patch v2.2

Yes, this is a problem. If not a shortcut key, making the button tabbable is the minimum to satisfy our accessibility principles.
Attachment #604908 - Flags: review?(ttaubert) → review-
(In reply to Dão Gottwald [:dao] from comment #34)
> Yes, this is a problem. If not a shortcut key, making the button tabbable is
> the minimum to satisfy our accessibility principles.

However, please think hard whether you really want to meet the minimum bar only in this case. For frequently used actions, you really want shortcut or accelerator keys.
(Assignee)

Comment 36

5 years ago
(In reply to Dão Gottwald [:dao] from comment #35)
> (In reply to Dão Gottwald [:dao] from comment #34)
> > Yes, this is a problem. If not a shortcut key, making the button tabbable is
> > the minimum to satisfy our accessibility principles.
> 
> However, please think hard whether you really want to meet the minimum bar
> only in this case. For frequently used actions, you really want shortcut or
> accelerator keys.

You're right. We need to come with an efficient way to toggle the Layout View from the keyboard as it's an important feature.
(Assignee)

Comment 37

5 years ago
triage, filter on centaur
Whiteboard: [minotaur] → [minotaur][firefox14-wanted]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 718250
(Assignee)

Updated

5 years ago
Assignee: paul → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Comment 39

5 years ago
We want the Layout View to be part of the Sidebar. So this invalidate this current work.
Depends on: 707809
No longer depends on: 717922
(Assignee)

Comment 40

5 years ago
Created attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar
(Assignee)

Updated

5 years ago
Attachment #604908 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
Todo (follow-ups):
* what to do for margin:auto?
* we need proper icons
* we need a keyboard shortcut
* if the view has the focus when the sidebar is closed, the inspector keyboard shortcuts won't work (the content needs to get the focus back)
(Assignee)

Comment 42

5 years ago
Comment on attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar

Dave, the CSS code has already been reviewed previously by Joe. Focus on the JSM and on the way I register/destroy the layoutview.

Also, I don't touch any browser/ related code.

Thanks.
Attachment #615886 - Flags: review?(dcamp)

Comment 43

5 years ago
Comment on attachment 615886 [details] [diff] [review]
patch v3 - layout view in sidebar

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

JS looks fine to me, a few comments below.

Please file a followup for preffing on, and make sure it gets a ui-r+ before turning it on.

::: browser/devtools/layoutview/LayoutView.jsm
@@ +66,5 @@
> +
> +    // Is the layout view was open before?
> +    // If there's nothing in the store, then it's the first time
> +    // it is open, we fallback to the preference.
> +    if (this.inspector._getStoreValue("layoutview-is-open") == null) {

Sorry, I just broke this - _getStoreValue is removed.  Just add a property to this.inspector.

@@ +79,5 @@
> +      this.undim();
> +      this.update();
> +      // We make sure we never add 2 listeners.
> +      if (!this.trackingPaint) {
> +        this.browser.addEventListener("MozAfterPaint", this.update, true);

Do you think the inspector object should do something to help here?  Some sort of "element display may have changed, please refresh yourself" signal rather than digging through chomeWindow->gBrowser->MozAfterPaint?

Just a thought...

@@ +99,5 @@
> +    // Build the layout view in the sidebar.
> +    this.buildView();
> +
> +    // Get messages from the iframe.
> +    this.inspector.chromeWindow.addEventListener("message", this.onMessage, true);

Is there a reason you're preferring using message handling here?  Chrome docs can talk to other chrome docs, so you could just provide a reference to your message handling function directly to the frame.

Just curious, I'm not opposed to it.
Attachment #615886 - Flags: review?(dcamp) → review+

Comment 44

5 years ago
(In reply to Dave Camp (:dcamp) from comment #43)

> Please file a followup for preffing on, and make sure it gets a ui-r+ before
> turning it on.

Make sure that bug blocks on a keybinding too.
(Assignee)

Comment 45

5 years ago
Created attachment 616782 [details] [diff] [review]
patch v3.1

Addressed Dave's comments.
(Assignee)

Updated

5 years ago
Attachment #615886 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
Bug 747218 - [layout view] Pref on the layout view
(Assignee)

Comment 47

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ccead923d5b6

Comment 48

5 years ago
https://tbpl.mozilla.org/?tree=Fx-Team&rev=dde2fce2058a
Whiteboard: [minotaur][firefox14-wanted] → [minotaur][firefox14-wanted][fixed-in-fx-team]

Comment 49

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/dde2fce2058a
https://hg.mozilla.org/mozilla-central/rev/dde2fce2058a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][firefox14-wanted][fixed-in-fx-team] → [minotaur][firefox14-wanted]
Target Milestone: --- → Firefox 14

Updated

5 years ago
Depends on: 747919
(Assignee)

Updated

5 years ago
Duplicate of this bug: 674887

Updated

5 years ago
status-firefox14: --- → disabled
status-firefox15: --- → disabled
Keywords: dev-doc-needed

Comment 52

5 years ago
this has now been enabled for Firefox 15 (I can't change the status-firefox15 bit, but I'm sure akeybl can!)

Updated

5 years ago
status-firefox15: disabled → fixed
No longer blocks: 718250
Documented: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Box_model_view, so marking as dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.