Closed Bug 1280548 Opened 8 years ago Closed 7 years ago

Don't display storage-sidebar after deleting all cookies because no data

Categories

(DevTools :: Storage Inspector, defect, P3)

defect

Tracking

(firefox48 affected, firefox49 affected, firefox50 affected, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox48 --- affected
firefox49 --- affected
firefox50 --- affected
firefox56 --- fixed

People

(Reporter: magicp.jp, Assigned: miker)

References

Details

(Whiteboard: [todo-mr][t1])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160616030228

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. https://addons.mozilla.org)
3. Open DevTools > Storage Inspactor
4. Select Cookies and any rows (storage-sidebar will be displayed)
5. Select "Delete All" in right-clicking menu



Actual results:

Storage-sidebar is still displayed. Probably keeping a selected row. This because after reloading current page and getting again cookies, previous selected row will be still kept. (two rows are selected)

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4832ac37867f93eb09ec5f057b82c7195de54456&tochange=b6f71c9b42b9cffc31e346a447bf736130211cf9


Expected results:

Don't display storage-sidebar after deleting all cookies because no data. And clearing selected row.
Blocks: 1260380
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Storage Inspector
Delete All should clearly close the sidebar if it is open.
Whiteboard: [todo-mr]
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
There were multiple issues causing this:

- We were sometimes trying to select the row after it was deleted and silently throwing.
- ui.js::displayObjectSidebar() is an async function but in ui.js::removeItemFromTable() we did not yield when calling it. Because ui.js::removeItemFromTable() is called a bunch of times when "delete all" is selected this meant that displayObjectSidebar's check to see if it should show the sidebar was detecting the wrong number of rows causing it to display the sidebar.

Both issues are fixed.
Comment on attachment 8877650 [details]
Bug 1280548 - Don't display storage-sidebar after deleting all cookies

https://reviewboard.mozilla.org/r/149106/#review153522

Thanks for the patch! Fixes the issue but I think we can have something simpler in storage/ui.js. 
Let me know what you think!

::: devtools/client/shared/widgets/TableWidget.js:1255
(Diff revision 1)
>      if (index < 0) {
>        this.selectedRow = null;
>        return;
>      }
>      let cell = this.cells[index];
> +    if (cell) {

We check index < 0 to set this.selectedRow = null just above. I think we merge both blocks as:

> let cell = this.cells[index];
> if (cell) {
>   cell.toggleClass("theme-selected");
>   this.selectedRow = cell.id;
> } else {
>   this.selectedRow = null;
> }

::: devtools/client/storage/ui.js:294
(Diff revision 1)
>      this.updateSidebarToggleButton();
>    },
>  
> +  hideSidebarOnEmptyTable: function () {
> +    if (this.table.items.size === 0) {
> +      this.table.selectedRow = null;

I find unexpected to update table.selectedRow from a method which is just supposed to update the visibility of a sidebar in case the table is empty.

What about udpating TableWidget.js::remove() in order to clear the selectedRow if this.items.size === 0 (it is already checked in order to display the placeholder text).

::: devtools/client/storage/ui.js:342
(Diff revision 1)
> +
>        this.table.remove(name);
> -      this.displayObjectSidebar();
> +
> +      // displayObjectSidebar is async so we need to see whether the sidebar should
> +      // be hidden after calling it (rows could have been removed whilst it is running).
> +      this.displayObjectSidebar().then(this.hideSidebarOnEmptyTable);

I think it's the responsibility of `displayObjectSidebar()` to know whether to show up or not. 

There is a single yield in it. After yielding, we could check if the selectedRow for the table is still the expected item. If it's not it means the state changed, and we should bail out.

This way we can leave most of the code here untouched, and hideSidebarOnEmptyTable is not needed anymore.

::: devtools/client/storage/ui.js:683
(Diff revision 1)
>  
>    /**
>     * Populates the selected entry from the table in the sidebar for a more
>     * detailed view.
>     */
>    displayObjectSidebar: Task.async(function* () {

not related to your change but this method is actually more "updating" the object sidebar, rather than just displaying it. 

If there is a selected row, then it displays its content in the sidebar. Otherwise it hides the sidebar.
Attachment #8877650 - Flags: review?(jdescottes)
Attachment #8877650 - Flags: review?(jdescottes)
Comment on attachment 8877650 [details]
Bug 1280548 - Don't display storage-sidebar after deleting all cookies

https://reviewboard.mozilla.org/r/149106/#review154608

Thanks for the updated patch Mike!

Let's clarify why remove() and clear() were updated on the Column instead of the TableWidget. Maybe it's a mistake?
Should be good to land after that.

::: devtools/client/shared/widgets/TableWidget.js:1394
(Diff revision 3)
>      }
>      this.cells[index].destroy();
>      this.cells.splice(index, 1);
>      delete this.items[item[this.uniqueId]];
> +
> +    if (this.items.size === 0) {

In my previous review, I was referring to TableWidget::remove() not Column::remove(). I think this makes more sense in TableWidget::remove() where we already have a check for this.items.size === 0 (http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/devtools/client/shared/widgets/TableWidget.js#856-858) 

Is there a good reason for updating the Column class here?

::: devtools/client/shared/widgets/TableWidget.js
(Diff revision 3)
>     */
>    clear: function () {
>      this.cells = [];
>      this.items = {};
>      this._itemsDirty = false;
> -    this.selectedRow = null;

not sure this should be removed.

::: devtools/client/shared/widgets/TableWidget.js:1490
(Diff revision 3)
> +      if (even) {
> +        cell.classList.add("even");
> +      } else {
> +        cell.classList.remove("even");
> +      }

I think cell.classList.toggle("even", even); would be equivalent?

::: devtools/client/storage/ui.js:674
(Diff revision 3)
> -      return;
> -    }
>  
>      // Get the string value (async action) and the update the UI synchronously.
> -    let value;
> +    if (item && item.name && item.valueActor) {
> -    if (item.name && item.valueActor) {
> +      if (item.name && item.valueActor) {

this inner if() is redundant with the one you added and can be removed.
Attachment #8877650 - Flags: review?(jdescottes)
Comment on attachment 8877650 [details]
Bug 1280548 - Don't display storage-sidebar after deleting all cookies

https://reviewboard.mozilla.org/r/149106/#review154608

> In my previous review, I was referring to TableWidget::remove() not Column::remove(). I think this makes more sense in TableWidget::remove() where we already have a check for this.items.size === 0 (http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/devtools/client/shared/widgets/TableWidget.js#856-858) 
> 
> Is there a good reason for updating the Column class here?

No, I completely misunderstood what you were saying... of course it makes more sense for it to be in TableWidget::remove().

> not sure this should be removed.

It should... we call column.clear() from 2 places:
  - TableWidget.clear where we reset the selected row.
  - From the tableWidget constructor.

It is not needed here, which is why I removed it.

> I think cell.classList.toggle("even", even); would be equivalent?

I was trying to discourage toggle because it is generally better to use add and remove in order to avoid getting out of sync.

I guess this is less of a concern with ebra striping so I will use toggle instead.

> this inner if() is redundant with the one you added and can be removed.

lol... I must have been distracted part way through making that change, fixed.
Comment on attachment 8877650 [details]
Bug 1280548 - Don't display storage-sidebar after deleting all cookies

https://reviewboard.mozilla.org/r/149106/#review155182

Looks good to me, thanks Mike!
Attachment #8877650 - Flags: review?(jdescottes) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edf38274225e
Don't display storage-sidebar after deleting all cookies r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/edf38274225e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Operating System:Windows 10 x64
Firefox version:52.0a.1

I have tried to reproduce this bug and it's reproducing the same.
It's displaying the storage bar after clearing all the data.

[bugday-20170628]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: