Closed Bug 1201700 Opened 6 years ago Closed 6 years ago

Create memory tool shell

Categories

(DevTools :: Memory, defect)

41 Branch
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Comment on attachment 8656835 [details] [diff] [review]
1201700-memory-tool.patch

r? bgrins for the toolbox options change
Attachment #8656835 - Flags: review?(bgrinstead)
Comment on attachment 8656835 [details] [diff] [review]
1201700-memory-tool.patch

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

::: browser/devtools/definitions.js
@@ +375,5 @@
>  };
>  
> +Tools.memory = {
> +  id: "memory",
> +  ordinal: 13,

It makes more sense to put it next to the Performance tab to me. Those 2 tabs are related anyway.
Comment on attachment 8656835 [details] [diff] [review]
1201700-memory-tool.patch

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

::: browser/devtools/framework/toolbox-options.js
@@ +229,5 @@
>      };
>  
>      // Populating the default tools lists
>      let toggleableTools = gDevTools.getDefaultTools().filter(tool => {
> +      return tool.visibilityswitch && tool.displayInOptions !== false;

I'd actually prefer a name like `hideInOptions` or `hidden` and then this condition could be `hideInOptions === true`, so that it doesn't add any confusion that addon devs might need to set this (since the current name implies you would want it to be true).

A comment for this new property should also be added in gDevTools.jsm above the registerTool function.
Attachment #8656835 - Flags: review?(bgrinstead)
Made toolbox definition changes and moved memory tool adjacent to perf tools
Attachment #8656835 - Attachment is obsolete: true
Attachment #8656835 - Flags: review?(nfitzgerald)
Attachment #8656887 - Flags: review?(nfitzgerald)
Attachment #8656887 - Flags: review?(bgrinstead)
Comment on attachment 8656887 [details] [diff] [review]
1201700-memory-tool.patch

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

::: browser/devtools/definitions.js
@@ +287,5 @@
> +  tooltip: "Memory (keyboardshortcut)",
> +  hiddenInOptions: true,
> +
> +  isTargetSupported: function (target) {
> +    // TODO Once Fx44 lands, we should add a root trait `memoryTool`

No TODOs without a bug number

::: browser/devtools/memory/memory.xul
@@ +8,5 @@
> +<?xml-stylesheet href="chrome://browser/skin/devtools/widgets.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/memory.css" type="text/css"?>
> +<!DOCTYPE window [
> +  <!ENTITY % debuggerDTD SYSTEM "chrome://browser/locale/devtools/performance.dtd">
> +  %debuggerDTD;

Gah no more xul... [x]html pls
Attachment #8656887 - Flags: review?(nfitzgerald) → review+
Attachment #8656887 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/6c0281a6aee3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.