Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Description • 9 years ago

      No description provided.

	courriel
	Comment 1 • 8 years ago

Add and update and delete , no ?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 8 years ago

(In reply to courriel from comment #1)
> Add and update and delete , no ?

You can already update a cookie by double-clicking items in the table and delete them via the context menu.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 3 • 8 years ago

Bug 1279602 discusses the UX and we decided to add the plus that is present in the Inspector. When the plus is clicked we simply add an editable line to the cookie table with sensible default values and select that line.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 5 • 8 years ago

Filter on Brobdingnagian.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 6 • 7 years ago

The difficulty I am having is that the row could appear anywhere depending on which column is sorted. Because we have infinite scrolling in the Storage Inspector the new cookie may not even be on the same page.

To fix this we really need to fix sorting first.

	Sebastian Zartner [:sebo]
	Comment 7 • 7 years ago

I'd say this doesn't necessarily depend on the sorting. To enter a new cookie the new line should always appear at the top or the bottom of the list of cookies independently of the column sorting. Only after you entered all data the cookie should be sorted (and may disappear from the viewport because of that).

Or do you imagine a different UX, Mike?

Sebastian

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 8 • 7 years ago

(In reply to Sebastian Zartner [:sebo] from comment #7)
> I'd say this doesn't necessarily depend on the sorting. To enter a new
> cookie the new line should always appear at the top or the bottom of the
> list of cookies independently of the column sorting. Only after you entered
> all data the cookie should be sorted (and may disappear from the viewport
> because of that).
> 
> Or do you imagine a different UX, Mike?
> 
> Sebastian

In the case of using the plus button to add a cookie I completely agree.

When a user right-clicks a row and chooses add cookie we should add the cookie as close to that row as possible with similar settings (like copy / paste). In this situation the column sort is vital and there is no point writing an algorithm now and then rewriting it when the column sort is fixed.

This is a feature that that we really want to work well so we it is well worth switching to natural sort first... I wouldn't pref it on by default without first fixing the sorting issue.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 9 • 7 years ago

I think that adding the cookie to the top of the list is the best option.

	Patrick Brosset <:pbro>
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review140478

This looks really good, good work Mike. I don't have many comments.

- One general UX thing: could we scroll to the new cookie automatically? If you have many cookies already, and a scrollbar, then clicking the + button may feel like it doesn't work. That's because the new cookie is added at the bottom of the list and you can't see it.
- Small design issue: the + button seems to have a larger right padding. Hovering the button turns its background to a darker grey, and makes this visible. https://www.dropbox.com/s/tb2lvlpz8r3qqc4/plus-sign-padding.PNG?dl=0
- Another thing with the + button: if I select a cookies domain, I can see the button, fine. Then if I click on the Indexed DB **folder** I can still see the button. It should become hidden as soon as I select something in the tree that does not support insertion, including folders.
- One last thing about backward compatibility: I can still see the + sign and context menu even when connecting to older backends that do not support adding cookies. The good point is clicking the button doesn't seem to cause errors, and nothing happens, but the button and menu should be hidden in this case.

	Tim Nguyen :ntim
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review140644

::: devtools/client/themes/storage.css:37
(Diff revision 1)
>    min-width: 250px;
>  }
>  
> +#storage-toolbar .add-button::before {
> +  background-image: url("chrome://devtools/skin/images/add.svg");
> +  list-style-image: url("chrome://devtools/skin/images/add.svg");

This line has no effect.

	Tim Nguyen :ntim
	Comment 13 • 7 years ago

> - Small design issue: the + button seems to have a larger right padding. Hovering the button turns its background to a darker grey, and makes this visible. https://www.dropbox.com/s/tb2lvlpz8r3qqc4/plus-sign-padding.PNG?dl=0

This is because .devtools-button is being used on a XUL <button>, you should either use .devtools-button on a HTML button, or .devtools-toolbarbutton on a XUL <toolbarbutton>

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review140644

> This line has no effect.

That is where the background is coming from... on the other hand `background-image: url("chrome://devtools/skin/images/add.svg");` was doing nothing so I have removed that.

The document is a strange mixture of XUL and HTML and will soon be changed to HTML so let's go with what works. I have removed the extra padding by hiding the innards of the XUL button.

	Patrick Brosset <:pbro>
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review141558

::: commit-message-6d452:3
(Diff revision 2)
> +> One general UX thing: could we scroll to the new cookie automatically? If you have many cookies already, and a scrollbar, then clicking the + button may feel like it doesn't work. That's because the new cookie is added at the bottom of the list and you can't see it.
> +
> +After a world of XUL pain I managed to find a simple way to get this working.
> +
> +> Small design issue: the + button seems to have a larger right padding. Hovering the button turns its background to a darker grey, and makes this visible. https://www.dropbox.com/s/tb2lvlpz8r3qqc4/plus-sign-padding.PNG?dl=0
> +
> +Fixed
> +
> +> Another thing with the + button: if I select a cookies domain, I can see the button, fine. Then if I click on the Indexed DB **folder** I can still see the button. It should become hidden as soon as I select something in the tree that does not support insertion, including folders.
> +
> +Fixed
> +
> +> One last thing about backward compatibility: I can still see the + sign and context menu even when connecting to older backends that do not support adding cookies. The good point is clicking the button doesn't seem to cause errors, and nothing happens, but the button and menu should be hidden in this case.
> +
> +Fixed

This discussion should not be inside the commit message.
Use these extra lines to describe the changes if needed instead.

::: devtools/client/shared/widgets/TableWidget.js:1191
(Diff revision 2)
> +        // We are updating a row and scrollIntoViewOnUpdate is set but we can
> +        // only scroll to cells if they are visible. We check for visibility
> +        // using clientHeight. Once we find the first visible cell in a row we
> +        // scroll it into view and reset the scrollIntoViewOnUpdate flag.

Not sure I understand this. Is `onRowUpdated` called several times in a sequence?

::: devtools/client/storage/test/head.js:974
(Diff revision 2)
> +  let toolbar = gPanelWindow.document.getElementById(
> +    "storage-toolbar");

nit: our eslint line max-length is 90 now. This fits on 1 line.

::: devtools/client/storage/ui.js:1033
(Diff revision 2)
>      let selectedItem = this.tree.selectedItem;
>      let type = selectedItem[0];
>      let actor = this.getCurrentActor();
>  
>      // IndexedDB only supports removing items from object stores (level 4 of the tree)
> -    if (!actor.removeItem || (type === "indexedDB" && selectedItem.length !== 4)) {
> +    if ((!actor.addItem && !actor.removeItem) ||

This works in cases where we right click on a table where removing or adding isn't supported, but it doesn't work in cases where we are connecting to an older backend.

`actor` here is a `Front` object. Fronts are instantiated automatically from the protocol spec, on the client-side, so they always have the methods you need. But that doesn't mean the corresponding `Actor` instance on the server has those methods too.

Fronts and Actors are instantiated separately, on each side of the connection, with whatever knowledge they each have eof the protocol spec.
So if you connect to an older server, the Actor will be instantiated without the `addItem` method, because the code there just doesn't have this method, it's older than your change.
The Front however will see that the spec has this method, and so it will implement the client-side version of it.

When you'll try to call it, it will send a request that can't be executed once it reaches the server.

So, the specific problem we have here is that the add item menu does show when you right click on a cookies table, even though you connect to an older backend.

So it looks like here, we need to check both `actor.addItem` so that the item doesn't appear for IndexedDB, **and** use the `actorHasMethod` function to check that, even though you're on a cookies table, the backend does support the add feature.

You could alternatively check this once when the tool starts only, and then store this somewhere.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review141558

> Not sure I understand this. Is `onRowUpdated` called several times in a sequence?

Yes, a cell is added to the row once for each column... I have updated the function description and my comment to explain this:

Function description:
```
   * Called when a row is updated e.g. a cell is changed. This means that
   * for a new row this method will be called once for each column. If a single
   * cell is changed this method will be called just once.
```

Comment:
```
  // When a new row is created this method is called once for each column
  // as each cell is updated. We can only scroll to cells if they are
  // visible. We check for visibility and once we find the first visible
  // cell in a row we scroll it into view and reset the
  // scrollIntoViewOnUpdate flag.
```

	Patrick Brosset <:pbro>
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review142022

	Tim Nguyen :ntim
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review142034

::: devtools/client/storage/storage.xul:46
(Diff revision 7)
>          <textbox id="storage-searchbox"
>                   class="devtools-filterinput"
>                   type="search"
>                   timeout="200"
>                   placeholder="&searchBox.placeholder;"/>
> +        <button id="add-button"
> +                class="devtools-button add-button"></button>
>          <spacer flex="1"/>
>          <button class="devtools-button sidebar-toggle" hidden="true"></button>

Can we match the inspector layout here?

+                  [Search     ] [>]

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 26 • 7 years ago
mozreview-review-reply

Comment on attachment 8864927 [details]
Bug 1231452 - Make it possible to add cookies to the storage inspector

https://reviewboard.mozilla.org/r/136586/#review142034

> Can we match the inspector layout here?
> 
> +                  [Search     ] [>]

We probably should but not as part of this bug.

Can you log a new bug for this?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 27 • 7 years ago

pbrosset asked about why I switched from using Services in the child to create the cookie to using document.cookie in the parent.

To use Services we need to cross process boundaries and that is costly. By using document.cookie we lose nothing and gain some performance.

	Tim Nguyen :ntim
	Comment 28 • 7 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #26)
> Comment on attachment 8864927 [details]
> Bug 1231452 - Make it possible to add cookies to the storage inspector
> 
> https://reviewboard.mozilla.org/r/136586/#review142034
> 
> > Can we match the inspector layout here?
> > 
> > +                  [Search     ] [>]
> 
> We probably should but not as part of this bug.
> 
> Can you log a new bug for this?

Bug 1364437

	Pulsebot
	Comment 29 • 7 years ago

Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5872d9137b51
Make it possible to add cookies to the storage inspector r=pbro

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5872d9137b51