Closed
Bug 1213100
Opened 9 years ago
Closed 9 years ago
Refactor styles in memory tool
Categories
(DevTools :: Shared Components, defect)
Tracking
(firefox44 fixed, firefox45 fixed)
RESOLVED
FIXED
Firefox 45
People
(Reporter: jsantell, Assigned: vporof)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
9.70 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
13.21 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
See bug 1213137 comment 18 for more clean up comments.
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
Now used to refactor and clean the memory.css file, rather than small style changes.
No longer blocks: memory-tools-fx44
Summary: Clean up styles in memory tool → Refactor styles in memory tool
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8680712 -
Flags: review?(jsantell)
Assignee | ||
Comment 6•9 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•9 years ago
|
Blocks: memory-tools-fx44
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8680742 -
Flags: review?(jsantell)
Reporter | ||
Updated•9 years ago
|
Attachment #8680742 -
Flags: review?(jsantell) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8680712 -
Flags: review?(jsantell) → review+
Assignee | ||
Comment 8•9 years ago
|
||
plz lnd
Reporter | ||
Comment 9•9 years ago
|
||
Once trees are up ;_;
Comment 10•9 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•9 years ago
|
||
Thanks for the comments, agree on all of them!
Reporter | ||
Comment 12•9 years ago
|
||
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 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e458fba06eb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=cb029edb917f
Updated•9 years ago
|
Blocks: dt-theme-cleanup
Assignee | ||
Comment 16•9 years ago
|
||
Clean up everything
Attachment #8680712 -
Attachment is obsolete: true
Attachment #8680742 -
Attachment is obsolete: true
Attachment #8681612 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•9 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•9 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•9 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•9 years ago
|
Attachment #8680712 -
Attachment is obsolete: false
Assignee | ||
Updated•9 years ago
|
Attachment #8681612 -
Flags: review?(bgrinstead) → review?(pbrosset)
Comment 20•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 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)
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=2021ea747bec
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 25•9 years ago
|
||
Thanks a bunch!
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 26•9 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•9 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)
Updated•9 years ago
|
Flags: needinfo?(nfitzgerald)
Updated•9 years ago
|
status-firefox44:
--- → fixed
Comment 29•9 years ago
|
||
bugherder uplift |
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•9 years ago
|
||
bugherder uplift |
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 → ---
Comment 31•9 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Component: Memory → CSS and Themes
Updated•2 years ago
|
Component: CSS and Themes → Shared Components
You need to log in
before you can comment on or make changes to this bug.
Description
•