970517 - Front end for Storage Inspector.

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

Attachment: WIP 1

This is the first iteration of the front end for Storage Inspector.

This patch adds a TableWidget which is used to display the storage objects.

Things To Do
 - Add comments.
 - Handle live update of existing rows. (with a subtle flash ? )
 - Save things like width of left pane, or some columns (maybe ? )
 - At least have the either a context menu on each row, or a sidebar to display the object using variables view. Please vote.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: Screenshot of WIP 1

Comment 2

[Girish Sharma [:Optimizer]]

OH and also, the most important TODO
 - Add proper strings for everything.

Comment 3

[Girish Sharma [:Optimizer]]

Comment on attachment 8373585 [details] [diff] [review]
WIP 1

(a very top level feedback for now)

Comment 4

[Joe Walker [:jwalker]]

Just tried this patch (along with the patch from bug 965872). I had to do a couple of minor rebases, but when it built I got this:

Chrome file doesn't exist: /Users/joe/projects/mozilla/devtools/obj-debug/dist/NightlyDebug.app/Contents/MacOS/browser/chrome/en-US/locale/browser/devtools/storage.dtd
[50719] WARNING: Failed to open external DTD: publicId "" systemId "chrome://browser/locale/devtools/storage.dtd" base "chrome://browser/content/devtools/storage.xul" URL "chrome://browser/locale/devtools/storage.dtd": file /Users/joe/projects/mozilla/devtools/parser/htmlparser/src/nsExpatDriver.cpp, line 696
Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: instance is undefined
Stack: DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1007
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80
Line: 1007, column: 6
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"Error occurred while creating actor 'constructor: [Exception... \"Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindow.localStorage]\"  nsresult: \"0x80040111 (NS_ERROR_NOT_AVAILABLE)\"  location: \"JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js :: getObjectForLocalOrSessionStorage/<.populateStoresForHosts :: line 497\"  data: no]Line: 497, column: 0"}
Stack: DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:655
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/commonjs/sdk/core/promise.js:43
then@resource://gre/modules/commonjs/sdk/core/promise.js:153
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:705
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80
Line: 655, column: 0

I've spent zero time trying to debug that, so it might be obvious. Noting this so I don't lose it.

Comment 5

[Joe Walker [:jwalker]]

Things I noticed from having a play around:

When cell values are 'too long' they overflow into the next cell.

Cells have max and min width which are overly restrictive. When you have some value that is long, you'd like to shrink the other columns to give you more space, but you don't have much latitude to do that. I suspect that's related to the problem above.

To further compound the issue, when you've changed the column width (or switched to another host/storage type) and then return you get some bizarre graphical glitches (will attach an image)

Comment 6

[Joe Walker [:jwalker]]

Attachment: name/value column overlap

Comment 7

[Joe Walker [:jwalker]]

An easy way to enable cookie value editing might be to open the command line on double click with the command "cookie set NAME " pre-populated. I can help with the code to do that if you'd like.

Comment 8

[Joe Walker [:jwalker]]

Also I'm getting some bizarre double row highlight issues. Click a row in one cookie, then pick another cookie and click another row, and you get a double highlight, followed by this error:

TypeError: this.cells[this.items[this.selectedRow]] is undefined: Column.prototype.selectRowAt@resource:///modules/devtools/TableWidget.jsm:325
Column.prototype.selectRow@resource:///modules/devtools/TableWidget.jsm:335
TableWidget.prototype.selectedRow@resource:///modules/devtools/TableWidget.jsm:89
TableWidget/this.boundSelectRow@resource:///modules/devtools/TableWidget.jsm:75
EventEmitter_emit@resource:///modules/devtools/shared/event-emitter.js:126
Column.prototype.onMousedown@resource:///modules/devtools/TableWidget.jsm:488

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #6)
> Created attachment 8374176 [details]
> name/value column overlap

This happens because xul. I have seen this too. Ideally, the splitter in between should prevent overlap of the left and right boxes.

This should be resolved when I reduce the long string's initial string length. Right now, it is too high (the default one) and I have never seen a value higher than that limit yet

.(In reply to Joe Walker [:jwalker] from comment #7)
> An easy way to enable cookie value editing might be to open the command line
> on double click with the command "cookie set NAME " pre-populated. I can
> help with the code to do that if you'd like.

Nice suggestion. This can compensate until the time when the tool supports editing in itself.

But, the gcli "cookie" command have access to only a limited subset from whatever the tool might show. For one, its domain matching logic is incomplete. (will raise a bug) and for second, we do not list cookies from inner iframes.

Comment 10

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #9)
> But, the gcli "cookie" command have access to only a limited subset from
> whatever the tool might show. For one, its domain matching logic is
> incomplete. (will raise a bug) and for second, we do not list cookies from
> inner iframes.

I'll do my best to fix any bugs you raise here.

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 8373585 [details] [diff] [review]
WIP 1

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

Very quick feedback - looks good to me. I'll dig into the code more deeply with r?

::: browser/devtools/shared/widgets/TableWidget.jsm
@@ +8,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource:///modules/devtools/shared/event-emitter.js");
> +let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;

I don't think you need this import any more

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Once you get to a point where try builds pass, please link to builds - very excited to see this and want to check out the work.

Comment 13

[Girish Sharma [:Optimizer]]

(In reply to Jeff Griffiths (:canuckistani) from comment #12)
> Once you get to a point where try builds pass, please link to builds - very
> excited to see this and want to check out the work.

Sure. Will do tomorrow.

[they should pass right now too, as there are no tests to fail :)]

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: WIP 2

Fixes the issues discovered by Joe. (Thanks for the feedback)

Also, right now I am trying to close the issues being faced on the actor side along with fixing some dependent bugs for cookies. Then I will pick this up in fast pane.

For builds to play with, I triggered a try : https://tbpl.mozilla.org/?tree=Try&rev=31d81efe3345

builds will be available at :
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-31d81efe3345

Comment 15

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Looks good. A couple of questions ( apologies if you've addressed these or someone else has already brought them up ): 

1. I noticed that very long field names in localstorage aren't elided to the current width of that column:

http://note.io/1g8eFBy

It's pretty common for key names to be UUID-ish so we should account for that.

2. the data doesn't update, is there some way we could get an event from localStorage when data is added / removed / altered and refresh the view?

3. When I opened the page I was on about:home, then I navigated to todomvc. Why do I still see the about:home data after navigation?

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Jeff Griffiths (:canuckistani) from comment #15)
> Looks good. A couple of questions ( apologies if you've addressed these or
> someone else has already brought them up ): 

No worries.
 
> 1. I noticed that very long field names in localstorage aren't elided to the
> current width of that column:
> 
> http://note.io/1g8eFBy
> 
> It's pretty common for key names to be UUID-ish so we should account for
> that.

Its fixed in the patch. but I triggered try before that (sorry ! :) )
 
> 2. the data doesn't update, is there some way we could get an event from
> localStorage when data is added / removed / altered and refresh the view?

It already does in the backend and there are events for everything. The frontend only respects the added/deleted events for now and adds a row or removes one live.
 
> 3. When I opened the page I was on about:home, then I navigated to todomvc.
> Why do I still see the about:home data after navigation?

I think this is because of docshell's type difference (chrome vs content). I am plannig to move to a different approach to detect page changes so lets see.

Comment 17

[sys.sgx]

A few notes:
- The "cookies", "localStorage", etc. headers should be a bit more clearly visibly to the developer; this might have to do with the UX team
- Should we move the sessionStorage section above localStorage, so we could add all the non temporary storage data at the end? (like having localStorage, then IndexedDB, etc.)

Would like to see more screenshot updates as this gets updated. :)

Comment 18

[sys.sgx]

Update:
We should also provide buttons-options to copy the data and extract them in text.
A new bug could add some popup info window to each cookie/storage data entry, where the time the cookie/entry got loaded, its size, headers, and other useful information.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: patch 0.1

This is almost feature complete except for one remaining thing - Sidebar should auto update when selected item value changes (and some small small things)

New in this:
 - Proper Strings
 - Proper addition, update and deletion of items with flash
 - A sidebar which also shows parsed (either json or key-value object or array) value of the value
 - Table columns can be hidden
 - Various other fixes

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to sys.sgx from comment #17)
> A few notes:
> - The "cookies", "localStorage", etc. headers should be a bit more clearly
> visibly to the developer; this might have to do with the UX team
> - Should we move the sessionStorage section above localStorage, so we could
> add all the non temporary storage data at the end? (like having
> localStorage, then IndexedDB, etc.)

Cookies are non temporary too !

> Would like to see more screenshot updates as this gets updated. :)

Sure !

(In reply to sys.sgx from comment #18)
> Update:
> We should also provide buttons-options to copy the data and extract them in
> text.

In my list, but maybe follow up.

> A new bug could add some popup info window to each cookie/storage data
> entry, where the time the cookie/entry got loaded, its size, headers, and
> other useful information.

Popup = ugly/bad ux. Anyways, The details sidebar has been added and I am thinking on including more details in cookies.

Comment 21

[sys.sgx]

Looks nice!

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 8379849 [details] [diff] [review]
patch 0.1

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

Looks good so far. What do you see as outstanding before you land?

::: browser/devtools/main.js
@@ +256,5 @@
> +  key: l10n("open.commandkey", storageStrings),
> +  ordinal: 8,
> +  accesskey: l10n("open.accesskey", storageStrings),
> +  modifiers: "shift",
> +  visibilityswitch: "devtools.storage.enabled",

I think we should start with the storage panel disabled because in many situations we're very short of space on the tab bar, and not everyone will need this panel.
So we should add to firefox.js:
  pref("devtools.storage.enabled", false);

::: browser/devtools/shared/widgets/TableWidget.jsm
@@ +25,5 @@
> +
> +const MAX_VISIBLE_STRING_SIZE = 100; // characters
> +
> +/**
> + * A dynamic table with various nifty features.

Tease! What nifty features?

@@ +34,5 @@
> + *        - initialColumns: map of key vs display name for initial columns of
> + *                          the table. See @setupColumns for more info.
> + *        - uniqueId: the column which will be the unique identifier of each
> + *                     entry in the table. Default: name.
> + *        - emptyText: text to display when no entries in the table to display.

This is called placeholder below. Shouldn't we call it the same thing here?

@@ +75,5 @@
> +  else if (this.emptyText) {
> +    this.setPlaceholderText(this.emptyText);
> +  }
> +
> +  this.boundSelectRow = (aEvent, aId) => {

bindSelectedRow ? Isn't this taking an action "Bind!" rather than asking a question "Bound?".

@@ +172,5 @@
> +      disabled[disabled.length - 1].removeAttribute("disabled");
> +    }
> +  },
> +
> +  setColumns: function(aColumns, aSortOn = this.sortedOn) {

I think we could do with some comments here telling us what setColumns expects. Same for push() and anything else that isn't obvious.

@@ +240,5 @@
> +    }
> +    this.items.set(aItem[this.uniqueId], aItem);
> +    this.count++;
> +    this.tbody.removeAttribute("empty");
> +    !aSuppressFlash && this.emit(EVENTS.ROW_UPDATED, aItem[this.uniqueId]);

Noooooo! Is an if statement too hard?

@@ +563,5 @@
> +    }
> +  },
> +
> +  sort: function(aItems) {
> +

Nit: blank line not needed, and while I'm here there are a few trailing spaces in this patch

::: browser/devtools/shared/widgets/widgets.css
@@ +104,5 @@
>  .variables-view-container[aligned-values] [optional-visibility] {
>    display: none;
>  }
> +
> +/* Table Widget */

Nit: The other widgets have a blank line after the 'intro' comment.

::: browser/devtools/storage/StorageUI.jsm
@@ +97,5 @@
> +});
> +
> +
> +/**
> + * StorageUI is controls and builds the UI of the Storage Inspector.

Nit: s/StorageUI is controls/StorageUI controls/

@@ +151,5 @@
> +  destroy: function() {
> +    this.front.off("stores-update", this.onUpdate);
> +  },
> +
> +  onUpdate: function({ changed, added, deleted }) {

Docs please.

::: browser/themes/shared/devtools/widgets.inc.css
@@ +999,5 @@
> +  border: none;
> +}
> +
> +.table-widget-body {
> +  overflow: auto;

Shouldn't this go in widgets.css?
Same comment goes for min-height, min-width, padding, etc below.

@@ +1044,5 @@
> +  border: none;
> +  padding: 8px 0 0 !important;
> +  color: inherit;
> +  text-align: center;
> +  font-weight: inherit !important;

There's quite a lot of !important in this file. I think we should only use it as a last resort because overuse of !important makes for messy CSS. Is there a good way to reduce the need for it?

Comment 23

[sys.sgx]

That's true, we should have normal CSS rules for the elements.

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #22)
> Comment on attachment 8379849 [details] [diff] [review]
> patch 0.1
> 
> Review of attachment 8379849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good so far. What do you see as outstanding before you land?

But ofcourse the actor needs to land first :)
Also, tests.

> ::: browser/devtools/main.js
> @@ +256,5 @@
> > +  key: l10n("open.commandkey", storageStrings),
> > +  ordinal: 8,
> > +  accesskey: l10n("open.accesskey", storageStrings),
> > +  modifiers: "shift",
> > +  visibilityswitch: "devtools.storage.enabled",
> 
> I think we should start with the storage panel disabled because in many
> situations we're very short of space on the tab bar, and not everyone will
> need this panel.
> So we should add to firefox.js:
>   pref("devtools.storage.enabled", false);

Makes sense. Although , I can see people wanting this to be enabled by default as it is a much requested tool after all :)

> ::: browser/devtools/shared/widgets/TableWidget.jsm
> @@ +25,5 @@
> > +
> > +const MAX_VISIBLE_STRING_SIZE = 100; // characters
> > +
> > +/**
> > + * A dynamic table with various nifty features.
> 
> Tease! What nifty features?

Resize-able and toggle-able columns, multi row select, sorting, pagination, keyboard navigation, should I go on ? ;)

> @@ +34,5 @@
> > + *        - initialColumns: map of key vs display name for initial columns of
> > + *                          the table. See @setupColumns for more info.
> > + *        - uniqueId: the column which will be the unique identifier of each
> > + *                     entry in the table. Default: name.
> > + *        - emptyText: text to display when no entries in the table to display.
> 
> This is called placeholder below. Shouldn't we call it the same thing here?

Sure.
 
> @@ +75,5 @@
> > +  else if (this.emptyText) {
> > +    this.setPlaceholderText(this.emptyText);
> > +  }
> > +
> > +  this.boundSelectRow = (aEvent, aId) => {
> 
> bindSelectedRow ? Isn't this taking an action "Bind!" rather than asking a
> question "Bound?".

Heh, yes, in this case its bindSelectedRow :)

> @@ +172,5 @@
> > +      disabled[disabled.length - 1].removeAttribute("disabled");
> > +    }
> > +  },
> > +
> > +  setColumns: function(aColumns, aSortOn = this.sortedOn) {
> 
> I think we could do with some comments here telling us what setColumns
> expects. Same for push() and anything else that isn't obvious.

Comments are in general missing right now. That is next in the todo list.

> @@ +240,5 @@
> > +    }
> > +    this.items.set(aItem[this.uniqueId], aItem);
> > +    this.count++;
> > +    this.tbody.removeAttribute("empty");
> > +    !aSuppressFlash && this.emit(EVENTS.ROW_UPDATED, aItem[this.uniqueId]);
> 
> Noooooo! Is an if statement too hard?

I was told that one kitten dies for each single like if block :(

> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +999,5 @@
> > +  border: none;
> > +}
> > +
> > +.table-widget-body {
> > +  overflow: auto;
> 
> Shouldn't this go in widgets.css?
> Same comment goes for min-height, min-width, padding, etc below.

You are right !

> @@ +1044,5 @@
> > +  border: none;
> > +  padding: 8px 0 0 !important;
> > +  color: inherit;
> > +  text-align: center;
> > +  font-weight: inherit !important;
> 
> There's quite a lot of !important in this file. I think we should only use
> it as a last resort because overuse of !important makes for messy CSS. Is
> there a good way to reduce the need for it?

Will do.

Comment 25

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #24)
> ...
> > > + * A dynamic table with various nifty features.
> > 
> > Tease! What nifty features?
> 
> Resize-able and toggle-able columns, multi row select, sorting, pagination,
> keyboard navigation, should I go on ? ;)

My point was to add them into the comment though...

Comment 26

[Joe Walker [:jwalker]]

[The comments in bug 965872 were about the front end. Moving replies to this bug]

(In reply to Girish Sharma [:Optimizer] from bug 965872 comment 35)
> (In reply to Joe Walker [:jwalker] from bug 965872 comment 32)
> > More thoughts from playing with the UI:
> > 
> > * We still have unresizable columns. Given that names and values have
> > lengths that vary a lot, I don't think we can get away with this (I know
> > this is an issue with the front end patch, but I'm obviously trying them out
> > together)
> 
> But they *are* resizeable . Aren't they ? (Or are you suggesting to remove
> the resizing functionality from the columns)

Not for me. The cursor goes <-> but the column widths don't change.

> > * We've got data both in the table and in the pop-up side-bar. I don't think
> > it needs to be in both places. Perhaps we should just have name|value in the
> > main table, and all the other values in the side-bar. Or we should get rid
> > of the side bar and have everything in the table. Probably UX should have
> > some input here.
> 
> The sidebar is helpful in many ways :
>  - Even if some columns are hidden, sidebar will show all properties anyways.
>  - Sidebar shows parsed value of cookies/localstorage items. If the value is
> of JSON type, or a key-separated array, or a key-value pair, it is shown as
> an object below the normal object.
>  - In future, I want to make the table rows multi-selectable (to perform
> various actions together). Then the sidebar will show all the selected items
> together.

So if the sidebar is helpful, then I think we want to simplify the main table.

However the sidebar also isn't very discoverable. I guess we could have 3 columns: name|value|> where the third column a 'click here' icon.

I'm going to needinfo? darrin for help.

Darrin - you can see the storage inspector here https://www.youtube.com/watch?v=evyaSydRqFk. The question is - how do we resolve the duplicate information in the table and in the sidebar, and how do we make the sidebar more discoverable (if we need to)

> > * We need an icon
> 
> Yes please :)

Also needinfo? darrin on this...

Comment 27

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #26)
> [The comments in bug 965872 were about the front end. Moving replies to this
> bug]
> 
> (In reply to Girish Sharma [:Optimizer] from bug 965872 comment 35)
> > (In reply to Joe Walker [:jwalker] from bug 965872 comment 32)
> > > More thoughts from playing with the UI:
> > > 
> > > * We still have unresizable columns. Given that names and values have
> > > lengths that vary a lot, I don't think we can get away with this (I know
> > > this is an issue with the front end patch, but I'm obviously trying them out
> > > together)
> > 
> > But they *are* resizeable . Aren't they ? (Or are you suggesting to remove
> > the resizing functionality from the columns)
> 
> Not for me. The cursor goes <-> but the column widths don't change.

That is weird. Will test it on OS X.

> 
> So if the sidebar is helpful, then I think we want to simplify the main
> table.
> 
> However the sidebar also isn't very discoverable. I guess we could have 3
> columns: name|value|> where the third column a 'click here' icon.

Network monitor also works this way, so I guess it will be intuitive because of experience ?

Also, sooner or later, we will need a toolbar to host buttons to clear things, get next set of items (as the storage item list is paginated) and a search box, so we can (then) also add a button to show sidebar like the network monitor's toolbar has.

Also, the same button repeating in each row would be overkill.

Comment 28

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #27)
> > So if the sidebar is helpful, then I think we want to simplify the main
> > table.
> > 
> > However the sidebar also isn't very discoverable. I guess we could have 3
> > columns: name|value|> where the third column a 'click here' icon.
> 
> Network monitor also works this way, so I guess it will be intuitive because
> of experience ?
> 
> Also, sooner or later, we will need a toolbar to host buttons to clear
> things, get next set of items (as the storage item list is paginated) and a
> search box, so we can (then) also add a button to show sidebar like the
> network monitor's toolbar has.
> 
> Also, the same button repeating in each row would be overkill.

Maybe, or maybe we could only show it on hover.

Comment 29

[sys.sgx]

Yep, the sidebar with the info should be clearly visible. Also I would like to focus on some possible additions:
- some kind of export for the data the user is viewing. (ie session ID)
- Where's the flash storage section?
- Could we add some onvaluechange events? (to work along with the debugger in the console)

Comment 30

[Tim Nguyen :ntim]

I have an icon for this. SVG like the other ones, Im gonna upload it here as soon as I can.

Comment 31

[Tim Nguyen :ntim]

Attachment: Possible storage inspector icon

Can someone try my icon ? thanks !

Comment 32

[Girish Sharma [:Optimizer]]

(In reply to Tim Nguyen [:ntim] from comment #31)
> Created attachment 8384237 [details]
> Possible storage inspector icon
> 
> Can someone try my icon ? thanks !

Thanks for the icon. Here is the screenshot : http://i.snag.gy/NylFQ.jpg

I feel two issues:
1) It is a bit smaller.
2) Does not quiet fit well with other icons.

Lets see what Darrin has to say (or add a new svg :) )

Comment 33

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Just an updated of previous patch rebased on the latest path in bug 965872. No review comments addressed for now.

Comment 34

[Girish Sharma [:Optimizer]]

I test on osx, as soon as the cursor goes -| , I am able to resize the columns using three finger swipe or click and drag .

or are you on Linux ?

Comment 35

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #34)
> I test on osx, as soon as the cursor goes -| , I am able to resize the
> columns using three finger swipe or click and drag .
> 
> or are you on Linux ?

I'm on OSX.
In the first patch I could only resize a small amount. In the second, none at all. Have you tried with the toolbox attached to the right hand side?

Comment 36

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #35)
> (In reply to Girish Sharma [:Optimizer] from comment #34)
> > I test on osx, as soon as the cursor goes -| , I am able to resize the
> > columns using three finger swipe or click and drag .
> > 
> > or are you on Linux ?
> 
> I'm on OSX.
> In the first patch I could only resize a small amount. In the second, none
> at all. Have you tried with the toolbox attached to the right hand side?

Ah.. that is probably because all the columns are already having their min-width (150px)

Comment 37

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #36)
> (In reply to Joe Walker [:jwalker] from comment #35)
> > (In reply to Girish Sharma [:Optimizer] from comment #34)
> > > I test on osx, as soon as the cursor goes -| , I am able to resize the
> > > columns using three finger swipe or click and drag .
> > > 
> > > or are you on Linux ?
> > 
> > I'm on OSX.
> > In the first patch I could only resize a small amount. In the second, none
> > at all. Have you tried with the toolbox attached to the right hand side?
> 
> Ah.. that is probably because all the columns are already having their
> min-width (150px)

I guess this was to do with preventing the value column from taking over when the values were long? Or maybe we can just remove the min-width?

Comment 38

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #37)
> (In reply to Girish Sharma [:Optimizer] from comment #36)
> > (In reply to Joe Walker [:jwalker] from comment #35)
> > > (In reply to Girish Sharma [:Optimizer] from comment #34)
> > > > I test on osx, as soon as the cursor goes -| , I am able to resize the
> > > > columns using three finger swipe or click and drag .
> > > > 
> > > > or are you on Linux ?
> > > 
> > > I'm on OSX.
> > > In the first patch I could only resize a small amount. In the second, none
> > > at all. Have you tried with the toolbox attached to the right hand side?
> > 
> > Ah.. that is probably because all the columns are already having their
> > min-width (150px)
> 
> I guess this was to do with preventing the value column from taking over
> when the values were long? Or maybe we can just remove the min-width?

This is actually just an aesthetic thing. I thought that a min width should be present. Though I have reduced the min-width to 100px in the latest patch.

If we remove the min-with, and the user has all the columns visible, then he won't be able to see anything in any column in the docked to right toolbox mode. Instead, I am planning to hide almost all the columns in that mode.

Comment 39

[Joe Walker [:jwalker]]

Please could you create a try build once the back end has landed for Darrin to play with?

Comment 40

[Darrin Henein [:darrin]]

> Darrin - you can see the storage inspector here
> https://www.youtube.com/watch?v=evyaSydRqFk. The question is - how do we
> resolve the duplicate information in the table and in the sidebar, and how
> do we make the sidebar more discoverable (if we need to)

Sorry for the late response here. The way I see it we are only duplicating the "value" or "data" column, which in itself usually only displays a preview of unparsed text. I don't see any problem having the sidebar show the full (and parsed) representation of that single column.

Could we have the sidebar open by default and empty if no rows are selected? Or auto select the first row when a source is chosen?

Comment 41

[Girish Sharma [:Optimizer]]

(In reply to Darrin Henein [:darrin] from comment #40)
> > Darrin - you can see the storage inspector here
> > https://www.youtube.com/watch?v=evyaSydRqFk. The question is - how do we
> > resolve the duplicate information in the table and in the sidebar, and how
> > do we make the sidebar more discoverable (if we need to)
> 
> Sorry for the late response here. The way I see it we are only duplicating
> the "value" or "data" column, which in itself usually only displays a
> preview of unparsed text. I don't see any problem having the sidebar show
> the full (and parsed) representation of that single column.
> 
> Could we have the sidebar open by default and empty if no rows are selected?
> Or auto select the first row when a source is chosen?

We can show the sidebar by default, but wouldn't that be against what other tools with on-demand sidebars are doing already ? (netmonitor and debugger).

Also, I am going to attach a new patch in the weekend here, as the current UI will not work well with Indexed DB. Will have builds to play with along with the patch.

Comment 42

[Darrin Henein [:darrin]]

Attachment: icon-storage.svg

Here is a modified icon in svg format for the storage inspector. Optimizer, most of our new icons are characterized as being 'flat', not necessarily line-drawings. Hopefully you're happy with this one :)

Comment 43

[Tim Nguyen :ntim]

(In reply to Darrin Henein [:darrin] from comment #42)
> Created attachment 8394292 [details]
> icon-storage.svg
> 
> Here is a modified icon in svg format for the storage inspector. Optimizer,
> most of our new icons are characterized as being 'flat', not necessarily
> line-drawings. Hopefully you're happy with this one :)

I noticed a small problem in the SVG : http://images.devs-on.net/Image/xCtBuiMD5SsT2I4W-Region.png
Also, it's quite huge, not sure if scaling down will look well.
Other than that, nice work :)

Comment 44

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #43)
> (In reply to Darrin Henein [:darrin] from comment #42)
> > Created attachment 8394292 [details]
> > icon-storage.svg
> > 
> > Here is a modified icon in svg format for the storage inspector. Optimizer,
> > most of our new icons are characterized as being 'flat', not necessarily
> > line-drawings. Hopefully you're happy with this one :)
> 
> I noticed a small problem in the SVG :
> http://images.devs-on.net/Image/xCtBuiMD5SsT2I4W-Region.png
> Also, it's quite huge, not sure if scaling down will look well.
Take back what I said, just Firefox making the SVG huge (which also causes the issue).

Comment 45

[Girish Sharma [:Optimizer]]

(In reply to Darrin Henein [:darrin] from comment #42)
> Created attachment 8394292 [details]
> icon-storage.svg
> 
> Here is a modified icon in svg format for the storage inspector. Optimizer,
> most of our new icons are characterized as being 'flat', not necessarily
> line-drawings. Hopefully you're happy with this one :)

Darrin, seems like this svg "actually" is *huge*, which makes it invisible on the tabs bar.

If I open other svg icons via link like [0], I get small size svg, while for storage, the icon is like full page width.

[0] chrome://browser/skin/devtools/tool-(inspector|styleeditor|web-console).svg

Comment 46

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Updated the front end code so that it works with indexed db. Implemented a new Tree Widget (still WIP).

builds will be available at [0] once try [1] is finished building.

Please provide feedback wrt the tree.

[0] https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d823c290a0c0
[1] https://tbpl.mozilla.org/?tree=Try&rev=d823c290a0c0

Comment 47

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #46)
> Created attachment 8395449 [details] [diff] [review]
> patch v0.3
> 
> Updated the front end code so that it works with indexed db. Implemented a
> new Tree Widget (still WIP).
> 
> builds will be available at [0] once try [1] is finished building.
> 
> Please provide feedback wrt the tree.

Nice ! I'm loving what I'm seeing :)
Here's some feedback :
- Auto expand tree if all items fit
- Make the arrow visible in light theme
- Make the table header fixed
- Make the table match styles from bug 951714
- Make key overflow correctly : http://images.devs-on.net/Image/2L6jTmU4xRJV7yFB-Region.png
- It would be nice if the header of the tree (Cookies, Local storage, Session Storage) had the debugger header styling. Feels a bit strange now for some reason. Though, I don't think we should do this if there are more than 2 levels (aka file system tree)

Comment 48

[Tim Nguyen :ntim]

Also, the info sidebar searchbox has no placeholder.

Comment 49

[Darrin Henein [:darrin]]

Attachment: icon-storage.svg (small)

Apologies Optimizer! I forgot to add the relative sizing to the svg tag. This one should match the others better.

Comment 50

[Girish Sharma [:Optimizer]]

(In reply to Darrin Henein [:darrin] from comment #49)
> Created attachment 8395668 [details]
> icon-storage.svg (small)
> 
> Apologies Optimizer! I forgot to add the relative sizing to the svg tag.
> This one should match the others better.

Cool. Thanks :)

Comment 51

[Girish Sharma [:Optimizer]]

New builds at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-226f986eeaf5

- Tree is almost finish. Have to decide API and add comments.
- Table column headers are fixed position (only half way, if you scroll up, they are at wrong place, until you resize the table somehow. This is an XUL bug, should be easy to fix though.)

Comment 52

[Girish Sharma [:Optimizer]]

Darrin, as you can see, the icons (left to the entries in the tree) do not look good on light/dark theme of the toolbox.

Can we have some well fitting icons for the followings things in a single sprite like [0] :

- File icons : CSS, JS, HTML, TXT, IMAGE, Manifest.
- Open folder icon
- World/Globe icon - to demonstrate website url like "http://google.com"

Bonus points if the icons are mono-colored, like rest of our icons :)

I know its lot of icons, no hurry :)

[0] https://raw.githubusercontent.com/campd/itchpad/master/chrome/content/[e-mail]

Comment 53

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #51)
> New builds at
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-226f986eeaf5
> 
> - Tree is almost finish. Have to decide API and add comments.
> - Table column headers are fixed position (only half way, if you scroll up,
> they are at wrong place, until you resize the table somehow. This is an XUL
> bug, should be easy to fix though.)

I've tried the build, and it has weird bugs :
- HTML string is parsed as JSON
- Empty cells are collapsed
- Table contents can be seen under the table header when scrolling
>> The table problems can be fixed by moving the table header out of the scrolling area

Comment 54

[Tim Nguyen :ntim]

- URLs are also read as JSON.

Comment 55

[Tim Nguyen :ntim]

Attachment: tree-icon-sprite.png

You might want to use these icons temporarily. I might make HDPI icons if I have some more time.

Comment 56

[Tim Nguyen :ntim]

Attachment: tree-icon-sprite.zip

The zip file also includes 2x icons. For light theme, you can invert them using SVG filters (I optimized the colors so they would look good inverted too.). Hope you like them :)

Comment 57

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #56)
> Created attachment 8399095 [details]
> tree-icon-sprite.zip
> 
> The zip file also includes 2x icons. For light theme, you can invert them
> using SVG filters (I optimized the colors so they would look good inverted
> too.). Hope you like them :)

I also optimized the PNGs in the zip file.

Comment 58

[Tim Nguyen :ntim]

Attachment: tree-icon-sprite.zip

Comment 59

[Girish Sharma [:Optimizer]]

Comment on attachment 8399096 [details]
tree-icon-sprite.zip

Hi ntim, thanks for putting together the icons. I am clearing the feedback as they also don't look good. See screenshot below. I am using the @2x versions, still they are not clear, and too dull.

screenshot : http://i.snag.gy/LvfaF.jpg

Comment 60

[Dane MacMillan [:danemacmillan]]

In reply to Comment 52:

> 
> World/Globe icon - to demonstrate website url like "http://google.com"
> 

Would using a site's favicon be up for consideration?

Comment 61

[Girish Sharma [:Optimizer]]

(In reply to Dane MacMillan from comment #60)
> In reply to Comment 52:
> 
> > 
> > World/Globe icon - to demonstrate website url like "http://google.com"
> > 
> 
> Would using a site's favicon be up for consideration?

That would be too complex. Specially for iframes, which can be from any domain and will not have a favicon. Also, even if we want to show site's favicon, lets do that in a followup.

Comment 62

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #59)
> Comment on attachment 8399096 [details]
> tree-icon-sprite.zip
> 
> Hi ntim, thanks for putting together the icons. I am clearing the feedback
> as they also don't look good. See screenshot below. I am using the @2x
> versions, still they are not clear, and too dull.
> 
> screenshot : http://i.snag.gy/LvfaF.jpg

You're not supposed to use the 2x icons for normal displays. Its obvious that 2x icons have a lower quality on normal dpi. This shouldn't be an issue for the tree API. You can check for retina in both CSS and JS.

Comment 63

[Girish Sharma [:Optimizer]]

Scaling down is never an issue. The previous icons were also 2x, but they were a bit clearer.

Comment 64

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #63)
> Scaling down is never an issue. The previous icons were also 2x, but they
> were a bit clearer.
I was thinking of a darker gray for the text (CSS/JS), but also for the globe icon gray. It'll make the icons "clearer" and will make the gray more noticeable when inverting the icons.

But I still think it's a bad idea to use 2x icons for 1x. Last time we did that, in turned out like this : bug 957291 . Meanwhile, you could try image-rendering:-moz-crisp-edges;

Comment 65

[Girish Sharma [:Optimizer]]

New builds with complete indexed db implementation : https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-edea0673b105

the sidebar is not yet updated completely to handle idb, but the table should show everything.

Comment 66

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #65)
> New builds with complete indexed db implementation :
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-edea0673b105
> 
> the sidebar is not yet updated completely to handle idb, but the table
> should show everything.

The storage inspector isn't working at all on Windows 8. It shows a blank sidebar and table

Comment 67

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

rebased on top of bug 993014.

try: https://tbpl.mozilla.org/?tree=Try&rev=5318bf03dfea

Comment 68

[Girish Sharma [:Optimizer]]

Clearing this needinfo as icons have now moved to other bug, and are pretty much done. Just pending ui-review on that bug

Comment 69

[Brian Grinstead [:bgrins]]

I had this and the table/tree patches applied and was inspecting storage on cnn.com, then left the browser alone for a while and came back to a ton of these errors:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "channel.loadGroup is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js" line: 626}]'[JavaScript Error: "channel.loadGroup is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js" line: 626}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

Not sure exactly what triggered it though.

Comment 70

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5

This is ready for review.

1 known issue : strings like "http://google.com" are split up in key-value pairs of "http" and "//google.com" by the value parser.

Writing tests now. Although, most parts of the tool are already tested now as widgets have their own complete set of tests and the actors are also tested on their own.

Comment 71

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #69)
> I had this and the table/tree patches applied and was inspecting storage on
> cnn.com, then left the browser alone for a while and came back to a ton of
> these errors:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "[JavaScript Error: "channel.loadGroup is null" {file:
> "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/storage.js" line:
> 626}]'[JavaScript Error: "channel.loadGroup is null" {file:
> "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/storage.js" line: 626}]' when
> calling method: [nsIObserver::observe]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame ::
> <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
> ************************************************************
> 
> Not sure exactly what triggered it though.

Hmm, this sure is weird that cookie was created by a network call and the channel.loadGroup is undefined. Will ask platform people.

Comment 72

[Brian Grinstead [:bgrins]]

Attachment: sidebar-close.png

A weird UI thing that bugs me here is that when I click on a row, I can't get rid of the sidebar showing the parsed value.  Escape opens split console - so an easy thing to do would be to catch it and close the sidebar instead, similar to how the console behaves.

Even that keyboard shortcut may not be obvious to a lot of people though.  Ideally there would be a button to collapse the sidebar panel, or (maybe?) clicking on the table row a second time could deselect it. Not sure how I feel about that toggle selection thing, just a thought.

Comment 73

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #72)
> Created attachment 8410336 [details]
> sidebar-close.png
> 
> A weird UI thing that bugs me here is that when I click on a row, I can't
> get rid of the sidebar showing the parsed value.  Escape opens split console
> - so an easy thing to do would be to catch it and close the sidebar instead,
> similar to how the console behaves.
> 
> Even that keyboard shortcut may not be obvious to a lot of people though. 
> Ideally there would be a button to collapse the sidebar panel, or (maybe?)
> clicking on the table row a second time could deselect it. Not sure how I
> feel about that toggle selection thing, just a thought.

Right now I am adding an "Escape" keypress, like in other tools.

Soon , there will be a toolbar for things like pagination, searchbox etc. Then I will add a button to collapse/show the sidebar, just like netmonitor.

Comment 74

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma [:Optimizer] from comment #71)
> (In reply to Brian Grinstead [:bgrins] from comment #69)
> > I had this and the table/tree patches applied and was inspecting storage on
> > cnn.com, then left the browser alone for a while and came back to a ton of
> > these errors:
> > 
> > ************************************************************
> > * Call to xpconnect wrapped JSObject produced this error:  *
> > [Exception... "[JavaScript Error: "channel.loadGroup is null" {file:
> > "resource://gre/modules/commonjs/toolkit/loader.js ->
> > resource://gre/modules/devtools/server/actors/storage.js" line:
> > 626}]'[JavaScript Error: "channel.loadGroup is null" {file:
> > "resource://gre/modules/commonjs/toolkit/loader.js ->
> > resource://gre/modules/devtools/server/actors/storage.js" line: 626}]' when
> > calling method: [nsIObserver::observe]"  nsresult: "0x80570021
> > (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame ::
> > <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
> > ************************************************************
> > 
> > Not sure exactly what triggered it though.
> 
> Hmm, this sure is weird that cookie was created by a network call and the
> channel.loadGroup is undefined. Will ask platform people.

Talked with Honza (:mayhemer) about this. He suggested to remove the dispatch and call the notification service directly in HttpBaseChannel.cpp where this noticiation emits.

The thing is that right now, its being dispatched in an async manner, so the channel might have been removed from the loadGroup by the time storage.js receives the notification.

Will file a followup for that. Right now , just checing if channel.loadGroup is non null before accessing a property out of it to prevent the exceptions.

Comment 75

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

Updated to work with the new tree widget api.

Comment 76

[Girish Sharma [:Optimizer]]

Attachment: rebased 0.6

(just rebased on top of bug 993014)

Comment 77

[Girish Sharma [:Optimizer]]

Attachment: patch v0.7

Brian noticed an issue. I fixed it.

Comment 78

[Girish Sharma [:Optimizer]]

Attachment: actual patch v0.7

Oops. This is the correct v0.7

Comment 79

[Tim Nguyen :ntim]

Comment on attachment 8417595 [details] [diff] [review]
actual patch v0.7

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

::: browser/themes/shared/devtools/storage.css
@@ +10,5 @@
> +  overflow: auto;
> +}
> +
> +.theme-dark #storage-tree {
> +  background: url(background-noise-toolbar.png), #343c45; /* Toolbars */

FYI, We're getting rid of this file in bug 1011173

Comment 80

[Joe Walker [:jwalker]]

I'm not down for a review of v0.7, should I be?

Comment 81

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Ping, this has been a few days. We get a lot of requests for this feature, would be nice to get it out the door.

Comment 82

[Girish Sharma [:Optimizer]]

Still on it. The main code is ready (I have a patch locally). Working on tests. A bit slow since last few days due to work load.

Comment 83

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Sounds good, and would love to take a recent try build for a spin. needinfo me the next time you get one.

Comment 84

[Dave Camp (:dcamp)]

Optimizer, do you have a newer patch than the one on this bug?  If we're just waiting on tests, can we get review of the main patch done now?

Comment 85

[Girish Sharma [:Optimizer]]

Attachment: patch v0.8

Okay. I am back at this now.
This is the final patch that should be good for first cut. I will file followups for the know limitations etc.

This patch is without tests, currently working on tests now. This can be reviewed in the mean time.

try push : https://tbpl.mozilla.org/?tree=Try&rev=b72e354e5865

Comment 86

[Joe Walker [:jwalker]]

I'm in mid review, and seeing this a bit in the console:

System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/storage.js:634 - NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]

Comment 87

[Joe Walker [:jwalker]]

Comment on attachment 8446105 [details] [diff] [review]
patch v0.8

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

Looks good so far.

I've not dug right into StorageUI.jsm yet.

::: browser/devtools/shared/widgets/TableWidget.js
@@ +690,1 @@
>      if (id != this.id) {

I know this isn't part of your changes, but why are we requiring callers to pass in the id of the column when they've already got the column to hide?

::: browser/devtools/storage/Makefile.in
@@ +5,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +libs::
> +	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools/
> +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/storage

If StorageUI.jsm is storageui.js then we can remove this file.
Otherwise we need build peer review.

::: browser/devtools/storage/StorageUI.jsm
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["StorageUI"];

I'm guessing that it would be fairly easy to convert this to being a JS file?

@@ +35,5 @@
> +  searchEnabled: true,
> +  searchPlaceholder: L10N.getStr("storage.search.placeholder"),
> +  preventDisableOnChange: true,
> +  preventDescriptorModifiers: true,
> +  eval: () => {}

This is allowing us to change values and then ignoring the changes - if we say eval:null then I think the variablesview won't allow changes in the first place.

@@ +131,5 @@
> +   * Event handler for "stores-update" event coming from the storage actor.
> +   *
> +   * @param {object} argument0
> +   *        An object containing the details of the added, changed and deleted
> +   *        storage objects.

Please could you document this better

::: browser/devtools/storage/moz.build
@@ +2,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Something like:

JS_MODULES_PATH = 'modules/devtools/storage'

EXTRA_JS_MODULES += [
    'panel.js',
    'storageui.js'
]

BROWSER_CHROME_MANIFESTS += ['test/browser.ini']

Comment 88

[Joe Walker [:jwalker]]

Attachment: problem1.png

There are some rendering problems for me when opening and closing tree nodes.

Comment 89

[Joe Walker [:jwalker]]

In the webconsole, the variablesview sidebar hides when escape is pressed. I think we should do the same here.

Comment 90

[Joe Walker [:jwalker]]

Attachment: problem2.png

It's looking good. Thanks.
One final comment, could we make the date columns narrower? I still can't see the whole table and there it lots of whitespace.
And are the column headers the height you'd expect? Maybe there's more padding than we need?

Comment 91

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #86)
> I'm in mid review, and seeing this a bit in the console:
> 
> System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/storage.js:634 -
> NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE)
> [nsIInterfaceRequestor.getInterface]

This is bug 1013635

Comment 92

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #87)
> Comment on attachment 8446105 [details] [diff] [review]
> patch v0.8
> 
> Review of attachment 8446105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good so far.
> 
> I've not dug right into StorageUI.jsm yet.
> 
> ::: browser/devtools/shared/widgets/TableWidget.js
> @@ +690,1 @@
> >      if (id != this.id) {
> 
> I know this isn't part of your changes, but why are we requiring callers to
> pass in the id of the column when they've already got the column to hide?

Initially this method was not supposed to be an API on the TableWidget, so it was just being used internally as an event dispatch handler. In this patch only I added so that this method can be called without arguments. But if arguments are present, then it is the event handler, and thus it needs the id check.


> ::: browser/devtools/storage/Makefile.in
> @@ +5,5 @@
> > +include $(topsrcdir)/config/rules.mk
> > +
> > +libs::
> > +	$(NSINSTALL) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools/
> > +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/storage
> 
> If StorageUI.jsm is storageui.js then we can remove this file.
> Otherwise we need build peer review.

I can easily convert to js and require it.

> ::: browser/devtools/storage/StorageUI.jsm
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +this.EXPORTED_SYMBOLS = ["StorageUI"];
> 
> I'm guessing that it would be fairly easy to convert this to being a JS file?

Yeah
 
> @@ +35,5 @@
> > +  searchEnabled: true,
> > +  searchPlaceholder: L10N.getStr("storage.search.placeholder"),
> > +  preventDisableOnChange: true,
> > +  preventDescriptorModifiers: true,
> > +  eval: () => {}
> 
> This is allowing us to change values and then ignoring the changes - if we
> say eval:null then I think the variablesview won't allow changes in the
> first place.

I see...
 
> @@ +131,5 @@
> > +   * Event handler for "stores-update" event coming from the storage actor.
> > +   *
> > +   * @param {object} argument0
> > +   *        An object containing the details of the added, changed and deleted
> > +   *        storage objects.
> 
> Please could you document this better

Will do.

> ::: browser/devtools/storage/moz.build
> @@ +2,5 @@
> > +# vim: set filetype=python:
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> 
> Something like:
> 
> JS_MODULES_PATH = 'modules/devtools/storage'
> 
> EXTRA_JS_MODULES += [
>     'panel.js',
>     'storageui.js'
> ]
> 
> BROWSER_CHROME_MANIFESTS += ['test/browser.ini']

So with this we don't need build peer review ?

(In reply to Joe Walker [:jwalker] from comment #88)
> Created attachment 8447108 [details]
> problem1.png
> 
> There are some rendering problems for me when opening and closing tree nodes.

I have seen such rendering problems in some other tools too in the past, not now but. Let me see if translateZ will fix it .

(In reply to Joe Walker [:jwalker] from comment #89)
> In the webconsole, the variablesview sidebar hides when escape is pressed. I
> think we should do the same here.

I have added such a check, but it only seems to work when the table is focused.

(In reply to Joe Walker [:jwalker] from comment #90)
> Created attachment 8447110 [details]
> problem2.png
> 
> It's looking good. Thanks.
> One final comment, could we make the date columns narrower? I still can't
> see the whole table and there it lots of whitespace.
> And are the column headers the height you'd expect? Maybe there's more
> padding than we need?

I am also seeing this. I think I made the min-width based on a Date().toString() but finally ended up using .toLocaleString() instead.

Comment 93

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #92)
>  
> > @@ +35,5 @@
> > > +  searchEnabled: true,
> > > +  searchPlaceholder: L10N.getStr("storage.search.placeholder"),
> > > +  preventDisableOnChange: true,
> > > +  preventDescriptorModifiers: true,
> > > +  eval: () => {}
> > 
> > This is allowing us to change values and then ignoring the changes - if we
> > say eval:null then I think the variablesview won't allow changes in the
> > first place.
> 
> I see...
>  

...in which case you don't need preventDisableOnChange.

Comment 94

[Victor Porof [:vporof][:vp]]

Is there any use in opening the variables view when there's only one key with one value? It that case there's 0 new information added by the variables view.

Comment 95

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #94)
> Is there any use in opening the variables view when there's only one key
> with one value? It that case there's 0 new information added by the
> variables view.

Yeah, but from user point of view, sidebar opening for some rows and not for some other might be a bit misleading.

Comment 96

[Victor Porof [:vporof][:vp]]

In that case I'd say we need to fix how rows with additional properties are displayed. But that's just a thought.

Comment 97

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #96)
> In that case I'd say we need to fix how rows with additional properties are
> displayed. But that's just a thought.

What do you mean?

FWIW, variables view is the best way to show a JSON/Array.

Comment 98

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #92)
> (In reply to Joe Walker [:jwalker] from comment #87)
> > ::: browser/devtools/storage/moz.build
> > @@ +2,5 @@
> > > +# vim: set filetype=python:
> > > +# This Source Code Form is subject to the terms of the Mozilla Public
> > > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > > +
> > 
> > Something like:
> > 
> > JS_MODULES_PATH = 'modules/devtools/storage'
> > 
> > EXTRA_JS_MODULES += [
> >     'panel.js',
> >     'storageui.js'
> > ]
> > 
> > BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
> 
> So with this we don't need build peer review ?

Maybe we do. Perhaps I should have said "Otherwise we'll get r- from build peer review."
I got an r- the last time I tried adding $(NSINSTALL)

Comment 99

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #5)

> To further compound the issue, when you've changed the column width (or
> switched to another host/storage type) and then return you get some bizarre
> graphical glitches (will attach an image)

I believe this is Bug 1021564 - it is most likely only affecting the light theme (we have the same kinds of issues currently when adding and removing command buttons)

Comment 100

[Girish Sharma [:Optimizer]]

Attachment: patch v0.9

- Fixed the min-width of date columns.
- removed Makefile.in (now the support is anyways removed I guess... )
- converted StorageUI.jsm to ui.js
- Fixed other review comments.

I am unable to see the weird extra padding in the table headers ..

Do I still need build peer review ? (see https://bugzilla.mozilla.org/show_bug.cgi?id=960671#c17 )

Comment 101

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #100)
> Do I still need build peer review ? (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=960671#c17 )

+1000 for getting rid of the Makefiles. I'm reviewing this now.

Comment 102

[Joe Walker [:jwalker]]

Comment on attachment 8468767 [details] [diff] [review]
patch v0.9

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

Very happy, except for the missing (?) ui.js...

::: browser/devtools/storage/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXTRA_JS_MODULES.devtools.storage += [
> +    'panel.js',
> +    'ui.js'

I think ui.js is missing from this patch, right?

Comment 103

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #102)
> Comment on attachment 8468767 [details] [diff] [review]
> patch v0.9
> 
> Review of attachment 8468767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very happy, except for the missing (?) ui.js...
> 
> ::: browser/devtools/storage/moz.build
> @@ +5,5 @@
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +EXTRA_JS_MODULES.devtools.storage += [
> > +    'panel.js',
> > +    'ui.js'
> 
> I think ui.js is missing from this patch, right?

Damnit. I forgot to hg add it.

I won't be able to upload a new patch for at least 3-4 hrs.

ui.j is exactly similar to last patch's StorageUI.jsm except for the more documented comment as asked in comment 87

Comment 104

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

Sorry about the last patch. This has ui.js

Comment 105

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Girish Sharma [:Optimizer] from comment #104)
> Created attachment 8469309 [details] [diff] [review]
> patch v1.0
> 
> Sorry about the last patch. This has ui.js

Do you mind triggering a try build for this? I'd love to take a look (and Joe may well be off for the day by now)

Comment 106

[Girish Sharma [:Optimizer]]

Here we go : https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=32f81452ea26

Comment 107

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Girish Sharma [:Optimizer] from comment #106)
> Here we go :
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=32f81452ea26

To be clear, the current functionality is:

* cookies, indexedDB, localstorage
* sessiontStorage is there but doesn't work for me in the UI. I see this in the logs:

EMITTING: emit(select, sessionStorage,http://localhost:3000, undefined) from TreeWidget.prototype.onClick() -> resource:///modules/devtools/shared/widgets/TreeWidget.js:328
DBG-SERVER: Packet 271 sent to "conn2.sessionStorage228"
DBG-SERVER: Received packet 271: {
  "type": "getStoreObjects",
  "host": "http://localhost:3000",
  "names": null,
  "to": "conn2.sessionStorage228"
}
DBG-SERVER: Packet 272 sent from "conn2.sessionStorage228"
DBG-SERVER: Received packet 272: {
  "offset": 0,
  "total": 9,
  "data": [
    {
      "name": "bat",
      "value": "2"
    },
    {
      "name": "foo",
      "value": "1"
    },
    {
      "name": "current-flag",
      "value": "In Flight"
    },
    {
      "name": "key",
      "value": ""
    },
    {
      "name": "getItem",
      "value": ""
    },
    {
      "name": "setItem",
      "value": ""
    },
    {
      "name": "removeItem",
      "value": ""
    },
    {
      "name": "clear",
      "value": ""
    },
    {
      "name": "length",
      "value": ""
    }
  ],
  "from": "conn2.sessionStorage228"
}

I guess we don't get read/write access in the UI until bug 1031192 is resolved. We can wait for that.

For the UI, I noticed some glitchy behaviour in the tree icons:

* http://note.io/1usJZAA
* http://note.io/XK2J3Z
* http://note.io/1usK736

To test this I was just adding data from the split web console directly below the storage inspector - I think this will be a fairly common workflow for people at least until we get read/write in the UI. adding a cookie updated immediately but I didn't see updated ui when adding indexedDB or localstorage data.

( to be clear, I tested indexedDB by injecting the localforage library into the page and then using it )

Comment 108

[Girish Sharma [:Optimizer]]

IndexedDB is not live. There is no platform way to get live updates and I was told not to add one from the storage team.

Local Storage is giving live updates for me, was there any exception for you ?

Some recently changed which has not resulted in getting the method names as well while doing (for key in localStorage). Previously, only local storage items used to come.

Will raise a bug for it and will see why session storage is not working.

Comment 109

[Joe Walker [:jwalker]]

Comment on attachment 8469309 [details] [diff] [review]
patch v1.0

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

Thanks for getting this patch ready, Optimizer. I'd like to land it, and we can fix things like the icon glitches in-tree.
We've got 3 weeks before the next uplift which is plenty of time to tweak.

::: browser/devtools/storage/ui.js
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["StorageUI"];

This isn't needed any more.

@@ +112,5 @@
> +  },
> +
> +  /**
> +   * Removes the given item from the storage table. Also hides the sidebar if
> +   * the item was selected.

I think this comment is incorrect? The sidebar is re-shown?

Comment 110

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #109)
> Comment on attachment 8469309 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 8469309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for getting this patch ready, Optimizer. I'd like to land it, and we
> can fix things like the icon glitches in-tree.

The glitch issue is an in general issue for your light theme in osx. As mentioned by Brian and covered in Bug 1021564.


> We've got 3 weeks before the next uplift which is plenty of time to tweak.
> 
> ::: browser/devtools/storage/ui.js
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +this.EXPORTED_SYMBOLS = ["StorageUI"];
> 
> This isn't needed any more.
> 

My bad, forgot to remove.

> @@ +112,5 @@
> > +  },
> > +
> > +  /**
> > +   * Removes the given item from the storage table. Also hides the sidebar if
> > +   * the item was selected.
> 
> I think this comment is incorrect? The sidebar is re-shown?

Yup you are right. Will change.

Comment 111

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #110)
> (In reply to Joe Walker [:jwalker] from comment #109)
> > Comment on attachment 8469309 [details] [diff] [review]
> > patch v1.0
> > 
> > Review of attachment 8469309 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for getting this patch ready, Optimizer. I'd like to land it, and we
> > can fix things like the icon glitches in-tree.
> 
> The glitch issue is an in general issue for your light theme in osx. As
> mentioned by Brian and covered in Bug 1021564.

I'm not sure that this is the same bug, unless if you are using a filter in CSS on the icons for the dark theme (which is what Jeff is using in his screenshots).  There is a chance it will be fixed by it though, we should keep an eye on 1021564 and file a new bug if the problem persists after landing.

Comment 112

[Girish Sharma [:Optimizer]]

Yup, I am using filter only.

Comment 113

[Tim Nguyen :ntim]

Here are some bugs I found :
- Tree text doesn't have ellipsis overflow
- If an URL in the tree is long, the tree will be sized as big as the URL

Just confirming some known bugs :
- The localStorage displays methods too (setItem, getItem,...)
- sessionStorage doesn't work
- No live update.

Comment 114

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0.1

Carry forwarding r+

Fixed the session storage issues and live update issues and addressed the two review comments by Joe.

try is closed but will push as soon as it reopens.

(In reply to Tim Nguyen [:ntim] from comment #113)
> Here are some bugs I found :
> - Tree text doesn't have ellipsis overflow
> - If an URL in the tree is long, the tree will be sized as big as the URL

That is correct. But there is a max width to the tree.
 
> Just confirming some known bugs :
> - The localStorage displays methods too (setItem, getItem,...)
> - sessionStorage doesn't work
> - No live update.

Except for live update for indexed db, everything should be fixed now.

Comment 115

[Girish Sharma [:Optimizer]]

Here we go

https://tbpl.mozilla.org/?tree=Try&rev=d11810f205bb
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d11810f205bb

Comment 116

[Michael Cooper [:mythmon]]

I downloaded the try build and ran it. It looks really good, and is very useful to have.

I noticed two problems:

1) The Indexed DB view won't show more than 30 rows in my test (I am sure there are more than 30 items).
2) A blob stored in Indexed DB is represented as an empty object. This isn't a blocker, but it is a little confusing.

Comment 117

[Girish Sharma [:Optimizer]]

(In reply to Mike Cooper [:mythmon] from comment #116)
> I downloaded the try build and ran it. It looks really good, and is very
> useful to have.
> 
> I noticed two problems:
> 
> 1) The Indexed DB view won't show more than 30 rows in my test (I am sure
> there are more than 30 items).

30 is a pagination limit. I was thinking to make that 50, but still the limit would be present to stop bloating the connection with huge data transfer.

Bug 1031189 will add a toolbar with options to see the next/prev set of 30 values.

> 2) A blob stored in Indexed DB is represented as an empty object. This isn't
> a blocker, but it is a little confusing.

Can you give me a sample link/ test case so that I can see and look into it.

Comment 118

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #117)
> (In reply to Mike Cooper [:mythmon] from comment #116)
> > I downloaded the try build and ran it. It looks really good, and is very
> > useful to have.
> > 
> > I noticed two problems:
> > 
> > 1) The Indexed DB view won't show more than 30 rows in my test (I am sure
> > there are more than 30 items).
> 
> 30 is a pagination limit. I was thinking to make that 50, but still the
> limit would be present to stop bloating the connection with huge data
> transfer.
> 
> Bug 1031189 will add a toolbar with options to see the next/prev set of 30
> values.
You could also add an endless scrolling feature (you scroll to the bottom, the next 30 items appear).

Comment 119

[Girish Sharma [:Optimizer]]

What is it ? tumblr ?(In reply to Tim Nguyen [:ntim] from comment #118)
> (In reply to Girish Sharma [:Optimizer] from comment #117)
> > (In reply to Mike Cooper [:mythmon] from comment #116)
> > > I downloaded the try build and ran it. It looks really good, and is very
> > > useful to have.
> > > 
> > > I noticed two problems:
> > > 
> > > 1) The Indexed DB view won't show more than 30 rows in my test (I am sure
> > > there are more than 30 items).
> > 
> > 30 is a pagination limit. I was thinking to make that 50, but still the
> > limit would be present to stop bloating the connection with huge data
> > transfer.
> > 
> > Bug 1031189 will add a toolbar with options to see the next/prev set of 30
> > values.
> You could also add an endless scrolling feature (you scroll to the bottom,
> the next 30 items appear).

What is it ? tumblr ?

Comment 120

[Tim Nguyen :ntim]

Well, this is usually considered as the modern version of pagination ;)

Comment 121

[Tim Nguyen :ntim]

Also, it spares you clicks, and allows everything to be shown at once.

Comment 122

[Girish Sharma [:Optimizer]]

mass component move . filter on #MassComponentMoveStorageInspector

Comment 123

[Michael Cooper [:mythmon]]

This page demonstrates the blob problem I mentioned. Maybe this should be another bug. http://jsbin.com/zaxib/1/

Open the page, make sure it says "Stored a blob in indexeddb.", and then look at the storage inspector.

Comment 124

[Girish Sharma [:Optimizer]]

Just a note that the issues in comment 107 are regression from bug 660237 which changes the storage object and also adjusted the storage actor tests to accept the storage prototype methods.

Comment 125

[Girish Sharma [:Optimizer]]

tests incoming - https://tbpl.mozilla.org/?tree=Try&rev=a1e1b7ac2585

patch shortly!

Comment 126

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1

Final patch. Fixed failing tests in toolkit/devtools/server/test.

Added events at various locations to help test properly.

Comment 127

[Girish Sharma [:Optimizer]]

Attachment: tests - v1

Tests. This patch adds tests for:

- Making sure that all the storage items from all types are displayed properly. This test just confirms that all required rows in the table and items in the tree are present. Proper value checking is already covered by the toolkit tests.
- Making sure that sidebar close/open behavior is correct
- Making sure that the sidebar displays correct value and parse value.

green dt try : https://tbpl.mozilla.org/?tree=Try&rev=24002666e0a5

ongoing full try : https://tbpl.mozilla.org/?tree=Try&rev=6dcb7ceef4df

Comment 128

[Girish Sharma [:Optimizer]]

Comment on attachment 8477882 [details] [diff] [review]
patch v1.1

Asking for a quick review again.

Joe, this is the interdiff : https://bugzilla.mozilla.org/attachment.cgi?oldid=8472621&action=interdiff&newid=8477882&headers=1

Comment 129

[Paul Rouget [:paul]]

Did anyone test this tool with a Firefox OS device?

Comment 130

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #129)
> Did anyone test this tool with a Firefox OS device?

Yes I did, see bug 1049888. It applies both to e10s and FxOS the same way.

Comment 131

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #130)
> (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> comment #129)
> > Did anyone test this tool with a Firefox OS device?
> 
> Yes I did, see bug 1049888. It applies both to e10s and FxOS the same way.

Make sure the storage inspector tool doesn't show up when debugging Firefox OS or E10S-firefox.

Comment 132

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #131)
> (In reply to Girish Sharma [:Optimizer] from comment #130)
> > (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from
> > comment #129)
> > > Did anyone test this tool with a Firefox OS device?
> > 
> > Yes I did, see bug 1049888. It applies both to e10s and FxOS the same way.
> 
> Make sure the storage inspector tool doesn't show up when debugging Firefox
> OS or E10S-firefox.

It does not show up as the Actor even fails to create itself.

Comment 133

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Tim Nguyen [:ntim] from comment #118)
> You could also add an endless scrolling feature (you scroll to the bottom,
> the next 30 items appear).

The debugger already does this for very long stack traces:

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#574

Comment 134

[Girish Sharma [:Optimizer]]

What is the plan here ? Do we land this read only version before merge ?

Or land it right after the merge to get more testing throughout the cycle and bug 1031192 and bug 1049888 will also get fixed by next merge.

Comment 135

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #134)
> What is the plan here ? Do we land this read only version before merge ?
> 
> Or land it right after the merge to get more testing throughout the cycle
> and bug 1031192 and bug 1049888 will also get fixed by next merge.

what's the status of IDB refresh?

Comment 136

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #135)
> (In reply to Girish Sharma [:Optimizer] from comment #134)
> > What is the plan here ? Do we land this read only version before merge ?
> > 
> > Or land it right after the merge to get more testing throughout the cycle
> > and bug 1031192 and bug 1049888 will also get fixed by next merge.
> 
> what's the status of IDB refresh?

The method you suggested does nto give me a diff of changes, and calculating diff myself is hard and required me to store all existing IDB information (to compare on next reload)

Now if we don't have a diff, we will have to wipe the IDB data on the frontend (the tree items and table item if IDB related table is visible) and build the tree for IDB again (on each reload)

Comment 137

[Paul Rouget [:paul]]

I believe bug 1049888 and this IDB issue should block preffing on this tool. I'll let Mike decide, but I think it's ok to land this feature preffed off (not even present in the option panel). Then get one more cycle to fix bug 1049888 and the IDB issue. I don't think bug 1031192 is required to pref it on.

Comment 138

[Girish Sharma [:Optimizer]]

Are we at a point when we are blocking features on e10s support ?
IMHO bug 1031192 will be more useful to developers in general. I agree that bug 1049888 is a blocker for FxOS devs, but think about all the developers who will be using this tool.

I am not sure if there is a way to hide stuff from Options Panel and keep it disabled.

And about the IDB, I think until we get support from platform, a reload button is the only plausible solution

Comment 139

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #138)
> Are we at a point when we are blocking features on e10s support ?

We are actively working on fixing our tools for e10s. Let's not introduce a tool that is not e10s ready.

> IMHO bug 1031192 will be more useful to developers in general.I agree that
> bug 1049888 is a blocker for FxOS devs, but think about all the developers
> who will be using this tool.

I prefer if we could get it right now. How hard is it? Are we talking about months of work? Or weeks?

> I am not sure if there is a way to hide stuff from Options Panel and keep it
> disabled.
> 
> And about the IDB, I think until we get support from platform, a reload
> button is the only plausible solution

Let's get platform support then. Is there a bug filed somewhere?

Comment 140

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #139)
> (In reply to Girish Sharma [:Optimizer] from comment #138)
> > Are we at a point when we are blocking features on e10s support ?
> 
> We are actively working on fixing our tools for e10s. Let's not introduce a
> tool that is not e10s ready.
> 
> > IMHO bug 1031192 will be more useful to developers in general.I agree that
> > bug 1049888 is a blocker for FxOS devs, but think about all the developers
> > who will be using this tool.
> 
> I prefer if we could get it right now. How hard is it? Are we talking about
> months of work? Or weeks?

Its definetely more than a week's effort. Both cookies and IDB actors will live/talk to a script in parent process to do anything.

> > I am not sure if there is a way to hide stuff from Options Panel and keep it
> > disabled.
> > 
> > And about the IDB, I think until we get support from platform, a reload
> > button is the only plausible solution
> 
> Let's get platform support then. Is there a bug filed somewhere?

When I was working with the storage actors, the platform people asked me not to add such notifications for performance reasons. Anyways, filed bug 1059724.

Comment 141

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #138)
> Are we at a point when we are blocking features on e10s support ?
> IMHO bug 1031192 will be more useful to developers in general. I agree that
> bug 1049888 is a blocker for FxOS devs, but think about all the developers
> who will be using this tool.
> 
> I am not sure if there is a way to hide stuff from Options Panel and keep it
> disabled.

Could you just check the pref in main.js before adding the tool?

if (Services.prefs.getBoolPref("whatever")) {
  Tools.storage = {
    id: "storage",
    ...
  }
}

Comment 142

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #141)
> (In reply to Girish Sharma [:Optimizer] from comment #138)
> > Are we at a point when we are blocking features on e10s support ?
> > IMHO bug 1031192 will be more useful to developers in general. I agree that
> > bug 1049888 is a blocker for FxOS devs, but think about all the developers
> > who will be using this tool.
> > 
> > I am not sure if there is a way to hide stuff from Options Panel and keep it
> > disabled.
> 
> Could you just check the pref in main.js before adding the tool?
> 
> if (Services.prefs.getBoolPref("whatever")) {
>   Tools.storage = {
>     id: "storage",
>     ...
>   }
> }

Actually, I think the pref check would happen adding it to the defaultTools array.

Comment 143

[Joe Walker [:jwalker]]

To document an irc conversation, the plan is:

Land this visible in the options panel, but turned off by default.
Before 35 leaves central (i.e. 2014-10-13) we'll fix e10s, otherwise we'll hide it from the options panel with a pref. Other blockers may exist too.

We will also try to get read-write working for 35, but in my opinion this tool is useful without read-write.

Comment 144

[Girish Sharma [:Optimizer]]

Attachment: patch v1.2

Minor changes:

 - change the max item number to 50.
 - Fix some edge cases found while adding the new dynamic updates test

Comment 145

[Girish Sharma [:Optimizer]]

Attachment: tests v1.1

Added a new test to test that table values are live and get updated correctly.

try : https://tbpl.mozilla.org/?tree=Try&rev=c00e03bc30fa

Comment 146

[Girish Sharma [:Optimizer]]

Attachment: patch v1.3

One line change to make sure that table widget gets highlighted even if the same row is getting updated (added a sync reflow)

Comment 147

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/048321be093e
https://hg.mozilla.org/integration/fx-team/rev/12c0a04e2a7f

Comment 148

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/048321be093e
https://hg.mozilla.org/mozilla-central/rev/12c0a04e2a7f

Comment 149

[Francesco Lodolo [:flod]]

Painful to see these many strings landing so close to merge day (and I won't be online to answer questions too).

> # LOCALIZATION NOTE (open.accesskey): The access key used to open the storage
> # editor.

Which key? I don't see any key called 'open', so we're already missing a match between the accesskey and the label.

By looking at the code: is it the "storage" key? How can an accesskey use a letter not available in the label? Are you calling an accesskey what is actually a commandkey/shortcut?
http://hg.mozilla.org/mozilla-central/rev/048321be093e#l3.59

> storage.tooltip=Storage Inspector (Cookies, Local Storage ...)

As usual: ... -> … (single Unicode character instead of 3 dots).

table.headers.cookies.lastAccessed:Last accessed on
table.headers.cookies.creationTime:Created on
table.headers.cookies.isHttpOnly:isHttpOnly
table.headers.cookies.isSecure:isSecure
table.headers.cookies.isDomain:isDomain

This is not how you write keys in a .properties file. Also, the last 3 values seem wrong, even imagining '=' instead of ':'

Comment 150

[Girish Sharma [:Optimizer]]

(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #149)
> Painful to see these many strings landing so close to merge day (and I won't
> be online to answer questions too).

Are there guidelines to not land strings close to merge day ? Especially at a cost of a new developer tools is landing ?

> > # LOCALIZATION NOTE (open.accesskey): The access key used to open the storage
> > # editor.
> 
> Which key? I don't see any key called 'open', so we're already missing a
> match between the accesskey and the label.
> 

Not sure what you really mean by this line, but I think that access key should now be either a. At the beginning of this patch, no character was available, so I went with something else.

I'll file a bug to fix that and the below mentioned things.

> As usual: ... -> … (single Unicode character instead of 3 dots).

Will fix.

> table.headers.cookies.lastAccessed:Last accessed on
> table.headers.cookies.creationTime:Created on
> table.headers.cookies.isHttpOnly:isHttpOnly
> table.headers.cookies.isSecure:isSecure
> table.headers.cookies.isDomain:isDomain
> 
> This is not how you write keys in a .properties file. Also, the last 3
> values seem wrong, even imagining '=' instead of ':'

No idea how I ended up writing : instead of =, and surprisingly, it works too!, but apart from that, what is wrong in the last three entries ? These are properties on cookies, Firebug also has the isHttpOnly one as HttpOnly only.

What else do you suggest ?

Comment 151

[Girish Sharma [:Optimizer]]

Filed bug 1060835

Comment 152

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #150)
> (In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from
> comment #149)
> > Painful to see these many strings landing so close to merge day (and I won't
> > be online to answer questions too).
> 
> Are there guidelines to not land strings close to merge day ? Especially at
> a cost of a new developer tools is landing ?
> 
> > > # LOCALIZATION NOTE (open.accesskey): The access key used to open the storage
> > > # editor.
> > 
> > Which key? I don't see any key called 'open', so we're already missing a
> > match between the accesskey and the label.
> > 
> 
> Not sure what you really mean by this line, but I think that access key
> should now be either a. At the beginning of this patch, no character was
> available, so I went with something else.
An accesskey is already for opening/accessing something, so it's probably more descriptive and consistent with other files to use storage.accesskey

> I'll file a bug to fix that and the below mentioned things.
> 
> > As usual: ... -> … (single Unicode character instead of 3 dots).
> 
> Will fix.
> 
> > table.headers.cookies.lastAccessed:Last accessed on
> > table.headers.cookies.creationTime:Created on
> > table.headers.cookies.isHttpOnly:isHttpOnly
> > table.headers.cookies.isSecure:isSecure
> > table.headers.cookies.isDomain:isDomain
> > 
> > This is not how you write keys in a .properties file. Also, the last 3
> > values seem wrong, even imagining '=' instead of ':'
> 
> No idea how I ended up writing : instead of =, and surprisingly, it works
> too!, but apart from that, what is wrong in the last three entries ? These
> are properties on cookies, Firebug also has the isHttpOnly one as HttpOnly
> only.
It might end up strange translating properties like isDomain (with no space between is and domain). So you might want to add in the l10n note that it shouldn't be translated and why. I'm guessing that's intended because it's a cookie property name (I haven't worked much with Cookies so I don't know).

Comment 153

[Tim Nguyen :ntim]

You might also want to change open.commandkey to storage.commandkey to match other files.

Comment 154

[Francesco Lodolo [:flod]]

(In reply to Girish Sharma [:Optimizer] from comment #150)
> Are there guidelines to not land strings close to merge day ? Especially at
> a cost of a new developer tools is landing ?

No, it's just a practical matter. Strings land close to merge day, they'll move to Aurora and they can't be touched there if they have errors (like in this case). Normally merges wouldn't happen on Saturday, and I wouldn't be checking strings on a Saturday evening either.

> Not sure what you really mean by this line, but I think that access key
> should now be either a. At the beginning of this patch, no character was
> available, so I went with something else.

Let's say the string is called "whatever", the accesskey should be "whatever.accesskey", and the commandkey "whatever.commandkey".

Tools try to match a string with its accesskey/commandkey, to allow checks and in same cases to display string and accesskey together for localization.

So, please call the keys 'storage.accesskey" and "storage.commandkey" if they're related to 'storage'.

See also this recent discussion on dev-l10n
See also https://groups.google.com/forum/#!topic/mozilla.dev.l10n/JC_3WGf9_5A

> No idea how I ended up writing : instead of =, and surprisingly, it works
> too!, but apart from that, what is wrong in the last three entries ? These
> are properties on cookies, Firebug also has the isHttpOnly one as HttpOnly
> only.

If that's the value you want to display, fine by me. As Tim wrote, add a comment explaining that's a value and shouldn't actually be localized.

Comment 155

[Girish Sharma [:Optimizer]]

I have started writing some documentation, but help is always appreciated :)

Comment 156

[Francesco Lodolo [:flod]]

I know this bug is pretty old, but: is there a specific reason for using "Indexed DB" instead of "IndexedDB" as label? Is it wanted/correct?

Comment 157

[Will Bamberg [:wbamberg]]

This has documentation (written by Optimizer): https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector
Marking dev-doc-complete.