mozilla-mobile/android-components

Allow for notifying consumers on unsuccessful Save to PDF requests

jonalmeida opened this issue · 4 comments

jonalmeida commented

In at least two cases in in the GeckEngine.requestPdfToDownload, we can unsuccessfully return a PDF. Our current error handling in #12277 is to silently fail, but this might not be helpful to users if they are expecting to see a PDF.

We should come up with a solution, possibly similar to the Download State one, for handling the error cases so that we can notify consumers.

cc: @Amejia481 , @ohall-m

┆Issue is synchronized with this Jira Task

ohall-m commented

Adding more info from the GV change in bug 1784554 - the new GV change will now return the input stream as early as possible. Because of this, if a PDF generation failure occurs late, the InputStream will now send an IO error. Before, the GeckoResult that held the input stream would be the most important exception. The GeckoResult can still have an exception too with this change, if the failure happens early enough.

I checked that this new IO exception is covered on some level before landing in GV, I believe here. The prior GV behavior would have also had IO exceptions as a possibility, but more unexpected. Not sure how this failure looks on the UI side.

Error States:

  • GeckoResult Exceptions - permalinked before landing 1784554, but these exceptions stay the same
  • InputStream IO Exception
  • 🎉2
jonalmeida commented

  • GeckoResult Exceptions - permalinked before landing 1784554, but these exceptions stay the same

  • InputStream IO Exception

@ohall-m doesn't this mean that GeckoSession.saveAsPdf() can now throw exceptions outside of the GeckoResult return value? If so, we should annotate the Java API so mention that it now throws an exception. IIRC, that will help consumers catch that this needs to be handled with build warnings.

ohall-m commented

Good point! GeckoInputStream inherits from InputStream and uses the throws keyword to mark the exception, so the compiler will require handling the IOException. The IOException can only happen when reading/writing. Did a quick check to make sure. I don't know if there is a way right now to directly annotate the wrapped InputStream's read/write exception in the API too, since it isn't the return value.