Bug 19208 – Inspector should remember resources sorting set by the user

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Anthony Ricaud]

It's a user preference, it should be remembered over instances.

[Alexander Pavlov (apavlov)]

Created attachment 62317 [details]
[PATCH] Suggested solution

[Joseph Pecoraro]

Comment on attachment 62317 [details]
[PATCH] Suggested solution

Looks good to me. The only thing I have to add is it would be nice to
add a comment or a line in the ChangeLog saying that you store both
the sorting options for the Resources Panel in a single setting, and
its benefit (for Remote debugging its just 1 message to get both settings).

Also, I would argue it might be easier or more future proof to store
settings as JSON strings, instead of strings that need need to be
manually parsed. This case is rather simple, but I think for just a few
more bytes its much more self descriptive and easier to understand.
Consider writing another patch if you agree.

Have you thought about saving other similar settings? Maybe bugs should
be opened up for the following as well, if they aren't already saved:

  - Profiles "Bottom up" / "Top Down".
  - Console filters
  - Resource filters

[Alexander Pavlov (apavlov)]

Created attachment 62402 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Created attachment 62406 [details]
[PATCH] ChangeLog message augmented

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 62317 [details])
> Looks good to me. The only thing I have to add is it would be nice to
> add a comment or a line in the ChangeLog saying that you store both
> the sorting options for the Resources Panel in a single setting, and
> its benefit (for Remote debugging its just 1 message to get both settings).

Done.

> Also, I would argue it might be easier or more future proof to store
> settings as JSON strings, instead of strings that need need to be
> manually parsed. This case is rather simple, but I think for just a few
> more bytes its much more self descriptive and easier to understand.
> Consider writing another patch if you agree.

Discussed on #webkit-inspector - we agreed on sticking to a JS object with 2 properties.

> Have you thought about saving other similar settings? Maybe bugs should
> be opened up for the following as well, if they aren't already saved:
> 
>   - Profiles "Bottom up" / "Top Down".
>   - Console filters
>   - Resource filters

Guess I'll think of coming up with a universal solution for these cases at once.

[Joseph Pecoraro]

Comment on attachment 62406 [details]
[PATCH] ChangeLog message augmented

Thanks!

> +    WebInspector.applicationSettings.resourcesSortOptions = {timeOption: this._selectedOptionNameForGraph(this.timeGraphItem), sizeOption: this._selectedOptionNameForGraph(this.sizeGraphItem)};
> +    WebInspector.applicationSettings.installSetting("resourcesSortOptions", "resources-sort-options", {timeOption: "responseTime", sizeOption: "transferSize"});

I thought our style was to put spaces around the braces
for JavaScript object literals. So instead of:

  {a:1, b:1}

I think we normally go with:

  { a:1, b:1 }

But I just did a search and found we do a lot of both,
so I guess it doesn't matter either way. But it would
be good to settle on a style for literals.

[Joseph Pecoraro]

> > Have you thought about saving other similar settings? Maybe bugs should
> > be opened up for the following as well, if they aren't already saved:
> > 
> >   - Profiles "Bottom up" / "Top Down".
> >   - Console filters
> >   - Resource filters
> 
> Guess I'll think of coming up with a universal solution for these cases at once.

Could you file a Bugzilla bug for this?

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> > > Have you thought about saving other similar settings? Maybe bugs should
> > > be opened up for the following as well, if they aren't already saved:
> > > 
> > >   - Profiles "Bottom up" / "Top Down".
> > >   - Console filters
> > >   - Resource filters
> > 
> > Guess I'll think of coming up with a universal solution for these cases at once.
> 
> Could you file a Bugzilla bug for this?

Sure, just need to check with the folks which of these (and others) have been/are planned to be implemented when I'm back in the office.

[Eric Seidel (no email)]

Comment on attachment 62317 [details]
[PATCH] Suggested solution

Cleared Joseph Pecoraro's review+ from obsolete attachment 62317 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/ResourcesPanel.js
        M       WebCore/inspector/front-end/Settings.js
Committed r64037