Tune verbosity and content of the non-inline comment on review-helper

[padenot]

Opening this issue so others can comment, potentially w/ examples.

[gijsk]

https://phabricator.services.mozilla.com/D295935#10329410

"This bug addresses an issue where [...]"

Said every summary ever. This should be removed.

Plus, it's followed by 2 subsentences (in commas and then in parens, and using "e.g." when it should be "i.e." - it's not an example, it's what happens in the UI, which is also redundant because it is already described in more general turns). It's really hard to read.

It should just lead with a very brief super high level summary:

Blocked files removed by the user are now marked as deleted in all downloads UI.

Also, 6 bullets, my goodness. And they just regurgitate what the patch is doing, but in prose.

Here is what I would have liked to see, following the high-level summary (based on bug comments, and high level patch).

The bug discussion started from a discrepancy between the downloads panel and the library. Blocked files are removed from history in one (but left on disk) and deleted from disk (but left in history) in the other.
We don't want to remove the item from download history entirely because especially in the library that feels too aggressive.
So instead, now we keep the item in history, delete from disk, and show "File discarded (Blocked)" for it in the UI.

While here, tidy glean user interaction recording into confirmBlock and make head.js's task_resetState slightly more resilient and correct, as finalization may also attempt to remove the file.

Also, instead of summarizing the test(s), I would prefer if it didn't remark on the test side of things at all unless to highlight a lack of coverage without a good reason.

[gijsk]

Also, we could omit (most of) the comment entirely if we think the extended commit msg already covers this ground.

But on the whole, if just summarizing, it should be much more succinct, or it should not bother summarizing and only give actual review feedback.

As a reviewer I'm looking to understand why some of these changes were made. It tries to cater to this but it's often circuitous, e.g.:

"In DownloadCore.sys.mjs, the confirmBlock() method now sets this.deleted = true after successfully removing the blocked data via the saver."

This is an overly precise description of a 1-line change (20 characters, not counting the indent - the prosification is 7 times as long!). The obvious question is "but why", and the LLM tries to answer but it's disappointing:

"This ensures the download object properly records that its file data has been removed."

Well yes, that is what setting a property called deleted to true would do. But calling it "properly" doesn't really add much. I am still left wondering "but whyyyyyy". And is this the right place to do it? The right condition?