[knewstuff] Do not leak ImageLoader on error
ClosedPublic

Authored by anthonyfieroni on Nov 14 2017, 5:47 AM.

Details

Summary
  1. Notify engine to not observe image preview any more on error codition
  2. Shutdown image loader, HTTP job simultaneously on error condition
Test Plan

We shouldn't restart image loader on error condition, no ?

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni created this revision.Nov 14 2017, 5:47 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 14 2017, 5:47 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
broulik added inline comments.
src/core/engine.cpp
536

You're copying entry into the lambda just for a debug.

src/core/imageloader_p.h
59

Why have this argument if you're not naming it and not using it?

src/core/engine.cpp
536

If / when someone want to read debug messages, they will be consistent.

src/core/imageloader_p.h
59

It can be an entry and/or type?

leinir added inline comments.Nov 15 2017, 11:49 AM
src/core/engine.cpp
536

A more descriptive error message would be good, but this is already an improvement over no error message at all :) As pointed out elsewhere, since you're explicitly passing the ImageLoader instance through the signal, might it not make sense to output the description of the error (which is available on the httpjob, which in turn is available through ImageHandler::job, so you could just add l to the capture to access that)?

anthonyfieroni marked an inline comment as done.
leinir added inline comments.Nov 15 2017, 12:46 PM
src/core/engine.cpp
536

You did add l to the capture, but unless i'm missing something super obvious... you're not using it ;) i meant to do something like add << l->job()->errorText() to the debug statement so we can see what the job says has gone wrong (which arguably is the more useful information here)...

Maybe also emit a signalError with an appropriate text (because that's how to tell the user about errors)... though i'm not entirely certain if we might not want to just fail quietly here.

src/core/engine.cpp
536

Job can be nullptr, so signal can be a text, it can capture entry by reference?

leinir added inline comments.Nov 15 2017, 1:05 PM
src/core/engine.cpp
536

i guess you could extend the signal further with the error text added in instead of capturing l in the lambda, yes

leinir accepted this revision.Nov 16 2017, 1:52 PM

Looks good to me :)

This revision is now accepted and ready to land.Nov 16 2017, 1:52 PM
This revision was automatically updated to reflect the committed changes.