Table headers stay the same when switching to an empty storage type

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Storage Inspector
P2
minor
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: sebo, Assigned: Fischer, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox50 verified)

Details

(Whiteboard: [good first bug][lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

58 bytes, text/x-review-board-request
miker
: review+
Details | Review
(Reporter)

Description

2 years ago
The table headers of the previously selected storage are still shown when selecting a storage, which doesn't contain any items.

Steps to reproduce:
1. Open the Storage Inspector on this page
2. Expand 'Cookies' and select 'bugzilla.mozilla.org'
   => Some cookies will be shown in a table with the headers 'Name', 'Path', ..., 'isHttpOnly'
3. Select 'Local Storage'

=> The headers of the 'Cookies' table are still shown.

Sebastian
Priority: -- → P2
Mentor: mratcliffe@mozilla.com
Whiteboard: [good-first-bug lang=js]
(Reporter)

Comment 1

2 years ago
I might be wrong, but I think bugsahoy requires the two whiteboard flags to be split.

Sebastian
Whiteboard: [good-first-bug lang=js] → [good-first-bug][lang=js]
(Reporter)

Updated

2 years ago
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
This bug happens because the table headers are reset only after at least one row of data is received from the server [1]. If there are no data, the client doesn't even know which columns are there - it fully depends on the info provided by the server.

Mike, how would you recommend to fix this? I see two ways:
1) The knowledge about which columns are displayed for each data type would be embedded in the client. This would also remove the need for the getEditableFields() method, as the client would already know what is editable and what isn't.
2) The getEditableFields() method would be transformed into getFields(), and would return the full column info about the table (names and editable flags). Independently from the getStoreObjects() return value. I like this approach more than #1, because it keeps the knowledge about data at one place, the server.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/storage/ui.js#420
Flags: needinfo?(mratcliffe)
I like #2 best. If we do that we can also include data types for the columns (useful to e.g. display a date picker for dates) and, in the future, validation functions.

Definitely the way to go.
Flags: needinfo?(mratcliffe)

Comment 4

a year ago
I would like to take up this bug. Can you give me the specifics ? Thanks in advance.
(Reporter)

Comment 5

a year ago
First you need to set up a build environment as described in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions.

The relevant comments are comment 0, comment 2 and comment 3. Mike Ratcliffe may provide more info on this.

Sebastian
(Assignee)

Comment 6

a year ago
@Michael,
Since this bug hasn't been touched for almost 1 month, I'd like to take on it.

Some question about the solution, I'd like to discuss with you:
I can find out the getEditableFields() method in [1]. If we are going for the solution #2 at comment 2 proposed by Jarda.
About the returned data format for column info, I am thinking maybe it could return like something below:
```
[
  { name: "name", editable: 1 },
  { name: "value", editable: 1 },
  // For the future extension, such as including data type. It could be
  // { name: "last-updated-time", editable: 1, dataType: "date" },
  .....
]
```

How do you think about this data format?

Please let know me any suggestion or concern, thank you.


[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/storage.js#1022
Assignee: nobody → fliu
Flags: needinfo?(mratcliffe)
(In reply to [:Fischer]Fischer from comment #6)
> @Michael,
> Since this bug hasn't been touched for almost 1 month, I'd like to take on
> it.
> 
> Some question about the solution, I'd like to discuss with you:
> I can find out the getEditableFields() method in [1]. If we are going for
> the solution #2 at comment 2 proposed by Jarda.
> About the returned data format for column info, I am thinking maybe it could
> return like something below:
> ```
> [
>   { name: "name", editable: 1 },
>   { name: "value", editable: 1 },
>   // For the future extension, such as including data type. It could be
>   // { name: "last-updated-time", editable: 1, dataType: "date" },
>   .....
> ]
> ```
> 
> How do you think about this data format?
> 
> Please let know me any suggestion or concern, thank you.
> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> storage.js#1022

Yes, this is exactly the way we should go. Columns can also be hidden but I am not sure if you will need to include that in the column meta.
Flags: needinfo?(mratcliffe)
Blocks: 1268844
(Assignee)

Comment 8

a year ago
Created attachment 8767940 [details]
1264582-patch-v1

@Mike,

Please see my patch:1264582-patch-v1 for this bug. Just tested and now even no data for one storage type, the table column name would still be updated when selecting host.
Here are some questions, could you help to give me some feedbacks:

- Currently it is that selecting one *host* will trigger the table column update not *Storage title*. This is to follow existing logic of "onHostSelect" event. I am not sure if this is enough since the bug description is "Select Local Storage"(Storage title*). But right now the situation is improved comparing to the old situation that the table only updates when data exists under one host.

- The "getEditableFields" is replaced by the "getFields" method. However, for the indexedDB type, the situation is a little bit complex because it have sub types: database and object store, not only host. As a result, we need to do sub type handling. Also, the metadata of indexedDB is kind of different from field info so I don't add the field info into metadata. Please let me if there is any better approach.

Thanks
Attachment #8767940 - Flags: feedback?(mratcliffe)
Depends on: 1286206
@Fischer: I have no access to the review / feedback request until you publish it (it is currently a draft). Can you open it in reviewboard and click publish?
Flags: needinfo?(fliu)
(Assignee)

Comment 10

a year ago
Created attachment 8770361 [details]
Bug 1264582 - Table headers are not removed when selecting an empty storage

Review commit: https://reviewboard.mozilla.org/r/62308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62308/
(Assignee)

Comment 11

a year ago
@Mike,
Sorry, didn't notice that. Now it is published, thank you.
Flags: needinfo?(fliu) → needinfo?(mratcliffe)
I am reviewing things at the moment but will review this soon.
Flags: needinfo?(mratcliffe)
Attachment #8770361 - Attachment is obsolete: true
Attachment #8770361 - Flags: review-
Comment on attachment 8770361 [details]
Bug 1264582 - Table headers are not removed when selecting an empty storage

https://reviewboard.mozilla.org/r/62308/#review63372

A great set of changes that begins to introduce the possibility of specifying column data types in the future. In addition it fixes the table headers.

If you correct the grammatical errors I have highlighted and the try run is green I will be happy to r+ it.

::: devtools/client/storage/ui.js:221
(Diff revision 1)
>  
> -  makeFieldsEditable: function* () {
> -    let actor = this.getCurrentActor();
> -
> -    if (typeof actor.getEditableFields !== "undefined") {
> -      let fields = yield actor.getEditableFields();
> +  /**
> +   *  Make column fields editable
> +   *
> +   *  @param {Array} editableFields
> +   *         An array of keys of columns which being made editable

An array of keys of columns which to be made editable

::: devtools/server/actors/storage.js:84
(Diff revision 1)
>   *                        store objects.
>   *   - toStoreObject : Given a store object, convert it to the required format
>   *                     so that it can be transferred over wire.
>   *   - populateStoresForHost : Given a host, populate the map of all store
>   *                             objects for it
> + *   - getFields: Given a subType(optional), get an array of object containing

get an array of objects containing
Sorry, the above corrections should be:
An array of keys of columns to be made editable

And:
get an array of objects containing

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11e6f29db3a8
https://reviewboard.mozilla.org/r/62308/#review63372

> An array of keys of columns which to be made editable

Sorry, that should be:
An array of keys of columns to be made editable
(Assignee)

Comment 16

a year ago
Comment on attachment 8770361 [details]
Bug 1264582 - Table headers are not removed when selecting an empty storage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62308/diff/1-2/
Attachment #8770361 - Attachment is obsolete: false
Attachment #8770361 - Flags: review-
(Assignee)

Updated

a year ago
Attachment #8767940 - Attachment is obsolete: true
Attachment #8767940 - Flags: feedback?(mratcliffe)
(Assignee)

Comment 17

a year ago
Comment on attachment 8770361 [details]
Bug 1264582 - Table headers are not removed when selecting an empty storage

@Mike,
Thank you for correcting the grammar and pushing the try server.
I will do it next time.
I have correct the grammar. Please see the patch.
Thanks
Attachment #8770361 - Flags: review?(mratcliffe)
Comment on attachment 8770361 [details]
Bug 1264582 - Table headers are not removed when selecting an empty storage

https://reviewboard.mozilla.org/r/62308/#review63582
Attachment #8770361 - Flags: review?(mratcliffe) → review+
Keywords: checkin-needed

Comment 19

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e6c96d5612d3
Table headers are not removed when selecting an empty storage. r=mratcliffe
Keywords: checkin-needed

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6c96d5612d3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Reporter)

Comment 21

a year ago
Sorry to say that, but this bug is not fixed for me as of Nightly 51.0a1 (2016-08-02). The headers are still not removed when the list is empty. See the test case in comment 0.
Mike, should we reopen this bug or create a new one?

Sebastian
Flags: needinfo?(mratcliffe)
(In reply to Sebastian Zartner [:sebo] from comment #21)
> Sorry to say that, but this bug is not fixed for me as of Nightly 51.0a1
> (2016-08-02). The headers are still not removed when the list is empty. See
> the test case in comment 0.
> Mike, should we reopen this bug or create a new one?
> 
> Sebastian

Please can you create a new one?
Flags: needinfo?(mratcliffe)
(Reporter)

Updated

a year ago
Blocks: 1291427
(Reporter)

Comment 23

a year ago
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> (In reply to Sebastian Zartner [:sebo] from comment #21)
> > Sorry to say that, but this bug is not fixed for me as of Nightly 51.0a1
> > (2016-08-02). The headers are still not removed when the list is empty. See
> > the test case in comment 0.
> > Mike, should we reopen this bug or create a new one?
> > 
> > Sebastian
> 
> Please can you create a new one?

Ok, I've filed bug 1291427. Maybe you can change the summary of this one to reflect the changes done here.

Sebastian
I have reproduced this bug with Nightly 48.0a1 (2016-04-14) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Beta
 Build ID   :  20161010144024
 User Agent :  Mozilla/5.0(Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
[bugday-20161012]
(Reporter)

Comment 25

a year ago
According to comment 24 I mark this as VERIFIED.

Sebastian
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
(Reporter)

Comment 26

a year ago
To be clear, what has changed is that the column headers are now reset when you select another storage.  So I changed the summary to reflect that.

Though the headers are still shown for empty storages and when selecting a storage type. This is now covered by bug 1291427.

Sebastian
Summary: Table headers are not removed when selecting an empty storage → Table headers stay the same when switching to an empty storage type
You need to log in before you can comment on or make changes to this bug.