On Friday 11 June 2010 23:58:31 Matthew Toseland wrote:
> On Friday 11 June 2010 14:43:09 Matthew Toseland wrote:
> > BEFORE RELEASING 1251, MUST FIX:
> > - Internal error on persistent download for mp3 with the check filter flag
> > enabled. This should be an unfilterable content error.
>
> My fault, fixed.
>
> > - WoT
> > - Freetalk
> > - Spider
> > - XMLSpider
> > - FlogHelper
>
> All done, thanks. Jars will be posted before 1251 is released.
>
> > - ContentFilter charset detection code MUST use readFully, or a loop
> > achieving similar goals. read() doesn't read the full buffer.
>
> This is STILL not fixed. We use readFully() but we only read up to
> available(). We should read the full buffer size specified by the
> CharsetExtractor, up to the limit of the *SIZE OF THE BUCKET*. available() is
> not very helpful here IMHO.
More issues that it would be good to fix before releasing a build with this
code. This is all usability stuff which IMHO will make a difference to
end-users' frustration levels (of course, filters for popular types, and
straight-through decompression-to-filtering, will also help with usability; if
any of these are big jobs we can postpone them but we must not forget them):
1) Error: Content sanitization failed: Unknown and potentially dangerous
content type: application/octet-stream
(The title of the warning page). This is redundant, too many words. IMHO we
should just show Error: Unknown and ...
2) When downloading a file to disk, with an unrecognised MIME type, with
filtering enabled, it now fails the download with a message explaining that it
is not supported; this message is visible on the queue page. However, it is a
*really* *long* message. All of the other failures are short messages like "New
URI" or "Invalid URI: Invalid crypto algorithm". Okay, those aren't terribly
helpful, but IMHO it is useful for it to be short, maybe with a link to a
longer explanation.
I'm not sure exactly how to handle this. IIRC GetFailed (the FCP mechanism
which underlies the queue page) has an ExtraDescription field we could use for
the long description. Then we just need a short description for the error
itself? I think we already use this, assembling the error from the short
description and then the long description after a colon ... this probably
happens in FetchException ... hmmm. Worth thinking about anyway, if you can
come up with a reasonably quick solution. I think we have short description and
long description for specific error codes, this may be relevant?
Of course the real solution is to write filters for the most popular content
types e.g. this is an MP3. :)
3) There should be some way to retry the download with filtering turned off.
Probably this should take the form of another button in the first column, or a
check box ([ ] Turn off filter). In future when the queue page has been cleaned
up, the first column will be just checkboxes, and the actions will be at the
bottom, at which point we can have a checkbox at the bottom; but that is way
out of scope for your Summer of Code project and I'm hoping Bombe will get
around to it eventually. But the lack of any obvious ability to retry with
filtering disabled, given that it was caused by an unsupported MIME type, is
IMHO a serious usability issue which will cause a lot of end-user frustration.
4) The filter flag is enabled by default for download to disk from within
fproxy. Even if we know ahead of time that the MIME type is one we don't
support, and even if security levels are all set to LOW. This will cause people
to have the above frustration.
IMHO:
- If the page the download button is on is itself a MIME type warning, the
filtered checkbox should default to OFF. IIRC if we have a nontrivial MIME type
it cannot change later on because ClientMetadata.mergeNoOverwrite() does not
allow us to change a nontrivial MIME type; so as long as the MIME type is not
empty and not application/octet-stream (which is the same thing iirc), if we
show the user the warning and they accept it, that is the MIME type that we
will have at the end of the fetch. However, USKs may throw a spanner in the
works here... so it may still be useful to be able to record that we've shown
the user the warning for type T and not to fail *IF* the output is of that type.
- If the network security level and physical security level are set to LOW, the
filtered checkbox should default to OFF.
- Ideally, if we know both the size and the MIME type, we should combine the
two warnings into a single page. That's long term, file a bug and forget about
it. A shorter term solution might be to show the MIME type warning before we
show the size warning, once we are fairly sure we know the MIME type (i.e. we
detect a non-null non-application/octet-stream type in the ExpectedMIME event
which fproxy picks up).
> >
> > Minor stuff:
> >
> > 15b6283e017a26dec96f35820028b84992b5260f
> > - should pass the overridden mime type