986838 - Add-on SDK modules should be listed after the add-on's own modules

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Dave Townsend [:mossop]]

The grouping of sources in the add-on debugger is alphabetical right now, so the "Add-on SDK" group always appears before the add-on's own modules. We should make the SDK group appear towards the end. If we can collapse groups in the future we should default it to collapsed too.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This allows us to define a set of known groups for URI and sort them at the end of the list.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch

Realised that getSourceGroup and getSourceLabel don't need splitting the -> urls.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8397944 [details] [diff] [review]
patch

Review of attachment 8397944 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/debugger-panes.js
@@ +11,5 @@
>  const INDENT_COUNT_THRESHOLD = 5; // percentage
>  const CHARACTER_LIMIT = 250; // line character limit
>  
> +// Maps known URLs to friends source group names
> +const SOURCE_GROUPS = {

Maybe we could name this better? I understand what it does from reading the code, but it seems like it isn't just SOURCE_GROUPS it's SPECIAL_SOURCE_GROUPS (where "special" could be improved upon; KNOWN_SOURCE_GROUPS?).

Also, the comment doesn't make sense to me wrt "friends".

@@ +1571,5 @@
> +    let sourceLabel = null;
> +
> +    for (let name of Object.keys(SOURCE_GROUPS)) {
> +      if (aUrl.startsWith(SOURCE_GROUPS[name]))
> +        sourceLabel = aUrl.substring(SOURCE_GROUPS[name].length);

Style nit: in devtools code we always wrap an if's consequent in brackets.

@@ +1575,5 @@
> +        sourceLabel = aUrl.substring(SOURCE_GROUPS[name].length);
> +    }
> +
> +    if (!sourceLabel)
> +      sourceLabel = this.trimUrl(aUrl);

ditto

@@ +1608,5 @@
>      let groupLabel = uri.prePath;
> +
> +    for (let name of Object.keys(SOURCE_GROUPS)) {
> +      if (aUrl.startsWith(SOURCE_GROUPS[name]))
> +        groupLabel = name;

ditto

::: browser/devtools/debugger/test/browser_dbg_addon-sources.js
@@ +91,5 @@
> +    let groups = gDebugger.document.querySelectorAll(".side-menu-widget-group-title .name");
> +    is(groups[0].value, "jar:", "Add-on bootstrap should be the first group");
> +    is(groups[1].value, "resource://jid1-ami3akps3baaeg-at-jetpack", "Add-on code should be the second group");
> +    is(groups[2].value, "Add-on SDK", "Add-on SDK should be the third group");
> +    is(groups.length, 3, "Should be only two groups.");

"Should only be *three* groups"

Comment 4

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/51b4d17d0513

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51b4d17d0513