D17233: Delay finishing download when state changes to "interrupted"

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, davidedmundson, fvogt.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION 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).
This patch remembers both state and error and then delays finishing the download slightly.

BUG: 385530

TEST PLAN
- Started a download in Firefox, canceled it, no longer got an "unknown error" notification
- Still works in Chrome

REPOSITORY
R856 Plasma Browser Integration

REVISION DETAIL
https://phabricator.kde.org/D17233

AFFECTED FILES
host/downloadjob.cpp
host/downloadjob.h

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik updated this revision to Diff 46459.
broulik added a comment.


- Update comment, originally had `singleShot(0` but that wasn't enough

REPOSITORY
R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17233?vs=46458&id=46459

REVISION DETAIL
https://phabricator.kde.org/D17233

AFFECTED FILES
host/downloadjob.cpp
host/downloadjob.h

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik updated this revision to Diff 46474.
broulik added a comment.


- Set no error explicitly when completing, just in case

REPOSITORY
R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17233?vs=46459&id=46474

REVISION DETAIL
https://phabricator.kde.org/D17233

AFFECTED FILES
host/downloadjob.cpp
host/downloadjob.h

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Fabian Vogt]

fvogt added a comment.


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.

REPOSITORY
R856 Plasma Browser Integration

REVISION DETAIL
https://phabricator.kde.org/D17233

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

broulik updated this revision to Diff 46536.
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.
broulik edited the test plan for this revision.

REPOSITORY
R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17233?vs=46474&id=46536

REVISION DETAIL
https://phabricator.kde.org/D17233

AFFECTED FILES
host/downloadjob.cpp

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Fabian Vogt]

fvogt added a comment.


LGTM otherwise.

INLINE COMMENTS

downloadjob.cpp:152
+ const QString error = it->toString();
+ if (!error.isEmpty()) {
if (error == QLatin1String("USER_CANCELED")

You could change this into

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

downloadjob.cpp:207
+
+ it = payload.constFind(QStringLiteral("state"));
+ if (it != end) {

Same here as for error above.

REPOSITORY
R856 Plasma Browser Integration

REVISION DETAIL
https://phabricator.kde.org/D17233

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Kai Uwe Broulik]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R856:90400518efcd: Only cancel job when an "error" is set and ignore "interrupted" (authored by broulik).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D17233?vs=46536&id=46609#toc

REPOSITORY
R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17233?vs=46536&id=46609

REVISION DETAIL
https://phabricator.kde.org/D17233

AFFECTED FILES
host/downloadjob.cpp

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart