1591058 - Debugger pretty print button the first time does not immediately open pretty tab

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[:Harald Kirschner :digitarald]

Attachment: fvsch's footer-pretty-print-no-feedback.mov

What were you doing?

1. Open Debugger on https://debugger-pretty-print-events-1532240.glitch.me/
2. Open the minified file
3. Hit pretty print button

What happened?

Button doesn't change and remains hoverable. If the file is visible in sources tree, its file icon does change to {}.
Only after a second click it changes to the processing spinner.

What should have happened?

Button immidiatly changes to the processing state.

Profile: https://perfht.ml/2Wce0Jz
First click: https://perfht.ml/31FPOjV

Via fvsch: So the farthest I got with mozregression is this pushlog for 2019-10-17:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cdcbac30666276f484747875e430ab0a74f123fb&tochange=c260b3893967535faf023573fdd429fc48738778 . Bug 1578752 and bug 1588637 touch pretty-print but are not obviously causing this issue.

Comment 1

[David Walsh :davidwalsh]

@zhaogang Would you be able to look at this? Your patch:

https://hg.mozilla.org/mozilla-central/rev/0762a641fec7

Fixed the column problem (yay!) but introduced an issue where large files require multiple click to be pretty printed. I believe the issue comes from this change:

-    await dispatch(selectSource(cx, prettySource.id));
+
+    const actors = getSourceActorsForSource(getState(), sourceId);
+    const content = getSourceContent(getState(), sourceId);
+    if (!content || !isFulfilled(content)) {
+      throw new Error("Cannot pretty-print a file that has not loaded");
+    }
+    await prettyPrintSource(sourceMaps, source, content.value, actors);
+    await dispatch(loadSourceText({ cx, source: prettySource }));

This doesn't happen on small files -- only large files appear to require a second click.

Comment 2

[Florens Verschelde :fvsch]

This doesn't happen on small files -- only large files appear to require a second click.

I think all files may be affected the same way, but for small files the pretty-printing is close to instant so it doesn't matter much that the loader icon is not showing up in the footer.

Comment 3

[David Walsh :davidwalsh]

Detail update: Only one click is required to get the pretty printed file open, but the wait time and lack of feedback is definitely a problem.

Comment 4

[Zhao Gang]

Seems like that previously used selectSource() method contains some hacky code/action to trigger the loader's state change. Maybe cutting that part into the new procedure can fix it. Will give it a look later today.

Btw, the reason why selectSource() is bad is because it discards the current select location.

Comment 5

[David Walsh :davidwalsh]

I've tried removing your new code and simply adding await selectPrettyLocation(cx, prettySource);, which seems as though it's supposed to translate the location, but in the case of the original bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1578752) STR, it highlights the same location:

Line 17, column 118.

Comment 6

[Zhao Gang]

@davidwalsh(In reply to David Walsh :davidwalsh from comment #5)

I've tried removing your new code and simply adding await selectPrettyLocation(cx, prettySource);, which seems as though it's supposed to translate the location, but in the case of the original bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1578752) STR, it highlights the same location:

Line 17, column 118.

That's a good direction for refactoring! It doesn't work because selectPrettyLocation() will fall back to selectSource() if the source is not prettified yet (which is true) - previously it's the selectSource() that did the prettyPrint job...

Comment 7

[Zhao Gang]

Attachment: Bug 1591058 - Change the branching condition of pretty print loader to reflect the new loading procedure. r=davidwalsh

The loader didn't show up because togglePrettyPrint no longer pre-selects the new source, falsifying its condition.

The new guarding condition is a predicate on the pretty source itself, and this predicate works very well with existing code.

This change adds a new get call inside the footer component, so the footer is expected to slow down a little bit. Further optimization of large-file prettyprint is definitely required anyway.

An alternative fix would be to add/modify action/reducer to achieve context-preserving pre-select, which I think is too task-specific and hard to maintain.

Note that memorizing location before selectSource then restoring by selectSpecificLocation() can fix, but will create a user-visible gap of jumping to the correct location, thus not used in this patch.

Comment 8

[Zhao Gang]

Attachment: Bug 1591058 - Put the memorization of selected location into togglePrettyPrint() . r=davidwalsh

The loader didn't show up because togglePrettyPrint no longer pre-selects the new source, falsifying its condition.

The new guarding condition is a predicate on the pretty source itself, and this predicate works very well with existing code.

This change adds a new get call inside the footer component, so the footer is expected to slow down a little bit.

An alternative fix would be to add/modify action/reducer to achieve context-preserving pre-select, which I think is too task-specific and hard to maintain.

Note that memorizing location before selectSource then restoring by selectSpecificLocation() can fix, but will create a user-visible gap of jumping to the correct location, thus not used in this patch.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2afe0433e87
Put the memorization of selected location into togglePrettyPrint() . r=davidwalsh

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f2afe0433e87

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for Beta uplift if you're comfortable doing so.

Comment 12

[David Walsh :davidwalsh]

Comment on attachment 9104146 [details]
Bug 1591058 - Put the memorization of selected location into togglePrettyPrint() . r=davidwalsh

Beta/Release Uplift Approval Request

• User impact if declined: User will see no feedback when trying to
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's not risky because it doesn't break the tool but it dramatically degrades user experience if not approved.
• String changes made/needed:

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9104146 [details]
Bug 1591058 - Put the memorization of selected location into togglePrettyPrint() . r=davidwalsh

Patch has tests, fixes a visible bug in the debugger and we are still in the early betas phase, uplift approved for 71 beta 6, thanks.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f2eba101928b

Comment 15

[Arthur Iakab [arthur_iakab]]

Backed out changeset f2eba101928b (bug 1591058) for causing devtools failures on browser_dbg-pretty-print-flow.js. a=backout

Backout revision https://hg.mozilla.org/releases/mozilla-beta/rev/a59b72082dd6ace7c5021af75e5f80f795e4eff9

Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&revision=85d1ea939621193269f1fe6d3a65ca817d5b25a3&searchStr=devtools

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=273879185&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=273881406&repo=mozilla-beta

Zaho can you please take a look?

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

David, can you guide how to get a patch which passes tests on beta, please?

Comment 17

[David Walsh :davidwalsh]

:aryx: The patch was updated here (https://phabricator.services.mozilla.com/D50568) and the try run passes -- could you try that? The try run passes.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0d207c20c709