Closed Bug 1174821 Opened 5 years ago Closed 5 years ago

Box-model view code clean-up

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

Some cleanup needed in both the css and js part of this panel, as a prerequisite for all bugs blocking bug 1150496.
Blocks: 1150496
Assignee: nobody → pbrosset
Bug 1174821 - 1 - eslint cleanup of view.js
Attachment #8622600 - Flags: review?(janx)
Bug 1174821 - 2 - Show only 1 box-model region on hover
Attachment #8622601 - Flags: review?(janx)
Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files

The layout-view used 2 css files, one in /browser/devtools/layoutview
and the other in /browser/themes.
This makes it unnecessarily confusing for people looking for the right
place to make css changes.
This change re-unit all css rules in one file under /browser/themes
and cleans up the code a bit (better structure, comments, simplifications).
Attachment #8622602 - Flags: review?(janx)
Bug 1174821 - 4 - Refactor layoutview's dimming
Attachment #8622603 - Flags: review?(janx)
Bug 1174821 - 5 - More eslint cleanup of view.js
Attachment #8622604 - Flags: review?(janx)
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #2)
> Created attachment 8622601 [details]
> MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover
> 
> Bug 1174821 - 2 - Show only 1 box-model region on hover
For info, part 2 is about changing the way the box-model of the current node gets highlighted in the page when hovering over the 4 different regions in the layoutview UI. We used to always highlight all 4 regions in the page but just move the guides around the hovered region.
We already discussed in the past about changing that to only highlight the corresponding region, and that's what Chrome does too, so I took the opportunity of this bug to do this.
Blocks: 1150499
Looks like I caught a permafailing test when I pushed to try yesterday:
> browser/devtools/debugger/test/browser_dbg_WorkerActor.attachThread.js | A promise chain failed to handle a rejection: at chrome://mochitests/content/browser/browser/devtools/debugger/test/head.js:1180 - TypeError: sources is undefined
This seems unrelated.
Attachment #8622600 - Flags: review?(janx) → review+
Comment on attachment 8622601 [details]
MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

I'm not sure I like this change. I wasn't part of that discussion, but where Chrome highlights just the border of an element separately from the other regions, the new behavior after this patch is to highlight content + padding + border in the same color (all contained regions get highlighted).

If we're going to highlight these regions separately, let's at least match Chrome's behavior in highlighting precisely (without the contained regions).
Attachment #8622601 - Flags: review?(janx) → review-
(In reply to Jan Keromnes [:janx] from comment #9)
> Comment on attachment 8622601 [details]
> MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover
> 
> I'm not sure I like this change. I wasn't part of that discussion, but where
> Chrome highlights just the border of an element separately from the other
> regions, the new behavior after this patch is to highlight content + padding
> + border in the same color (all contained regions get highlighted).
> 
> If we're going to highlight these regions separately, let's at least match
> Chrome's behavior in highlighting precisely (without the contained regions).
Now that you mention it, we indeed wanted to do this too. However this requires some changes to the highlighter (right now it cannot do this).
Comment on attachment 8622602 [details]
MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

> --- a/browser/themes/shared/devtools/layoutview.css
> +++ b/browser/themes/shared/devtools/layoutview.css
> @@ -6,11 +6,62 @@
>    box-sizing: border-box;
>  }
>  
> +body {
> +  /* The view will grow bigger as the window gets resized, until 400px */
> +  max-width: 400px;

Nit: 400px is very large for the box model view, don't you think? I personally preferred 320px, but I'll let you make this call.

> +#margins {
> +  /* This opacity applies to all of the regions, since they are nested. */

Super-nit: This is the only comment with a period at the end. Please consistently add or omit them.

> +/* Coordinates should be different when the window is small, because we make
> +   the regions smaller then */
> +
> +
> +@media (max-height: 228px) {

Super-nit: Please remove the extra empty line.
Attachment #8622602 - Flags: review?(janx) → review+
Comment on attachment 8622603 [details]
MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

> +  isViewVisibleAndValidNode: function() {

That function name is quite the tongue twister! But maybe shorter names like "isValid" or "hasValidNode" would have been confusing or misleading.

> +    if (isActive) {
> +      this.doc.body.classList.remove("inactive");
> +      this.trackReflows();
> +    } else {
> +      this.doc.body.classList.add("inactive");
> +      this.untrackReflows();
> +    }

Nit: You can factor out the classList lines by using toggle("inactive", !isActive).
Attachment #8622603 - Flags: review?(janx) → review+
Comment on attachment 8622604 [details]
MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

> -const SHARED_L10N = new ViewHelpers.L10N("chrome://browser/locale/devtools/shared.properties");
> +const BUNDLE = "chrome://browser/locale/devtools/shared.properties";
> +const SHARED_L10N = new ViewHelpers.L10N(BUNDLE);

Nit: The usual name for BUNDLE seems to be STRINGS_URI (or something with "STRINGS" in it).
Attachment #8622604 - Flags: review?(janx) → review+
Attachment #8622600 - Attachment description: MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js → MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx
Attachment #8622600 - Flags: review+ → review?(janx)
Comment on attachment 8622600 [details]
MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

Bug 1174821 - 1 - eslint cleanup of view.js; r=janx
Comment on attachment 8622601 [details]
MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx

Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx
Attachment #8622601 - Attachment description: MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover → MozReview Request: Bug 1174821 - 2 - Show only 1 box-model region on hover; r=janx
Attachment #8622601 - Flags: review- → review?(janx)
Comment on attachment 8622602 [details]
MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx

The layout-view used 2 css files, one in /browser/devtools/layoutview
and the other in /browser/themes.
This makes it unnecessarily confusing for people looking for the right
place to make css changes.
This change re-unit all css rules in one file under /browser/themes
and cleans up the code a bit (better structure, comments, simplifications).
Attachment #8622602 - Attachment description: MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files → MozReview Request: Bug 1174821 - 3 - Consolidate and cleanup layoutview's css files; r=janx
Attachment #8622602 - Flags: review+ → review?(janx)
Attachment #8622603 - Attachment description: MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming → MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx
Attachment #8622603 - Flags: review+ → review?(janx)
Comment on attachment 8622603 [details]
MozReview Request: Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx

Bug 1174821 - 4 - Refactor layoutview's dimming; r=janx
Comment on attachment 8622604 [details]
MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx

Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx
Attachment #8622604 - Attachment description: MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js → MozReview Request: Bug 1174821 - 5 - More eslint cleanup of view.js; r=janx
Attachment #8622604 - Flags: review+ → review?(janx)
Comment on attachment 8622600 [details]
MozReview Request: Bug 1174821 - 1 - eslint cleanup of view.js; r=janx

I don't know why mozreview keeps asking for reviews on patches that have already been R+'d, and I don't know if there's a way for me to avoid this.
Attachment #8622600 - Flags: review?(janx) → review+
Attachment #8622602 - Flags: review?(janx) → review+
Attachment #8622603 - Flags: review?(janx) → review+
Attachment #8622604 - Flags: review?(janx) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #19)
> I don't know why mozreview keeps asking for reviews on patches that have
> already been R+'d, and I don't know if there's a way for me to avoid this.

I think mozreview doesn't care about actual attachment flags. Last time I used it, it didn't r+ the patches when I used "Ship it!", and here I didn't use "Ship it!" so that's probably why it failed to realized that the patches were approved.
Attachment #8622601 - Flags: review?(janx) → review+
Could you please rebase your patches and re-submit to try, just to be sure? Other try pushes don't seem to have these permafail oranges.
(In reply to Jan Keromnes [:janx] from comment #20)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #19)
> > I don't know why mozreview keeps asking for reviews on patches that have
> > already been R+'d, and I don't know if there's a way for me to avoid this.
> 
> I think mozreview doesn't care about actual attachment flags. Last time I
> used it, it didn't r+ the patches when I used "Ship it!", and here I didn't
> use "Ship it!" so that's probably why it failed to realized that the patches
> were approved.

MozReview's behavior has recently changed: it used to only make one BZ attachment for the entire parent review, and you had to "ship it!" on the root review request to see it in BZ.

We now live in a more logical time, where MozReview makes attachments per commit.  When reviewing, you can "ship it!" per commit, and it should appear in BZ for just that commit.

If you upload a future revision, you may need to "re-ship it!" modified commits yourself.  I think you can do that before publishing the updated review draft.

You can always file MozReview bugs to improve it as well.  It is actively improved and worked on.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22) 
> You can always file MozReview bugs to improve it as well.  It is actively
> improved and worked on.
Yeah, that's what I did. Thanks for the clarification Ryan.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.