1194190 - Enable Storage Inspector by default

Status: VERIFIED FIXED

(DevTools :: Storage Inspector, defect)

Comment 1

[Patrick Brosset <:pbro>]

I'd argue that bug 1031189 shouldn't block this one. Do we need it for edit mode? If yes then fine, let's leave it here. If not, maybe we can do it later.

Comment 2

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #1)
> I'd argue that bug 1031189 shouldn't block this one. Do we need it for edit
> mode? If yes then fine, let's leave it here. If not, maybe we can do it
> later.

We don't need it for edit mode, as we can use a context menu for that. I agree we can do it later.

Comment 3

[Tim Nguyen :ntim]

I think the tool is complete enough to be enabled by default for Firefox 48. Edit and Delete support got implemented, and I think that's enough to allow it to be enabled.

What do you think ?

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Yes, we can now enable it by default.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> Yes, we can now enable it by default.

Was this discussed further outside of the bug?  Enabling a new tool by default is a pretty big change and we should discuss with product / ux before doing it.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> > Yes, we can now enable it by default.
> 
> Was this discussed further outside of the bug?  Enabling a new tool by
> default is a pretty big change and we should discuss with product / ux
> before doing it.

I have discussed this with product, UX and my manager in the past but as long as you are asking.

@Helen and Brian: Do we need to do anything else before making the storage inspector visible by default?

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

There are two things that would make me feel fine with the change, and neither of them are Storage-panel specific. I'm listing them here and hopefully Bryan can weigh in on whether or not he agrees and whether or not he considers either of these blockers:

- I think the Style Editor should be turned /off/ by default. It's just such a weird tool and isn't as useful as Chrome's Resources panel, which is what everyone seems to wish it was.
- I think having better panel management would put a lot of these questions to rest: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%5Bdevtools-toolbar%5D&list_id=12972387 We get antsy turning on/off new tools because they're really space-greedy on the toolbar at the moment. Fang and I are working on this, but it seems annoying to block this bug on that.

Bryan, what do you think?

Comment 8

[Joe Walker [:jwalker]]

Style Editor off by default is OK by me, but maybe we should think again when we have better panel management. Also happy for Bryan to have the final call.
Lets do that in a separate bug though?

Comment 9

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #8)
> Style Editor off by default is OK by me, but maybe we should think again
> when we have better panel management. Also happy for Bryan to have the final
> call.
> Lets do that in a separate bug though?

Chatted with Bryan some about this—sounds like there's no problem from him or I about turning on the Storage panel by default, and Style Editor/panel management stuff belongs in other bugs.

I won't clear his ni but I'm fairly certain it's safe to go ahead and implement the change. I'll ping him once he's on IRC.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #9)
> (In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment
> #8)
> > Style Editor off by default is OK by me, but maybe we should think again
> > when we have better panel management. Also happy for Bryan to have the final
> > call.
> > Lets do that in a separate bug though?
> 
> Chatted with Bryan some about this—sounds like there's no problem from him
> or I about turning on the Storage panel by default, and Style Editor/panel
> management stuff belongs in other bugs.
> 
> I won't clear his ni but I'm fairly certain it's safe to go ahead and
> implement the change. I'll ping him once he's on IRC.

Okay, i'll wait until you have confirmed it with bgrins.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #7)
> - I think the Style Editor should be turned /off/ by default. It's just such
> a weird tool and isn't as useful as Chrome's Resources panel, which is what
> everyone seems to wish it was.
> - I think having better panel management would put a lot of these questions
> to rest:
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%5Bdevtools-
> toolbar%5D&list_id=12972387 We get antsy turning on/off new tools because
> they're really space-greedy on the toolbar at the moment. Fang and I are
> working on this, but it seems annoying to block this bug on that.

Agreed that we should do these things but I don't think they block this work.  I want to remove the style editor but do that deliberately in a separate bug because we have a better solution for users.

Lets focus on the tab management soon as I agree that having easier access to the non-default panels will make choosing a small set (it should be a small set!) of default panels straightforward because customization is easy.

> Bryan, what do you think?

Yes, I think we're good to turn this on.  I've been working on a better process to talk about why something should be in the panel by default but I don't want to gate this on that work. 

Our competition has a very similar panel / functionality which means many users will expect this from us as well.  The panel is robust with enough features to call it ready for prime time.

Comment 12

[Brian Grinstead [:bgrins]]

What do you think about waiting until after the merge day to enable it, so there's a full release cycle in Nightly for bug reports / fixes before shipping this to our Dev Edition audience?  Merge day is Monday: https://wiki.mozilla.org/RapidRelease/Calendar.

Comment 13

[Patrick Brosset <:pbro>]

I filed bug 1267153 to discuss turning off the style editor by default.

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to (Unavailable until April 25) Brian Grinstead [:bgrins] from comment #12)
> What do you think about waiting until after the merge day to enable it, so
> there's a full release cycle in Nightly for bug reports / fixes before
> shipping this to our Dev Edition audience?  Merge day is Monday:
> https://wiki.mozilla.org/RapidRelease/Calendar.

Good idea, I don't think we're on a rush schedule here.

Comment 15

[Tim Nguyen :ntim]

It's been almost 3 months, we're past the merge date mentioned in comment 12.
The Storage inspector has been polished since thanks to Jarda's awesome work, with edge cases being fixed like bug 1268460 or bug 1283800. Deleting IDB databases is also supported now.

I think it's safe to enable now, what do you think ?

Comment 16

[Patrick Brosset <:pbro>]

We just enabled the memory panel by default yesterday (bug 1241298)!
So, now we're in the same situation again: do we have enough horizontal space to enable yet another one?

I feel sad that we can't surface easily our new tools, but I think the wise thing to do here would be to work on better panel/toolbar management first.
Namely, have that + button that allows you to easily add tabs, and X button to close them (or something similar).

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> It's been almost 3 months, we're past the merge date mentioned in comment 12.
> The Storage inspector has been polished since thanks to Jarda's awesome
> work, with edge cases being fixed like bug 1268460 or bug 1283800. Deleting
> IDB databases is also supported now.
> 
> I think it's safe to enable now, what do you think ?

There's still a lot of unresolved blockers on this meta, are you suggesting none of them should be blockers for enabling by default?

(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #16)
> We just enabled the memory panel by default yesterday (bug 1241298)!
> So, now we're in the same situation again: do we have enough horizontal
> space to enable yet another one?
> 
> I feel sad that we can't surface easily our new tools, but I think the wise
> thing to do here would be to work on better panel/toolbar management first.
> Namely, have that + button that allows you to easily add tabs, and X button
> to close them (or something similar).

Yeah, there's bug 1239859 for related work

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

I think this should be enabled by default and even if space starts getting tight I'd rather take that pain than appear to not have this functionality which other browsers ship by default.  I'm not convinced that this should block on bug 1239859 but I do think panel improvements are important work.

Lets review the existing blockers on this bug and decide based on that

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #18)
> I think this should be enabled by default and even if space starts getting
> tight I'd rather take that pain than appear to not have this functionality
> which other browsers ship by default.  I'm not convinced that this should
> block on bug 1239859 but I do think panel improvements are important work.
> 
> Lets review the existing blockers on this bug and decide based on that

I don't think any of the "blockers" are really blockers.

More than happy to make this on by default.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1194190-enable-storage-inspector-by-default.diff

In the interests of clarity I don't believe that any of the "blockers" on this bug should be blockers... the majority of them are enhancement requests.

Comment 21

[Brian Grinstead [:bgrins]]

Helen and Bryan, can you please do a UX / product review of the panel to help decide if we should turn it on by default now or if there's still things that we need to block on?

Comment 22

[Brian Grinstead [:bgrins]]

Attachment: clean-profile-with-storage-side-docked.png

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #18)
> I think this should be enabled by default and even if space starts getting
> tight I'd rather take that pain than appear to not have this functionality
> which other browsers ship by default.  I'm not convinced that this should
> block on bug 1239859 but I do think panel improvements are important work.

We need to also consider the UX with a side docked toolbox - here's a screenshot of a clean profile + storage inspector, the 'docking' controls and close button on the toolbox get pushed off the side of the window.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> Created attachment 8774801 [details]
> clean-profile-with-storage-side-docked.png
> 
> (In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #18)
> > I think this should be enabled by default and even if space starts getting
> > tight I'd rather take that pain than appear to not have this functionality
> > which other browsers ship by default.  I'm not convinced that this should
> > block on bug 1239859 but I do think panel improvements are important work.
> 
> We need to also consider the UX with a side docked toolbox - here's a
> screenshot of a clean profile + storage inspector, the 'docking' controls
> and close button on the toolbox get pushed off the side of the window.

We have a bug somewhere to add "+" to the right edge of the toolbars. Clicking the plus would show a dropdown of all hidden tools similar to Chromes.

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

They also use the right pointing guillemet, » (»), for overflowing toolbars.

Comment 25

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> They also use the right pointing guillemet, » (»), for overflowing
> toolbars.

This is my biggest concern. Should this be a blocker on this bug?

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #25)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> > They also use the right pointing guillemet, » (»), for overflowing
> > toolbars.
> 
> This is my biggest concern. Should this be a blocker on this bug?

I am not sure... my gut feeling is that if we enable the panel by default we would get a flood of users complaining about the crowded (tool|tab)bar.

On the other hand we have been waiting for that feature for a long time so maybe enabling this panel would up the pressure to finally get it done.

So I vote that we should enable the panel.

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

I'd like to get this enabled sooner than waiting on the fix for the tabs.  None of the blocking bugs here look like actual blockers to getting this enabled by default, more like nice fixes down the road.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #27)
> I'd like to get this enabled sooner than waiting on the fix for the tabs. 
> None of the blocking bugs here look like actual blockers to getting this
> enabled by default, more like nice fixes down the road.

I understand not wanting to wait for framework-level fixes, but does https://bugzilla.mozilla.org/attachment.cgi?id=8774801 look acceptable to you?  I don't want to ship that to our users.  Maybe there's some smaller work we can do around the toolbox UI without having to add the whole '+' button feature.

Comment 29

[Jarda Snajdr [:jsnajdr]]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> Maybe there's some smaller work we can do around the toolbox UI without having
> to add the whole '+' button feature.

If the part of the toolbar with the tool buttons is too big, make it scrollable? I mean exactly the same thing Firefox does when you have too many tabs open.

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #29)
> (In reply to Brian Grinstead [:bgrins] from comment #28)
> > Maybe there's some smaller work we can do around the toolbox UI without having
> > to add the whole '+' button feature.
> 
> If the part of the toolbar with the tool buttons is too big, make it
> scrollable? I mean exactly the same thing Firefox does when you have too
> many tabs open.

It's not a bad idea, although the amount of work required to do that is probably equal to what it would take to ship the UI we've been talking about in Bug 1239859 - and both will also have Bug 1245921 as a blocker

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> (In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #27)
> > I'd like to get this enabled sooner than waiting on the fix for the tabs. 
> > None of the blocking bugs here look like actual blockers to getting this
> > enabled by default, more like nice fixes down the road.
> 
> I understand not wanting to wait for framework-level fixes, but does
> https://bugzilla.mozilla.org/attachment.cgi?id=8774801 look acceptable to
> you?  I don't want to ship that to our users.  Maybe there's some smaller
> work we can do around the toolbox UI without having to add the whole '+'
> button feature.

No, that looks broken.  But we don't know how many users would experience that.  If we knew only 1% of our users would see that interface for some period of time before it is fixed; I think that's ok.  By shipping what we have now we essentially run the experiment to see how many people will be affected.  Which reminds me that we need bug 1205845 implemented so we have data on the docking preference.

Comment 32

[Jarda Snajdr [:jsnajdr]]

(In reply to Brian Grinstead [:bgrins] from comment #30)
> It's not a bad idea, although the amount of work required to do that is
> probably equal to what it would take to ship the UI we've been talking about
> in Bug 1239859 - and both will also have Bug 1245921 as a blocker

Another two ideas:

1. Increase the min-width of the side-docked toolbox so that it accommodates all the default buttons. It seems there are only about 20px between when the right side of the toolbar starts sliding away and when the min-width point is hit.

2. Compress the icons on the toolbar better, so that they fit into a smaller space. Several opportunities are visible on your screenshot:
- when the icon width goes below certain threshold, hide the text label. Will save a few pixels on the right side, and will make the icon centered.
- there seems to be a lot of space between the RDM and "split console" icon
- the spacing of the settings/dock-bottom/dock-separate could be compressed, too

This tweaking could make the spacing not only more compressed, but also more even and better-looking.

Comment 33

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@clarkbw: Let's make a decision on this now. Shall we enable it?

Comment 34

[Bryan Clark (DevTools PM) [:clarkbw]]

Its not about making the decision, it is the after effect. I'm looking for people who can work on the fixes we need.

@mike: Can you commit to getting bug 1239859 and bug 1238982 started?  If so, then lets enable it.

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Well, bug 1239859 is important but I don't think we should block on bug 1238982.

I am more than happy to commit to bug 1239859 and then get started on bug 1238982.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #35)
> I am more than happy to commit to bug 1239859 and then get started on bug
> 1238982.

Ok, lets turn this on then! :-D

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug 1239859 depends on the toolbox being converted to HTML in order for the toolbar to be converted to HTML.

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1194190 - Enable Storage Inspector by default

Review commit: https://reviewboard.mozilla.org/r/138780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138780/

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8867206 [details]
Bug 1194190 - Enable Storage Inspector by default

The code changes are very minimal and OK to me. I know we've been discussing about enabling the panel, but I'd like a final GO from Bryan on this.
I think the idea was to maybe disable the memory panel by default too. If so, we should do this in this bug too.

Comment 40

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 8867206 [details]
Bug 1194190 - Enable Storage Inspector by default

Just reviewed this with :pbro and it looks good.  

Looking at the available width I think we have enough room for the 1024 width, which is still a common screen size.

Comment 41

[Tom Tromey :tromey]

Comment on attachment 8867206 [details]
Bug 1194190 - Enable Storage Inspector by default

https://reviewboard.mozilla.org/r/138780/#review142702

Thanks, this looks good.  I'm giving my r+ after a brief discussion on irc:

<mikeratcliffe> Just a simple pref change... bclark and pbro both r+ssed it
		but pbro forgot to do the ship it thing in reviewboard.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69cc2c002805
Enable Storage Inspector by default r=tromey

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69cc2c002805

Comment 44

[Sajedul Islam]

I have reproduced this bug with Nightly 43.0a1 (2015-08-13) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 55.0a1 !

Build ID      20170519030205
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[testday-20170519]