Issue 4661048523096064: Issue 2120 - Add support for animations. (Closed)
|
Created:
April 9, 2015, 8:38 p.m. by kzar Modified:
April 14, 2015, 5:15 p.m. CC:
saroyanm Visibility:
Public. |
DescriptionIssue 2120 - Add support for animations.
Depends on changes to conversion script https://github.com/kzar/adblockplus-website-converter/compare/2120-animations?expand=1
Patch Set 1 #
Total comments: 1
Patch Set 2 : Forgot to give localise_path filter a default return! #
Total comments: 16
Patch Set 3 : Improved inline_file filter and removed localise_file filter we no longer need. #Patch Set 4 : Use base64 module for the base64 encoding in the inline_file filter. #
Total comments: 8
Patch Set 5 : Improved license header for animation.js. #
Total comments: 8
Patch Set 6 : Addressed feedback. #
MessagesTotal messages: 15
Patch Set 1 http://codereview.adblockplus.org/4661048523096064/diff/5629499534213120/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/5629499534213120/stat... static/js/animation.js:8: var loadSuffix = ".xml"; I had to change loadPrefix and loadSuffix to get this to work. Otherwise the script is unmodified.
Patch Set 2 : Forgot to give localise_path filter a default return!
http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/inline_image.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:19: def inline_image(path): The code here isn't specific to images. So you might rather want to call it "path_to_data_url" or "inline_data" or something like that. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:20: mime_type = mimetypes.guess_type(path)[0] Note that mimetype.guess_type() might return None. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:21: with open(path, "rb") as f: Do we really want to resolve the path relative to the CWD? Also we probably should normalize path separators. Otherwise it won't work on Windows when using posix path seperators in the template and vice-versa. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/localise_path.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/localise_path.py:18: def localise_path(path, locale): Maybe it's just me. But I feel that British spelling ("-ise" instead "-ize") is quite uncommon in source code. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/localise_path.py:21: if os.path.isfile(full_path): Again, are you sure it's safe to resolve paths relative to the CWD? And what's about platform-dependent path separators? http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/stat... static/js/animation.js:1: if (typeof window.addEventListener != "undefined") { Is this our code? If yo the copyright and license disclaimer is missing. If not it's probably missing as well, plus there should be a link where this code originates from. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/temp... File templates/raw.tmpl (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/temp... templates/raw.tmpl:18: {{body|safe}} What is this template used for?
http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/inline_image.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:22: encoded = urllib.quote(f.read().encode("base64")) I'm not a fan of Python's str-to-str encodings. Calling .encode() on str or having .decode() return str is kinda unexpected. Moreover, does the base-64 encoding always append an unneeded newline. So I'd prefer to use the base64 module instead. output = StringIO() output.write("data:%s;base64," % mime_type) base64.encode(f, output) return output.getvalue()
Patch Set 3 : Improved inline_file filter and removed localise_file filter we no longer need. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/inline_image.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:19: def inline_image(path): On 2015/04/10 07:55:46, Sebastian Noack wrote: > The code here isn't specific to images. So you might rather want to call it > "path_to_data_url" or "inline_data" or something like that. Done. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:20: mime_type = mimetypes.guess_type(path)[0] On 2015/04/10 07:55:46, Sebastian Noack wrote: > Note that mimetype.guess_type() might return None. You're right, if we make this filter generic for all files perhaps it does matter. I now accept mime_type as an argument, if not specified try to guess it, if that fails default to "application/octet-stream". http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:21: with open(path, "rb") as f: On 2015/04/10 07:55:46, Sebastian Noack wrote: > Do we really want to resolve the path relative to the CWD? > > Also we probably should normalize path separators. Otherwise it won't work on > Windows when using posix path seperators in the template and vice-versa. Good point, I now make use of the cms.Source methods instead. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:22: encoded = urllib.quote(f.read().encode("base64")) On 2015/04/10 09:50:25, Sebastian Noack wrote: > I'm not a fan of Python's str-to-str encodings. Calling .encode() on str or > having .decode() return str is kinda unexpected. Moreover, does the base-64 > encoding always append an unneeded newline. So I'd prefer to use the base64 > module instead. > > output = StringIO() > output.write("data:%s;base64," % mime_type) > base64.encode(f, output) > return output.getvalue() I'm not sure this is possible now that I'm using the read_file method of cms.Source. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/stat... static/js/animation.js:1: if (typeof window.addEventListener != "undefined") { On 2015/04/10 07:55:46, Sebastian Noack wrote: > Is this our code? If yo the copyright and license disclaimer is missing. If not > it's probably missing as well, plus there should be a link where this code > originates from. We got to the bottom of it in the end. Done. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/temp... File templates/raw.tmpl (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/temp... templates/raw.tmpl:18: {{body|safe}} On 2015/04/10 07:55:46, Sebastian Noack wrote: > What is this template used for? The animation files use this as a way to avoid being wrapped by the default.tmpl template. (They are .tmpl files themselves which is required because we need to generate different version of them for the different locales.)
http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/inline_image.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:22: encoded = urllib.quote(f.read().encode("base64")) On 2015/04/11 13:32:01, kzar wrote: > On 2015/04/10 09:50:25, Sebastian Noack wrote: > > I'm not a fan of Python's str-to-str encodings. Calling .encode() on str or > > having .decode() return str is kinda unexpected. Moreover, does the base-64 > > encoding always append an unneeded newline. So I'd prefer to use the base64 > > module instead. > > > > output = StringIO() > > output.write("data:%s;base64," % mime_type) > > base64.encode(f, output) > > return output.getvalue() > > I'm not sure this is possible now that I'm using the read_file method of > cms.Source. Well, you could still use base64.b64encode().
Patch Set 4 : Use base64 module for the base64 encoding in the inline_file filter. http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... File filters/inline_image.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5741031244955648/filt... filters/inline_image.py:22: encoded = urllib.quote(f.read().encode("base64")) On 2015/04/11 13:56:41, Sebastian Noack wrote: > On 2015/04/11 13:32:01, kzar wrote: > > On 2015/04/10 09:50:25, Sebastian Noack wrote: > > > I'm not a fan of Python's str-to-str encodings. Calling .encode() on str or > > > having .decode() return str is kinda unexpected. Moreover, does the base-64 > > > encoding always append an unneeded newline. So I'd prefer to use the base64 > > > module instead. > > > > > > output = StringIO() > > > output.write("data:%s;base64," % mime_type) > > > base64.encode(f, output) > > > return output.getvalue() > > > > I'm not sure this is possible now that I'm using the read_file method of > > cms.Source. > > Well, you could still use base64.b64encode(). Done.
http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" I wonder whether we should make the mime_type argument mandatory for files the mimetype can not be guessed for, raising an exception? http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:29: if source.has_localizable_file(locale, path): Do we actually need this branch? Assuming we need this filter only for static files (like images), how about renaming it to inline_static_file, calling source.read_static(path) unconditionally? http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/stat... static/js/animation.js:5: * Adblock Plus is free software: you can redistribute it and/or modify Well, this script isn't part of Adblock Plus but of the Adblock Plus website. So shouldn't it rather be something like "This script is free software:"? How does the licence header of other JavaScript files of our websites look like?
Patch Set 5 : Improved license header for animation.js. # http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" On 2015/04/11 14:15:32, Sebastian Noack wrote: > I wonder whether we should make the mime_type argument mandatory for files the > mimetype can not be guessed for, raising an exception? I think defaulting to application/oclet-stream is enough, I don't mind throwing a ValueError exception instead though if you guys feel strongly about it. http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:29: if source.has_localizable_file(locale, path): On 2015/04/11 14:15:32, Sebastian Noack wrote: > Do we actually need this branch? Assuming we need this filter only for static > files (like images), how about renaming it to inline_static_file, calling > source.read_static(path) unconditionally? We do need this branch, the images used in the animations might be locale files like `locales/de/images/anim_example.png`, static files like `static/images/anim_example.png` or a locale file that hasn't been created for the user's locale meaning we should default to the English version like `locales/en/images/anim_example.png`. http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/stat... static/js/animation.js:5: * Adblock Plus is free software: you can redistribute it and/or modify On 2015/04/11 14:15:32, Sebastian Noack wrote: > Well, this script isn't part of Adblock Plus but of the Adblock Plus website. So > shouldn't it rather be something like "This script is free software:"? How does > the licence header of other JavaScript files of our websites look like? Done.
http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" Please create your own MimeTypes instance rather than using the default one: mimetypes.MimeTypes().guess_type(). The mimetypes module will try to parse various system files by default, meaning introducing inconsistent system-dependent behavior. http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:3: * Copyright (C) 2007-2015 Wladimir Palant Eyeo GmbH please, I've transferred all the copyrights. http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:5: * This script is free software: you can redistribute it and/or modify This is now different from all our other license headers, including other license headers in this repository. As said in other reviews, we should keep things consistent. If we want to change our license headers, we should decide on a general approach rather than deciding for each file individually.
http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" On 2015/04/12 17:01:45, kzar wrote: > On 2015/04/11 14:15:32, Sebastian Noack wrote: > > I wonder whether we should make the mime_type argument mandatory for files the > > mimetype can not be guessed for, raising an exception? > > I think defaulting to application/oclet-stream is enough, I don't mind throwing > a ValueError exception instead though if you guys feel strongly about it. Well, if the path doesn't contain a distinct file extension the developer must specify the mime type in the template. Otherwise the browser might not recognize the data correctly. By throwing an exception the developer gets remembered to do so.
Patch Set 6 : Addressed feedback. http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/5709068098338816/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" On 2015/04/13 08:12:58, Sebastian Noack wrote: > On 2015/04/12 17:01:45, kzar wrote: > > On 2015/04/11 14:15:32, Sebastian Noack wrote: > > > I wonder whether we should make the mime_type argument mandatory for files > the > > > mimetype can not be guessed for, raising an exception? > > > > I think defaulting to application/oclet-stream is enough, I don't mind > throwing > > a ValueError exception instead though if you guys feel strongly about it. > > Well, if the path doesn't contain a distinct file extension the developer must > specify the mime type in the template. Otherwise the browser might not recognize > the data correctly. By throwing an exception the developer gets remembered to do > so. Done. http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/filt... File filters/inline_file.py (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/filt... filters/inline_file.py:25: mime_type = mimetypes.guess_type(path)[0] or "application/octet-stream" On 2015/04/12 18:45:27, Wladimir Palant wrote: > Please create your own MimeTypes instance rather than using the default one: > mimetypes.MimeTypes().guess_type(). The mimetypes module will try to parse > various system files by default, meaning introducing inconsistent > system-dependent behavior. Done. http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:5: * This script is free software: you can redistribute it and/or modify On 2015/04/12 18:45:27, Wladimir Palant wrote: > This is now different from all our other license headers, including other > license headers in this repository. As said in other reviews, we should keep > things consistent. If we want to change our license headers, we should decide on > a general approach rather than deciding for each file individually. Done.
http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:5: * This script is free software: you can redistribute it and/or modify On 2015/04/12 18:45:27, Wladimir Palant wrote: > This is now different from all our other license headers, including other > license headers in this repository. As said in other reviews, we should keep > things consistent. If we want to change our license headers, we should decide on > a general approach rather than deciding for each file individually. Sure, but this file isn't part of Adblock Plus, but of the Adblock Plus website as I pointed out earlier in this review. So wouldn't it be meaningless to declare Adblock Plus as free software under the terms of the GPL here?
http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:5: * This script is free software: you can redistribute it and/or modify On 2015/04/13 09:41:50, Sebastian Noack wrote: > On 2015/04/12 18:45:27, Wladimir Palant wrote: > > This is now different from all our other license headers, including other > > license headers in this repository. As said in other reviews, we should keep > > things consistent. If we want to change our license headers, we should decide > on > > a general approach rather than deciding for each file individually. > > Sure, but this file isn't part of Adblock Plus, but of the Adblock Plus website > as I pointed out earlier in this review. So wouldn't it be meaningless to > declare Adblock Plus as free software under the terms of the GPL here? I just realized that we seem to use this header consistently in the code here. So LGTM. But I still think that this header doesn't make much sense, and should probably be changed for all website files.
LGTM http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... File static/js/animation.js (right): http://codereview.adblockplus.org/4661048523096064/diff/4848992307380224/stat... static/js/animation.js:5: * This script is free software: you can redistribute it and/or modify On 2015/04/13 10:01:52, Sebastian Noack wrote: > I just realized that we seem to use this header consistently in the code here. Yes, that was the point. Unfortunately, I'm not a lawyer and deciding what that header should look like isn't trivial. Too bad this isn't MPL 2.0 :-( 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
