Only cancel job when an "error" is set and ignore "interrupted"
ClosedPublic

Authored by broulik on Nov 29 2018, 11:15 AM.

Details

Summary

In Chrome we get state change to "interrupted" and the error in unison whereas Firefox first signals a state change and then updates the error later.
Previously, we would finish the job as soon as it was interrupted leading to a "unknown error" any time a download aborted (even if explicitly canceled by the user).
According to documentation the "error" is set in case of an error, so we'll want to cancel the job in this case.

BUG: 385530

Test Plan
  • Started a download in Firefox, canceled it, no longer got an "unknown error" notification, finish and retry still works
  • Still works in Chrome

Diff Detail

Repository
R856 Plasma Browser Integration
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Nov 29 2018, 11:15 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 29 2018, 11:15 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Nov 29 2018, 11:15 AM
broulik updated this revision to Diff 46459.
  • Update comment, originally had singleShot(0 but that wasn't enough
broulik updated this revision to Diff 46474.Nov 29 2018, 2:31 PM
  • Set no error explicitly when completing, just in case
fvogt added a comment.Nov 29 2018, 6:16 PM

It might work as well to just ignore the "interrupted" state completely and only react when error becomes set.

The docs say that error is undefined if no error occured (It's actually null here, so it's already a doc violation...), so if it's set that's a better trigger to terminate the KJob AFAICT.

The way I tested it here (a php script with a sleep inside) the download doesn't actually terminate until the sleep runs out, so I got the "unknown error" message anyway.

broulik updated this revision to Diff 46536.Nov 30 2018, 9:02 AM
broulik retitled this revision from Delay finishing download when state changes to "interrupted" to Only cancel job when an "error" is set and ignore "interrupted".
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
fvogt added a comment.Nov 30 2018, 7:11 PM

LGTM otherwise.

host/downloadjob.cpp
159

You could change this into

QString error = payload.value(QStringLiteral("error")).toString();
if(!error.isEmpty())
160

Same here as for error above.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 1 2018, 11:50 AM
This revision was automatically updated to reflect the committed changes.