Closed Bug 1534808 Opened 9 months ago Closed 7 months ago

Pretty print context menu item shouldn't display for non-JS files

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davidwalsh, Assigned: davidwalsh)

Details

Attachments

(1 file, 1 obsolete file)

We re-implemented the shim but naively -- we shouldn't show the pretty print context menu item for tabs if the source isn't JS.

Assignee: nobody → dwalsh
Priority: -- → P3
Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33f142760b69
Don't show pretty print option for non-JS sources r=jlast

David, will this handle webpack:///webpack/bootstrap or webpack:///public/locales%20lazy%20^\.\/.*\/send\.ftl$%20namespace%20object in https://send.firefox.com/ , which are JS file but do not have the extension. I don't have examples of minified files; but build steps are easy to mess up.

The main concern is that the context menu is the last resort for Debugger's pretty print detection missing prettifiable files; and this patch depends on JS being properly detected – which it doesn't seem for the files above.

Flags: needinfo?(dwalsh)
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

:digitarald You're right -- it doesn't do those URLs. My understanding is we wanted this shim for cases where sourcemaps failed. Would you like a revert of this?

Flags: needinfo?(dwalsh) → needinfo?(hkirschner)

My understanding is we wanted this shim for cases where sourcemaps failed. Would you like a revert of this?

How far do you trust that source maps are always pointing to the pretty printed results?

Maybe I am missing a reason that this should really be hidden. The main risk here is that this is a last-resort power-user option from a secondary interface; which would cause any harm if it doesn't work on some files.

Flags: needinfo?(hkirschner)

I hid this after testing https://davidwalsh.name :

  • We show an (index)
  • I right-click the tab, pretty print, and then see "Loading....." forever

... because we don't pretty-print HTML. Or XML.

Chrome auto-detects JS in the bootstrap case, which might be interesting investigating.

I talked to Harald and he doesn't wan this.

To add some context for posterity: Anything that could potentially impact productivity for the web compat team needs to be considered carefully. Showing/hiding the pretty-print button correctly is a long-standing issue, making the context menu a last-resort option to force pretty printing.

Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cbcf5b79aa44
Backed out changeset 33f142760b69 - Don't check for JavaScript before showing debugger source tab title r=jlast
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 67 → ---

ni? David, given that bug 1539179 appeared in DevEdition; could it be that the patch made it into 67 but the backout didn't?

Flags: needinfo?(dwalsh)

Removing that JS check is in our uplift list: https://bugzilla.mozilla.org/show_bug.cgi?id=1539813

Flags: needinfo?(dwalsh)

I believe this is fixed.

Status: REOPENED → RESOLVED
Closed: 9 months ago7 months ago
Resolution: --- → FIXED
Attachment #9050678 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.