Refactor styles in memory tool

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Developer Tools: Memory
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jsantell, Assigned: vporof)

Tracking

(Blocks: 1 bug)

43 Branch
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This'll be a good way to distribute workload on the memory tool, and someone faster than me can do some styles. It'd be rather easy, but in this case we're using xhtml instead of xul (so one step closer to HTML, but xhtml lets us transition easier), but we lose a lot of out-of-the-box styles we already have for XUL environments, so this could be an opportunity to start creating reusable widgets for non-XUL.

Many things in devtools/client/themes/memory.css will be tagged with this bug number to be cleaned up or generalized.

Comment 1

2 years ago
See bug 1213137 comment 18 for more clean up comments.
More TODOs that I'm aware of:

* Entire panel scrolls (create more snapshots so it flows over), just snapshot area should scroll
* All the .heap-view-panel state messages need styling love -- they just describe the current state. Probably could use a loader.
* Maybe the default state should hide the snapshot bar, otherwise the default message will be off center
* If bug 960776 lands, the tree needs badly styled, but will be handled in follow up bug 1217239
* Dropdown breakdown menu maybe could look like our other dropdowns (console?)
* we'll be adding a checkbox with some text, or maybe we can just use the recording button from performance, so that may need something, in bug 1214066

Snapshot Item:
* Export (save?) button on the snapshot list item, like performance (functionality in bug 1215954)
* We'll have total bytes/count for each snapshot, maybe we can display that in a nice way? Like IE's tools: https://i-msdn.sec.s-msft.com/dynimg/IC769141.png

Buttons:
* "Take Snapshot" button in default view needs styled
* Clear button on the snapshot list item, like performance (functionality in bug 1215955)
* Import button on the main toolbar (probably?), like performance (functionality in bug 1215953)
Now used to refactor and clean the memory.css file, rather than small style changes.
No longer blocks: 1195323
Summary: Clean up styles in memory tool → Refactor styles in memory tool
(Assignee)

Updated

2 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1219548
(Assignee)

Comment 5

2 years ago
Created attachment 8680712 [details] [diff] [review]
p1
Attachment #8680712 - Flags: review?(jsantell)
(Assignee)

Comment 6

2 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #5)
> Created attachment 8680712 [details] [diff] [review]
> p1

This fixes bug 1219548 and bug 1219074, plus more crazy things.
(Reporter)

Updated

2 years ago
Blocks: 1195323
(Assignee)

Comment 7

2 years ago
Created attachment 8680742 [details] [diff] [review]
fix-selection-issue.patch
Attachment #8680742 - Flags: review?(jsantell)
(Reporter)

Updated

2 years ago
Attachment #8680742 - Flags: review?(jsantell) → review+
(Reporter)

Updated

2 years ago
Attachment #8680712 - Flags: review?(jsantell) → review+
(Assignee)

Comment 8

2 years ago
plz lnd
Once trees are up ;_;

Comment 10

2 years ago
Comment on attachment 8680712 [details] [diff] [review]
p1

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

::: devtools/client/themes/memory.css
@@ +53,4 @@
>  .devtools-toolbar .devtools-button.take-snapshot {
> +  -moz-appearance: none;
> +  -moz-margin-start: 1px;
> +  -moz-margin-end: 1px;

nit: margin-inline-start and margin-inline-end

@@ +63,3 @@
>    background-size: 64px 16px;
>    background-position: 0 center;
>    background-repeat: no-repeat;

nit: you can remove the width, height and background-repeat properties, they are already defined in toolbars.inc.css

@@ +84,3 @@
>  .list {
> +  width: var(--sidebar-width);
> +  overflow-y: scroll;

nit: overflow-y: auto;
otherwise, the light theme on Windows will always have a scrollbar, even though there is no content to scroll. `auto` does some magic to show the scrollbar when needed.

@@ +89,1 @@
>    background-color: var(--theme-toolbar-background);

Can we use var(--theme-sidebar-background) ? After all, that variable was intended for use in sidebars.

@@ +89,2 @@
>    background-color: var(--theme-toolbar-background);
> +  -moz-border-end: 1px solid var(--theme-splitter-color);

nit: border-inline-end

@@ +228,5 @@
>  .heap-tree-item-total-bytes,
>  .heap-tree-item-total-count {
> +  text-align: end;
> +  -moz-border-end: var(--cell-border-color) 1px solid;
> +  -moz-padding-end: 5px;

nit: border-inline-end and padding-inline-end.

@@ +256,5 @@
>    background-position: -24px -24px;
> +  background-repeat: no-repeat;
> +  margin: 0px;
> +  margin-top: 2px;
> +  -moz-margin-end: 5px;

nit: margin-inline-end
(Assignee)

Comment 11

2 years ago
Thanks for the comments, agree on all of them!
In the process of uplifting, any changes have to be filed in another bug (I did add ntim's comments in before starting, however)

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e458fba06eb1
https://hg.mozilla.org/mozilla-central/rev/e458fba06eb1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=cb029edb917f

Updated

2 years ago
Blocks: 1220368
(Assignee)

Comment 16

2 years ago
Created attachment 8681612 [details] [diff] [review]
p2

Clean up everything
Attachment #8680712 - Attachment is obsolete: true
Attachment #8680742 - Attachment is obsolete: true
Attachment #8681612 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

2 years ago
Comment on attachment 8681612 [details] [diff] [review]
p2

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

::: devtools/client/themes/memory.css
@@ +73,4 @@
>  }
>  
> +.devtools-toolbar > .devtools-button.take-snapshot {
> +  -moz-appearance: none;

You can get rid of this rule (HTML buttons don't need -moz-appearance: none, the styles in toolbars.inc.css are enough).
(Assignee)

Comment 18

2 years ago
(In reply to Tim Nguyen [:ntim] from comment #17)
> Comment on attachment 8681612 [details] [diff] [review]
> p2
> 
> Review of attachment 8681612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/memory.css
> @@ +73,4 @@
> >  }
> >  
> > +.devtools-toolbar > .devtools-button.take-snapshot {
> > +  -moz-appearance: none;
> 
> You can get rid of this rule (HTML buttons don't need -moz-appearance: none,
> the styles in toolbars.inc.css are enough).

Thanks Tim!
(Assignee)

Comment 19

2 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #16)

There are many bugs filed for the memory tool about css. Most of them were fixed by the first patch attached in this bug, but then a bad rebase and a few improper changes were landed. The second patch in this bug addresses all of them.
(Assignee)

Updated

2 years ago
Attachment #8680712 - Attachment is obsolete: false
(Assignee)

Updated

2 years ago
Attachment #8681612 - Flags: review?(bgrinstead) → review?(pbrosset)
Comment on attachment 8681612 [details] [diff] [review]
p2

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

Looks good. Tested locally as discussed on IRC.
Attachment #8681612 - Flags: review?(pbrosset) → review+

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/74156ecb4a20
Duplicate of this bug: 1219802
(Assignee)

Comment 23

2 years ago
Nick, This should land on aurora ASAP. I'm currently cloning it, but it seems to go on forever. If you have an up to date aurora handy, can you please land this?
Flags: needinfo?(nfitzgerald)
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=2021ea747bec
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 25

2 years ago
Thanks a bunch!
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74156ecb4a20
Victor, Nick, could you please nominate this patch for uplift to Aurora? While this will miss the first Aurora44 build, I would like us to fix this soon.
Flags: needinfo?(vporof)
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 28

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #27)
> Victor, Nick, could you please nominate this patch for uplift to Aurora?
> While this will miss the first Aurora44 build, I would like us to fix this
> soon.

comment 24
Flags: needinfo?(vporof)
Flags: needinfo?(nfitzgerald)

Updated

2 years ago
status-firefox44: --- → fixed

Comment 29

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e458fba06eb1
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/74156ecb4a20
status-b2g-v2.5: --- → fixed

Comment 30

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/49d39d4df89b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2021ea747bec
status-b2g-v2.5: fixed → ---
Reproduced the bug with Firefox Nightly 44.0a1 (2015-10-08);(Build ID: 20151008030232) on Linux, 64 Bit

This Bug is now verified as fixed on Latest Firefox Nightly 45.0a1 (2015-12-10);(Build ID: 20151210030212)

And also Latest Firefox Dev Edition 44.0a2 (2015-12-10); (Build ID: 20151210004006)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
QA Whiteboard: [bugday-20151209]
You need to log in before you can comment on or make changes to this bug.