Libor Spevak has posted comments on this change.
Change subject: engine: Fixes in non plugin console invocation
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/GetAttachmentServlet.java
Line 16: private static final long serialVersionUID = 8496520437603585173L;
Line 17:
Line 18: @Override
Line 19: protected void doPost(HttpServletRequest request,
HttpServletResponse response) throws ServletException, IOException {
Line 20: request.setCharacterEncoding("UTF-8");
Do some browsers set it or it is always null?
Line 21: String contentType = request.getParameter("contenttype");
Line 22: String fileName = request.getParameter("filename");
Line 23: Boolean cache =
Boolean.parseBoolean(request.getParameter("cache"));
Line 24: String encodingType = request.getParameter("encodingtype");
Line 19: protected void doPost(HttpServletRequest request,
HttpServletResponse response) throws ServletException, IOException {
Line 20: request.setCharacterEncoding("UTF-8");
Line 21: String contentType = request.getParameter("contenttype");
Line 22: String fileName = request.getParameter("filename");
Line 23: Boolean cache =
Boolean.parseBoolean(request.getParameter("cache"));
What if the request.getParameter returns null?
Line 24: String encodingType = request.getParameter("encodingtype");
Line 25: String content = request.getParameter("content");
Line 26:
Line 27: if (contentType != null) {
Line 23: Boolean cache =
Boolean.parseBoolean(request.getParameter("cache"));
Line 24: String encodingType = request.getParameter("encodingtype");
Line 25: String content = request.getParameter("content");
Line 26:
Line 27: if (contentType != null) {
Can be empty string?
Line 28: response.setContentType(contentType);
Line 29: }
Line 30:
Line 31: if (fileName == null) {
Line 27: if (contentType != null) {
Line 28: response.setContentType(contentType);
Line 29: }
Line 30:
Line 31: if (fileName == null) {
Can be empty string?
Line 32: fileName = "attachment";
Line 33: }
Line 34: response.setHeader("Content-Disposition", "attachment;
filename*='UTF-8'" +
URLEncoder.encode(StringEscapeUtils.unescapeHtml(fileName), "UTF-8"));
Line 35:
Line 30:
Line 31: if (fileName == null) {
Line 32: fileName = "attachment";
Line 33: }
Line 34: response.setHeader("Content-Disposition", "attachment;
filename*='UTF-8'" +
URLEncoder.encode(StringEscapeUtils.unescapeHtml(fileName), "UTF-8"));
Why this should be attachment? IE had big problems with this in some versions.
Do we need file name? IE takes just console.vv name.
Not sure about local caching of the content.
What about just header:
Content-type: application/octet-stream
or some application specific:
Content-type: application/....
Line 35:
Line 36: if (!cache) {
Line 37: response.setHeader("Cache-Control", "max-age=0,
must-revalidate"); //disable caching HTTP/1.1
Line 38: response.setHeader("Expires", "Sat, 26 Jul 1997 05:00:00
GMT"); //disable caching HTTP/1.0
Line 41: if (content == null) {
Line 42: return;
Line 43: }
Line 44:
Line 45: if ("binary".equals(encodingType)) {
Isn't it better to extract strings to static attributes?
Line 46:
response.getOutputStream().write(Base64.decodeBase64(content));
Line 47: } else if ("plain".equals(encodingType)) {
Line 48: content = StringEscapeUtils.unescapeHtml(content);
Line 49:
--
To view, visit http://gerrit.ovirt.org/13332
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7fe6fbf274a9ac215e8f6593cc000f778a431928
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Libor Spevak <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
