Issue 11544056: Prepared buildtools for Safari (Closed)
|
Created:
Sept. 4, 2013, 8:03 a.m. by Sebastian Noack Modified:
Oct. 31, 2013, 4:29 p.m. Visibility:
Public. |
DescriptionPrepared buildtools for Safari
Patch Set 1 #Patch Set 2 : Prepared buildtools for Safari #Patch Set 3 : #
Total comments: 81
Patch Set 4 : #Patch Set 5 : #
Total comments: 11
Patch Set 6 : #Patch Set 7 : Added ext directory to includedFiles for chrome and extensions based on it. #Patch Set 8 : Made "Chrome" be not replaced with "Safari" in the warning on the first run page #
Total comments: 11
Patch Set 9 : Addressed comments #Patch Set 10 : Made first run page always generated, fix absolute URLs for Safari during build, introduced browser… #
Total comments: 1
Patch Set 11 : Fixed regex for fixing absolute URLs in Safari #Patch Set 12 : #
MessagesTotal messages: 33
Beside some code cleanup, I added support for generating signed xar archives. In order to generate a signed Safari extension, you need an apple developer certificate, its private key, and its certificate chain. And you must have xar 1.6.1 (unofficial version, with signing support) installed. In order to build a signed safari extension call build.py with following options: ./build.py -t safari build \ --key=<PEM-encoded, not encrypted private key> --cert=<developer certificate> \ --cert=<Apple's certificate used to issue the developer certificate> \ --cert=<Apple's root CA>
http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl File Info.plist.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode5 Info.plist.tmpl:5: <key>CFBundleDisplayName</key> Nit: We generally use two spaces for indentation, no tabs. http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode8 Info.plist.tmpl:8: <string>{{ identifier }}</string> I suggest generating the identifier automatically rather than specifying it explicitly: org.adblockplus.{{basename}} http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode12 Info.plist.tmpl:12: <string>{{ version }}</string> According to Apple documentation we shouldn't be using the full version string (including the build number) here. It should rather be metadata.get("general", "version") here. http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode67 Info.plist.tmpl:67: {%- for key, value in props.iteritems() %} Having Jinja tags unindented makes the whole thing pretty hard to follow, particularly for nested loops. How about using the same indentation approach as in install.rdf.tmpl? http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode137 Info.plist.tmpl:137: {%- endif %} There should always be an update URL for development builds, it also overwrites the "regular" update URL. You should do it similarly to install.rdf.tmpl or manifest.json.tmpl: {- if not releaseBuild or updateURL %} <key>Update Manifest URL</key> <string> {%- if not releaseBuild -%} https://adblockplus.org/devbuilds/ {{- metadata.get('general', 'basename') -}} /updates.plist {%- else -%} {{ updateURL }} {%- endif -%} </string> {%- endif %} http://codereview.adblockplus.org/11544056/diff/10001/build.py File build.py (right): http://codereview.adblockplus.org/11544056/diff/10001/build.py#newcode193 build.py:193: certs.append(value) I suggest reusing the approach from the Gecko packager instead of introducing a new parameter. The signing key file for Gecko builds starts with the private key followed by the certificates. M2Crypto can still load the key directly, look for keyData in packagerGecko.py for the certificate loading code. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:22: import PIL.ImageMath Please don't require these modules at the top level - building the Chrome extension should be possible without it. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:316: def convert(self, params, files): Does this need to be an object? You are never really using the self variable, this looks like abuse of object oriented programming. How about having a function convertImage with multiple nested functions and a filters dictionary at the top? filters = {"blend": filter_blend, ...} Note that this entire functionality is only marginally related to the packager. I am inclined to suggest moving all the generic functionality (meaning image construction/saving and filters) into a separate module while only keeping the real packager-related code here. Then again, isn't blend the only filter we actually need? I would vote against having code around just because "we might need it". This introduces unnecessary complexity and leads to code that is broken because it isn't used anywhere. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:319: image = PIL.Image.open(steps.pop(0)) This path should be relative to the metadata file it is specified in: baseDir = os.path.dirname(metadata.option_source('convert_img', filename)) parts = steps.pop(0).split('/') image = PIL.Image.open(os.path.join(baseDir, *parts)) Note that you also need to pass in baseDir to the filter functions. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:333: overlay = PIL.Image.open(args[0]) This file name should be resolved relative to the base dir. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:335: if image.mode != overlay.mode or image.mode == 'P': Please comment here that 'P' means "palette" (which is the only mode where two images can have the same mode yet not support the same colors). That's not really obvious. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:337: image = image.convert('RGBA') Style nit: we don't usually align equal signs and such (no real readability improvement but makes changing code more complicated). http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:350: raise TypeError How about: raise TypeError("Wrong number of blend filter parameters") That will make debugging typos a bit easier. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:352: return PIL.Image.blend(image, overlay, float(args[-1])) I suggest processing the opacity parameter when you handle the other parameters above. E.g.: overlay = PIL.Image.open(args[0]) opacity = float(args[1]) This will make this more obvious and won't cause errors if somebody adds a third way to call this filter and forgets to put opacity last. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:370: ) Won't image.convert("LA", dither=None) do the right thing? Note that your approach is wrong - a proper grayscale conversion doesn't treat all colors equally, the correct factors are actually 0.299, 0.587, 0.144. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:377: def filter_colorToAlpha(self, image, *color): I cannot really imagine what we would use this for. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:23: from collections import OrderedDict OrderedDict isn't actually used here. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:28: def createPlist(params, files): Please consider renaming this into createManifest for consistency with the other packagers - the format isn't really important, it's the package manifest. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:29: template = getTemplate('Info.plist.tmpl') Please use autoEscape=True here, without automatic escaping you have to think twice about what you are inserting into the template. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:31: catalog = json.loads(files['_locales/en_US/messages.json']) Please import defaultLocale from packagerChrome and use it here. Also, "catalog" doesn't really describe what this variable is about. Maybe "locale_data" or something like this? http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:56: d[key] = '<%s>%s</%s>' % (type, val, type) Please don't generate markup here. This should do: try: val = float(val) except ValueErrro: pass d[key] = val The template should check isinstance(val, float) then and generate the correct markup. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:58: return rv Please consider not abbreviating variable names: val => value, opt => option, rv => result http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:70: for perm in re.split(r'\s+', metadata.get('general', 'permissions')): Won't metadata.get('general', 'permissions').split() be sufficient here? http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:74: continue How about reusing Chrome's format? Meaning that "http://*/*", "https://*/*" is used instead of "<all_urls>". That is already handled by the code below. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue If you really want this to be correct then it should be: if url.hostname == '*': allowAllDomains = True continue if url.hostname.startswith('*.'): domain = url.hostname[2:] else: domain = url.hostname allowedDomains.add(domain) Note that this covers all the cases allowed by http://developer.chrome.com/extensions/match_patterns.html (host name can be '*', '*.something' or 'something'). http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:94: website=get_optional('general', 'website'), The name used in Gecko metadata is "homepage" (localizable there), we should use that here for consistency. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:102: 'end': metadata.get('contentScripts', 'document_end' ).split(), This makes a contentScripts section mandatory - the Chrome packager doesn't however. I don't think that having content script (especially both kinds of them) should be mandatory. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:110: template = getTemplate('background.html.tmpl') Please use autoEscape=True here. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:112: backgroundScripts=re.split(r'\s+', params['metadata'].get( Shouldn't params['metadata'].get('general', 'backgroundScripts').split() be sufficient here? http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:141: os.makedirs(os.path.dirname(path)) I don't like how this can go into an endless loop if the library functions don't behave the way we expect them to. This uses fewer lines without potential endless loops: try: file = open(path, 'wb') except IOError, e: if e.errno != errno.ENOENT: raise os.makedirs(os.path.dirname(path)) file = open(path, 'wb') Only disadvantage is duplicating one fairly trivial line of code. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:155: digestinfo_filename = tempfile.mktemp() mktemp() is deprecated, I suggest using mkstemp() and closing the handle immediately instead. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:161: '--sig-size', str(len(key.sign(''))) Wouldn't it make more sense to check the length of key.private_encrypt('', M2Crypto.RSA.pkcs1_padding) given that this is what we are actually using? http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:163: arg for cert in certs for arg in ('--cert-loc', cert) While this is certainly a clever approach, I have the distinct impression that a regular for loop would be more readable here: command = [ 'xar', '--sign', '-f', outFile, '--digestinfo-to-sign', digestinfo_filename, '--sig-size', str(len(key.sign(''))) ] for cert in certs: command.extend(['--cert-loc', cert]) subprocess.check_call(command) http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:177: fd, signature_filename = tempfile.mkstemp() No need to remove the file manually: with tempfile.NamedTemporaryFile(delete=False) as file: ... file.close() subprocess.check_call([..., file.name, ...]) http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:208: process=lambda path, data: data) I would suggest defining a processFile() function like in the Chrome packager. We replace it to create "special" builds without changing the source code. Also, you might actually need that function to correct the relative URLs in HTML files at build time (see my comment in the other review). http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:226: dirname = getDefaultFileName('', metadata, version, 'safariextension') It doesn't look like the name of that directory is important, it doesn't need to match the file name. I would suggest going with: dirname = '%s.safariextension' % metadata.get('general', 'basename') http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl File safariInfo.js.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl#newc... safariInfo.js.tmpl:24: {%- for x in ('application', 'platform') %} I think that the platform here is "webkit", not "safari" (the applications building on that platform would be "safari" and "mobilesafari"). But even if the fields were the same, I would certainly prefer get applicationVersion() { return this.platformVersion; } to this loop.
Agreed, to all comments not replied to. http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl File Info.plist.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode8 Info.plist.tmpl:8: <string>{{ identifier }}</string> On 2013/09/10 10:15:27, Wladimir Palant wrote: > I suggest generating the identifier automatically rather than specifying it > explicitly: > > org.adblockplus.{{basename}} I assumed that the templates (or buildtools in general) shouldn't be too ABP specific. If this isn't the case we could hard-code adblockplus here. However wouldn't it supposed to be com.eyeo? As far as I know org isn't dedicated to open source projects, but to non-profit organizations, which we aren't. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:316: def convert(self, params, files): On 2013/09/10 10:15:27, Wladimir Palant wrote: > Does this need to be an object? You are never really using the self variable, > this looks like abuse of object oriented programming. How about having a > function convertImage with multiple nested functions and a filters dictionary at > the top? > > filters = {"blend": filter_blend, ...} I'm using self to access the filters. Yes, I could use a dict instead, but it would require to define the filter names redundant, once in the function definitions and once in the definition of the filters dictionary. > Note that this entire functionality is only marginally related to the packager. > I am inclined to suggest moving all the generic functionality (meaning image > construction/saving and filters) into a separate module while only keeping the > real packager-related code here. Agreed. > Then again, isn't blend the only filter we actually need? I would vote against > having code around just because "we might need it". This introduces unnecessary > complexity and leads to code that is broken because it isn't used anywhere. All filters are used in the new metadata.chrome and metadata.safari files. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:377: def filter_colorToAlpha(self, image, *color): On 2013/09/10 10:15:27, Wladimir Palant wrote: > I cannot really imagine what we would use this for. Safari ignores all image data except the alpha channel for toolbar item icons. So without converting white to alpha, our logo would be just a blank hexagon in Safari. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:74: continue On 2013/09/10 10:15:27, Wladimir Palant wrote: > How about reusing Chrome's format? Meaning that "http://*/*", "https://*/*" is > used instead of "<all_urls>". That is already handled by the code below. <all_urls> is actually part of Chrome's format. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue On 2013/09/10 10:15:27, Wladimir Palant wrote: > If you really want this to be correct then it should be: > > if url.hostname == '*': > allowAllDomains = True > continue > > if url.hostname.startswith('*.'): > domain = url.hostname[2:] > else: > domain = url.hostname > > allowedDomains.add(domain) > > Note that this covers all the cases allowed by > http://developer.chrome.com/extensions/match_patterns.html (host name can be > '*', '*.something' or 'something'). Maybe we should just remove that code, and hard code to allow all domains in the template? Converting chrome's url permissions to Apple's domain restrictions, isn't probably worth. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:141: os.makedirs(os.path.dirname(path)) On 2013/09/10 10:15:27, Wladimir Palant wrote: > I don't like how this can go into an endless loop if the library functions don't > behave the way we expect them to. This uses fewer lines without potential > endless loops: > > try: > file = open(path, 'wb') > except IOError, e: > if e.errno != errno.ENOENT: > raise > os.makedirs(os.path.dirname(path)) > file = open(path, 'wb') > > Only disadvantage is duplicating one fairly trivial line of code. This uses exactly the same amount of lines of code, but it duplicates logic, which I really want to avoid. Note that in order to avoid an endless loop I re-raise the IOError in any other case than the directory doesn't exist. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:161: '--sig-size', str(len(key.sign(''))) On 2013/09/10 10:15:27, Wladimir Palant wrote: > Wouldn't it make more sense to check the length of key.private_encrypt('', > M2Crypto.RSA.pkcs1_padding) given that this is what we are actually using? No, if you encrypt an empty plain text you get an empty cipher text. However sign() always returns a signature with the block size of the used cipher, which we have to figure out here. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:163: arg for cert in certs for arg in ('--cert-loc', cert) On 2013/09/10 10:15:27, Wladimir Palant wrote: > While this is certainly a clever approach, I have the distinct impression that a > regular for loop would be more readable here: > > command = [ > 'xar', '--sign', '-f', outFile, > '--digestinfo-to-sign', digestinfo_filename, > '--sig-size', str(len(key.sign(''))) > ] > for cert in certs: > command.extend(['--cert-loc', cert]) > subprocess.check_call(command) I really like the feature in python that you can use nested loops in list comprehensions, please don't take it away from me. :) If the loop would be more complex I would prefer a regular for loop, too. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:177: fd, signature_filename = tempfile.mkstemp() On 2013/09/10 10:15:27, Wladimir Palant wrote: > No need to remove the file manually: > > with tempfile.NamedTemporaryFile(delete=False) as file: > ... > file.close() > subprocess.check_call([..., file.name, ...]) That wouldn't work. You close the file twice, but don't delete it. If you want to use NamedTemporaryFile the code would have to look like that, which isn't any improvement over using mkstemp(): file = tempfile.NamedTemporaryFile(delete=False) try: try: ... finally: file.close() subprocess.check_call([..., file.name, ...]) finally: os.unlink(file.name) http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl File safariInfo.js.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl#newc... safariInfo.js.tmpl:24: {%- for x in ('application', 'platform') %} On 2013/09/10 10:15:27, Wladimir Palant wrote: > I think that the platform here is "webkit", not "safari" (the applications > building on that platform would be "safari" and "mobilesafari"). But even if the > fields were the same, I would certainly prefer get applicationVersion() { return > this.platformVersion; } to this loop. I've no idea what application and platform is supposed to be. I've looked at the chromeInfo.js and on Chrome application is always "chrome" and platform "chromium", both with the chrome (not webkit) version number. So I'm a little confused now. :/
http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl File Info.plist.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/Info.plist.tmpl#newcode8 Info.plist.tmpl:8: <string>{{ identifier }}</string> On 2013/09/10 12:40:43, sebastian wrote: > I assumed that the templates (or buildtools in general) shouldn't be too ABP > specific. If this isn't the case we could hard-code adblockplus here. Well, we hardcode the devbuild URLs already so there is an inherent assumption in buildtools that the extension will be hosted on adblockplus.org. We could have an additional "id" field like in the Gecko metadata but there we have it mostly for compatibility reasons - some extensions don't use the foo@adblockplus.org ID scheme and we can no longer change that. > However wouldn't it supposed to be com.eyeo? As far as I know org isn't > dedicated to open source projects, but to non-profit organizations, which we > aren't. Why does it matter? adblockplus.org is where we do development. Concerning the .org TLD you could read http://en.wikipedia.org/wiki/.org#Registrations for example. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:379: color = map(float, color) Shouldn't we verify that three colors are given and throw a meaningful exception if not? http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, I have a pretty hard time judging whether this algorithm is correct. However, it seems to make more sense to have a Python function difference() that will simply calculate the difference to color and replace the weird max() sequence above by max(difference(red_band, green_band, blue_band)). Still, aren't we applying this to grayscale images only? If so then this is seriously overcomplicating things - we can have a luminosityToAlpha filter which will convert the image to grayscale first and use any band of the resulting image as the alpha band (all color bands are the same for grayscale images by definition). http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:413: ) Please indicate the source of your code whenever you reuse code. I incidentally found out that the source of this code is http://stackoverflow.com/a/1617909/785541. This particular piece of code appears to be licensed under the LGPL and was posted on Stack Overflow in violation of the license terms, something that we need to know and consider. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:74: continue On 2013/09/10 12:40:43, sebastian wrote: > <all_urls> is actually part of Chrome's format. Ok, I didn't realize that. Fine with me then. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue On 2013/09/10 12:40:43, sebastian wrote: > Maybe we should just remove that code, and hard code to allow all domains in the > template? Converting chrome's url permissions to Apple's domain restrictions, > isn't probably worth. Then a change to Chrome extension permissions would have to be followed-up by a template change - not really a great situation. Mind you, Safari's manifest format isn't really meant to be edited manually. So I'd rather keep it in the metadata file. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:141: os.makedirs(os.path.dirname(path)) On 2013/09/10 12:40:43, sebastian wrote: > This uses exactly the same amount of lines of code, One line less actually but that's not really the point. > Note that in order to avoid an endless loop I > re-raise the IOError in any other case than the directory doesn't exist. Yes, exactly what I meant. The implicit assumption here: "ENOENT cannot be raised again if we called os.makedirs() and it succeeded." http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:161: '--sig-size', str(len(key.sign(''))) On 2013/09/10 12:40:43, sebastian wrote: > No, if you encrypt an empty plain text you get an empty cipher text. That's not what I see. For my key I actually get a 256 byte string with private_encrypt(), same as with key.sign(). In fact, private_encrypt() is clearly meant to encrypt digests - it will always produce a result with the same length and fail if the input is too large. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:163: arg for cert in certs for arg in ('--cert-loc', cert) On 2013/09/10 12:40:43, sebastian wrote: > I really like the feature in python that you can use nested loops in list > comprehensions, please don't take it away from me. :) > > If the loop would be more complex I would prefer a regular for loop, too. I guess this is the point where we disagree - the inline loop here is already too complex and obscures the logic IMHO, a regular loop is much easier to understand. Well, we can ask Felix for his opinion. If he thinks that this is understandable as is then we can leave it. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:177: fd, signature_filename = tempfile.mkstemp() On 2013/09/10 12:40:43, sebastian wrote: > That wouldn't work. You close the file twice, but don't delete it. You are right, my comment was wishful thinking, not how NamedTemporaryFile actually works. Too bad that deleting was tied to closing here, I don't really get the logic behind that decision. My assumption was that delete=False would decouple them again but looking at the source code I see that I was wrong. http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl File safariInfo.js.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl#newc... safariInfo.js.tmpl:24: {%- for x in ('application', 'platform') %} On 2013/09/10 12:40:43, sebastian wrote: > I've no idea what application and platform is supposed to be. "application" is the specific application. "platform" is normally a platform that multiple applications using the same extension mechanism share. Given that Safari is currently the only application using that extension mechanism using "safari" as the platform name might make sense of course - other WebKit-based application aren't exactly running on the same platform indeed.
http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:379: color = map(float, color) On 2013/09/10 14:02:12, Wladimir Palant wrote: > Shouldn't we verify that three colors are given and throw a meaningful exception > if not? I'm not a big fan of adding extra code to throw an exceptions in cases where an exception would be thrown anyway. I rather prefer to keep the code simple and free of unneeded error handling, so the traceback is more meaningful. ;) http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, On 2013/09/10 14:02:12, Wladimir Palant wrote: > I have a pretty hard time judging whether this algorithm is correct. However, it > seems to make more sense to have a Python function difference() that will simply > calculate the difference to color and replace the weird max() sequence above by > max(difference(red_band, green_band, blue_band)). > > Still, aren't we applying this to grayscale images only? If so then this is > seriously overcomplicating things - we can have a luminosityToAlpha filter which > will convert the image to grayscale first and use any band of the resulting > image as the alpha band (all color bands are the same for grayscale images by > definition). Yes you could do that in plain python instead of with an image expression. But it would be way slower, which might be ok in our case since our icons are rather small images. But I don't see how this would result in a much simpler implementation. At the moment images passed to that filter aren't converted to grayscale. But yes in our specific case we could convert them to grayscale and hardcode the color white, to make the algorithm simpler. But I would prefer to keep the more flexible implementation. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue On 2013/09/10 14:02:12, Wladimir Palant wrote: > On 2013/09/10 12:40:43, sebastian wrote: > > Maybe we should just remove that code, and hard code to allow all domains in > the > > template? Converting chrome's url permissions to Apple's domain restrictions, > > isn't probably worth. > > Then a change to Chrome extension permissions would have to be followed-up by a > template change - not really a great situation. Mind you, Safari's manifest > format isn't really meant to be edited manually. So I'd rather keep it in the > metadata file. Instead of hard-coding it in the template, we could also make them, metadata settings that are more like safari's settings than chrome's permissions. However actually I like the current approach to convert chrome's url permissions to safari settings. So they only need to be defined at one place. But as you have already noted, they aren't very compatible anyway, and a fair amount of code that is hard to get right is required for the conversion. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:141: os.makedirs(os.path.dirname(path)) On 2013/09/10 14:02:12, Wladimir Palant wrote: > On 2013/09/10 12:40:43, sebastian wrote: > > This uses exactly the same amount of lines of code, > > One line less actually but that's not really the point. > > > Note that in order to avoid an endless loop I > > re-raise the IOError in any other case than the directory doesn't exist. > > Yes, exactly what I meant. The implicit assumption here: "ENOENT cannot be > raised again if we called os.makedirs() and it succeeded." Ok with no repeated code and no assumptions that might (theoretically) result in an infinite loop, the code would look like that: for retries in xrange(2): try: file = open(path, 'wb') break except IOError, e: if retries > 0 or e.errno != errno.ENOENT: raise os.makedirs(os.path.dirname(path)) Or alternatively, we could just do it the other way around, which would be simpler but then we attempt to create the directory every time, even though it most likely already exist: try: os.makedirs(os.path.dirname(path)) except OSError: pass file = open(path, 'wb') Pick one. ;) http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:161: '--sig-size', str(len(key.sign(''))) On 2013/09/10 14:02:12, Wladimir Palant wrote: > On 2013/09/10 12:40:43, sebastian wrote: > > No, if you encrypt an empty plain text you get an empty cipher text. > > That's not what I see. For my key I actually get a 256 byte string with > private_encrypt(), same as with key.sign(). In fact, private_encrypt() is > clearly meant to encrypt digests - it will always produce a result with the same > length and fail if the input is too large. Ah, it always pads the data up to the given block size. That's why even an empty string results in 256 bytes of cipher text. So we could actually use private_encrypt() instead of sign() here. But it doesn't make any difference since sign is just means digest + encrypt. So it will always have the same output length as just encrypt.
http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, On 2013/09/10 16:50:05, sebastian wrote: > At the moment images passed to that filter aren't converted to grayscale. But > yes in our specific case we could convert them to grayscale and hardcode the > color white, to make the algorithm simpler. But I would prefer to keep the more > flexible implementation. Given the complexity of this implementation I'm going to insist on a simpler solution, this will become a maintenance burden otherwise. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue On 2013/09/10 16:50:05, sebastian wrote: > Instead of hard-coding it in the template, we could also make them, metadata > settings that are more like safari's settings than chrome's permissions. Given that Chrome's permissions are well-documented I would prefer staying with them. IMHO the conversion is fairly straightforward. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:141: os.makedirs(os.path.dirname(path)) On 2013/09/10 16:50:05, sebastian wrote: > Or alternatively, we could just do it the other way around, which would be > simpler but then we attempt to create the directory every time, even though it > most likely already exist: > > try: > os.makedirs(os.path.dirname(path)) > except OSError: > pass > file = open(path, 'wb') I was actually about to suggest that solution given that we use it elsewhere already. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:161: '--sig-size', str(len(key.sign(''))) On 2013/09/10 16:50:05, sebastian wrote: > So we could actually use > private_encrypt() instead of sign() here. But it doesn't make any difference > since sign is just means digest + encrypt. So it will always have the same > output length as just encrypt. Sure, but that's an implementation detail. Given that encrypt is what we use below, the length of encrypt output is what we need to know here.
http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:316: def convert(self, params, files): On 2013/09/10 10:15:27, Wladimir Palant wrote: > Does this need to be an object? You are never really using the self variable, > this looks like abuse of object oriented programming. How about having a > function convertImage with multiple nested functions and a filters dictionary at > the top? > > filters = {"blend": filter_blend, ...} > > Note that this entire functionality is only marginally related to the packager. > I am inclined to suggest moving all the generic functionality (meaning image > construction/saving and filters) into a separate module while only keeping the > real packager-related code here. > > Then again, isn't blend the only filter we actually need? I would vote against > having code around just because "we might need it". This introduces unnecessary > complexity and leads to code that is broken because it isn't used anywhere. I'd vote for having this as a bunch of free functions in a separate module. You should be able to use locals() to call the correct filter function, no need for a dict. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:379: color = map(float, color) On 2013/09/10 16:50:05, sebastian wrote: > On 2013/09/10 14:02:12, Wladimir Palant wrote: > > Shouldn't we verify that three colors are given and throw a meaningful > exception > > if not? > > I'm not a big fan of adding extra code to throw an exceptions in cases where an > exception would be thrown anyway. I rather prefer to keep the code simple and > free of unneeded error handling, so the traceback is more meaningful. ;) I agree somewhat. However, in this case, I think it'd be most clear to just have r, g and b in the signature instead of color*. It's the same for the caller and will lead to friendlier errors. http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, On 2013/09/11 06:41:36, Wladimir Palant wrote: > On 2013/09/10 16:50:05, sebastian wrote: > > At the moment images passed to that filter aren't converted to grayscale. But > > yes in our specific case we could convert them to grayscale and hardcode the > > color white, to make the algorithm simpler. But I would prefer to keep the > more > > flexible implementation. > > Given the complexity of this implementation I'm going to insist on a simpler > solution, this will become a maintenance burden otherwise. I prefer the approach suggested by Wladimir for this, simpler and easier to follow. We can still look into using PIL.ImageMath and a faster algorithm if this takes too long in practice. I somewhat doubt it though. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:56: d[key] = '<%s>%s</%s>' % (type, val, type) On 2013/09/10 10:15:27, Wladimir Palant wrote: > Please don't generate markup here. This should do: > > try: > val = float(val) > except ValueErrro: > pass > d[key] = val > > The template should check isinstance(val, float) then and generate the correct > markup. Agreed, that's simpler. But I'd rather not ignore a ValueError here. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:163: arg for cert in certs for arg in ('--cert-loc', cert) On 2013/09/10 14:02:12, Wladimir Palant wrote: > On 2013/09/10 12:40:43, sebastian wrote: > > I really like the feature in python that you can use nested loops in list > > comprehensions, please don't take it away from me. :) > > > > If the loop would be more complex I would prefer a regular for loop, too. > > I guess this is the point where we disagree - the inline loop here is already > too complex and obscures the logic IMHO, a regular loop is much easier to > understand. > > Well, we can ask Felix for his opinion. If he thinks that this is understandable > as is then we can leave it. I think Sebastian's approach is understandable, and if he prefers it, we should go with that. I'd personally have gone for the for loop though. http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl File safariInfo.js.tmpl (right): http://codereview.adblockplus.org/11544056/diff/10001/safariInfo.js.tmpl#newc... safariInfo.js.tmpl:24: {%- for x in ('application', 'platform') %} On 2013/09/10 14:02:12, Wladimir Palant wrote: > On 2013/09/10 12:40:43, sebastian wrote: > > I've no idea what application and platform is supposed to be. > > "application" is the specific application. "platform" is normally a platform > that multiple applications using the same extension mechanism share. Given that > Safari is currently the only application using that extension mechanism using > "safari" as the platform name might make sense of course - other WebKit-based > application aren't exactly running on the same platform indeed. Voting for using "safari" as the platform name.
http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, On 2013/09/11 14:44:35, Felix H. Dahlke wrote: > On 2013/09/11 06:41:36, Wladimir Palant wrote: > > On 2013/09/10 16:50:05, sebastian wrote: > > > At the moment images passed to that filter aren't converted to grayscale. > But > > > yes in our specific case we could convert them to grayscale and hardcode the > > > color white, to make the algorithm simpler. But I would prefer to keep the > > more > > > flexible implementation. > > > > Given the complexity of this implementation I'm going to insist on a simpler > > solution, this will become a maintenance burden otherwise. > > I prefer the approach suggested by Wladimir for this, simpler and easier to > follow. We can still look into using PIL.ImageMath and a faster algorithm if > this takes too long in practice. I somewhat doubt it though. I still don't see how not using PIL.ImageMath would actually make anything simpler. PIL.ImageMath isn't only faster, it also avoids the otherwise required boilerplate code to modify the image data pixel by pixel. Yes it's built-in syntax and functions are limited, but you can easily make python functions available within ImageMath when required, as already done here. However I will look if I find a simpler approach for our use case. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:56: d[key] = '<%s>%s</%s>' % (type, val, type) On 2013/09/11 14:44:35, Felix H. Dahlke wrote: > On 2013/09/10 10:15:27, Wladimir Palant wrote: > > Please don't generate markup here. This should do: > > > > try: > > val = float(val) > > except ValueErrro: > > pass > > d[key] = val > > > > The template should check isinstance(val, float) then and generate the correct > > markup. > > Agreed, that's simpler. But I'd rather not ignore a ValueError here. Wladimir's point wasn't that his approach is simpler and in fact it isn't. The code for generating the markup is just moved to the template. And this is the point, that markup belongs in the template. I don't disagree, though this will require to repeat myself in the template. However not ignoring the ValueError is not an option. It's entirely legit, when the value isn't a number. But we have to convert it to a numeric type in order to use {% if value is number %} in the template, to either generate a <real> or <string> tag dependent on the type.
http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerChrome.py#newco... packagerChrome.py:404: difference2=lambda source, col: (col - source) / col, As I said, converting to greyscale and taking any color band as the alpha band should be sufficient. PIL.ImageMath is certainly powerful but it seems to be unnecessary here. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:56: d[key] = '<%s>%s</%s>' % (type, val, type) On 2013/09/11 15:48:06, sebastian wrote: > Wladimir's point wasn't that his approach is simpler and in fact it isn't. The > code for generating the markup is just moved to the template. And this is the > point, that markup belongs in the template. I don't disagree, though this will > require to repeat myself in the template. Yes. You shouldn't repeat yourself in the template however, that's what macros are good for. > However not ignoring the ValueError is not an option. Agreed. We are dealing with floats and strings here, in case of ValueError we fall back to treating the parameter as a string.
http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:56: d[key] = '<%s>%s</%s>' % (type, val, type) On 2013/09/11 16:14:56, Wladimir Palant wrote: > Agreed. We are dealing with floats and strings here, in case of ValueError we > fall back to treating the parameter as a string. My bad, ignoring it is fine of course.
Except the comment below, all issues should be fixed now. http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:85: continue On 2013/09/10 10:15:27, Wladimir Palant wrote: > If you really want this to be correct then it should be: > > if url.hostname == '*': > allowAllDomains = True > continue > > if url.hostname.startswith('*.'): > domain = url.hostname[2:] > else: > domain = url.hostname > > allowedDomains.add(domain) > > Note that this covers all the cases allowed by > http://developer.chrome.com/extensions/match_patterns.html (host name can be > '*', '*.something' or 'something'). You either have to list each domain explicitly or you have to allow all domains in Safari. So if *.example.com is given, and I would use the approach you suggested, example.com would be allowed, but not foo.example.com, for example. So whenever a wildcard is used, the only thing I can do is to fall back to allow all domains in Safari.
http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:23: from collections import OrderedDict On 2013/09/10 10:15:27, Wladimir Palant wrote: > OrderedDict isn't actually used here. Not addressed? The same goes for most of Wladimir's comments below. http://codereview.adblockplus.org/11544056/diff/32001/imageConversion.py File imageConversion.py (right): http://codereview.adblockplus.org/11544056/diff/32001/imageConversion.py#newc... imageConversion.py:67: image = image.convert('L') I guess I'm missing something here, shouldn't image.convert('LA') do what we want? If not, worth a comment IMO. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... packagerChrome.py:163: def convertFirstRunPage(params, files): As I said in the other review, I think it'd be fine to just always use a template for the first run page. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... packagerChrome.py:174: trim_blocks=True, Seems like the indentation is off here.
http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/10001/packagerSafari.py#newco... packagerSafari.py:23: from collections import OrderedDict On 2013/10/15 16:24:38, Felix H. Dahlke wrote: > On 2013/09/10 10:15:27, Wladimir Palant wrote: > > OrderedDict isn't actually used here. > > Not addressed? The same goes for most of Wladimir's comments below. Nevermind, looks like I looked at the wrong diff here for some reason. I'll review the newest version tomorrow.
http://codereview.adblockplus.org/11544056/diff/32001/imageConversion.py File imageConversion.py (right): http://codereview.adblockplus.org/11544056/diff/32001/imageConversion.py#newc... imageConversion.py:67: image = image.convert('L') On 2013/10/15 16:24:38, Felix H. Dahlke wrote: > I guess I'm missing something here, shouldn't image.convert('LA') do what we > want? If not, worth a comment IMO. Image.convert('LA') doesn't work reliable, in all cases. I observed that when the image uses an indexed palette (mode P), converting it to 'LA', results in an image that just looks like random gray pixels. However converting to 'L', always works properly. Also that way an alpha channel is only added if actually needed. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... packagerChrome.py:163: def convertFirstRunPage(params, files): On 2013/10/15 16:24:38, Felix H. Dahlke wrote: > As I said in the other review, I think it'd be fine to just always use a > template for the first run page. My intention was to don't make the change on the first run page depend on the change on the buildtools and vice versa. So the firstRun.html will just work like before, without processing it with jinja2. In my experience it is always a mess to require two repositories that depend on each other, to be updated at the same time. For example the changes on the Firefox extension, would have to update the revision of the buildtools the subrepo reffers to, to a version that doesn't exist yet. We also would have to update the Chrome repo at the time we merge this (don't know about IE), if we require the first run page to be always generated. What do you think? I think it would be better to do it in two steps, if we want the first run page to be always generated. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... packagerChrome.py:174: trim_blocks=True, On 2013/10/15 16:24:38, Felix H. Dahlke wrote: > Seems like the indentation is off here. True, I'll fix that.
Also reviewed packagerSafari.py now, looks like the issues have all been addressed, except for one. http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/32001/packagerChrome.py#newco... packagerChrome.py:163: def convertFirstRunPage(params, files): On 2013/10/15 17:06:56, sebastian wrote: > On 2013/10/15 16:24:38, Felix H. Dahlke wrote: > > As I said in the other review, I think it'd be fine to just always use a > > template for the first run page. > > My intention was to don't make the change on the first run page depend on the > change on the buildtools and vice versa. So the firstRun.html will just work > like before, without processing it with jinja2. > > In my experience it is always a mess to require two repositories that depend on > each other, to be updated at the same time. For example the changes on the > Firefox extension, would have to update the revision of the buildtools the > subrepo reffers to, to a version that doesn't exist yet. We also would have to > update the Chrome repo at the time we merge this (don't know about IE), if we > require the first run page to be always generated. > > What do you think? I think it would be better to do it in two steps, if we want > the first run page to be always generated. Fair enough, sure. http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py#newco... packagerSafari.py:101: endScripts= (get_optional('contentScripts', 'document_end' ) or '').split(), I'd prefer to have no whitespace after endScripts= as well as after 'document_end', that'd be consistent with the surrounding code. http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py#newco... packagerSafari.py:108: template = getTemplate('background.html.tmpl') Wladimir suggested to use autoEscape=True here, did you consider that? Sounds reasonable.
http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py#newco... packagerSafari.py:101: endScripts= (get_optional('contentScripts', 'document_end' ) or '').split(), On 2013/10/15 22:46:11, Felix H. Dahlke wrote: > I'd prefer to have no whitespace after endScripts= as well as after > 'document_end', that'd be consistent with the surrounding code. I found it more readable when the identical parts in those two lines are aligned. But if it doesn't match our coding convention I'll change it. http://codereview.adblockplus.org/11544056/diff/32001/packagerSafari.py#newco... packagerSafari.py:108: template = getTemplate('background.html.tmpl') On 2013/10/15 22:46:11, Felix H. Dahlke wrote: > Wladimir suggested to use autoEscape=True here, did you consider that? Sounds > reasonable. I added autoescaping for Info.plist.tmpl and just forgot to add it here too. I'll fix that.
I've uploaded a new patch set.
LGTM
I had to add a directory to packagedFiles, because I split ext.js (the browser abstraction layer for chrome and safari) into multiple files. Please review again.
On 2013/10/23 14:34:35, sebastian wrote: > I had to add a directory to packagedFiles, because I split ext.js (the browser > abstraction layer for chrome and safari) into multiple files. Please review > again. It's only this change, right? http://codereview.adblockplus.org/11544056/diff2/45001:53001/packagerChrome.py LGTM for that
On 2013/10/23 14:36:00, Felix H. Dahlke wrote: > On 2013/10/23 14:34:35, sebastian wrote: > > I had to add a directory to packagedFiles, because I split ext.js (the browser > > abstraction layer for chrome and safari) into multiple files. Please review > > again. > > It's only this change, right? > > http://codereview.adblockplus.org/11544056/diff2/45001:53001/packagerChrome.py > > LGTM for that Yes it is. You can also see above in the "Delta from patch set" column, which files have been changed with the new patch set.
On 2013/10/23 14:39:26, sebastian wrote: > Yes it is. You can also see above in the "Delta from patch set" column, which > files have been changed with the new patch set. I know, just wanted to make sure nothing funny happened to the diff, sometimes happens :(
I had to add a hack on top of the hack that replaces "Chrome in all translations with "Safari" or "Opera", in order to avoid that for the warning on the first run page shown to Safari users with old Safari versions, where Chrome actually means Chrome. ;)
http://codereview.adblockplus.org/11544056/diff/123001/build.py File build.py (right): http://codereview.adblockplus.org/11544056/diff/123001/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, outFile=outFile, buildNum=buildNum, releaseBuild=releaseBuild, keyFile=keyFile) Nit: restricting line length to 80 characters here might be a good idea (see PEP-8 and our coding style document). http://codereview.adblockplus.org/11544056/diff/123001/imageConversion.py File imageConversion.py (right): http://codereview.adblockplus.org/11544056/diff/123001/imageConversion.py#new... imageConversion.py:35: return image.point(table, 'L') Nit: I would add an explicit |return None| at the end of this function. http://codereview.adblockplus.org/11544056/diff/123001/imageConversion.py#new... imageConversion.py:72: return image Isn't this function equivalent to |return image.convert("LA", dither=None)|? I asked in the earlier review round but it doesn't look like you replied to that comment. http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py#newc... packagerChrome.py:178: files['firstRun.html'] = env.from_string(files['firstRun.html']).render(type=params['type']) Hardcoding the file here that needs to be passed through jinja2 is quite a hack. I guess the best solution would be a metadata section: [preprocess] firstRun.html = foo.html = The code dealing with it should be part of the shared code in packager.py, a method Files.preprocess(self, filenames, params) I guess - all packagers can call that one. Also, it should be kept generic - meaning that params should be passed as template parameters, as with all templates. Also, trim_blocks and lstrip_blocks shouldn't be set, this can be done explicitly in the template if necessary. A TODO comment is required explaining that special tag syntax is meant to be removed. And finally, autoescaping needs to be enabled automatically for any files with .html or .xml file extension (not enabling autoescaping is a huge footgun). http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py#newc... packagerChrome.py:306: files[path] = re.sub(r"\b(?<!Google )Chrome\b", params['type'].capitalize(), data) Ouch, we should really get rid of this hack... How about the following approach: * Change "enable_only_the" string into "Enable only the filter lists you need. Too many can make your browser unresponsive." * Replace "description" string by description-chrome, description-opera and description-safari. * Make the description string configurable in the metadata file: [general] description = __MSG_description-chrome__ Would be great if we could make these changes for the current translation round. http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py#newc... packagerSafari.py:33: catalog = json.loads(files['_locales/%s/messages.json' % defaultLocale]) Missing .decode("utf-8") here? The file data will be stored as an ASCII string.
On 2013/10/23 14:40:22, Felix H. Dahlke wrote: > I know, just wanted to make sure nothing funny happened to the diff, sometimes > happens :( That generally happens only when the patch is rebased.
http://codereview.adblockplus.org/11544056/diff/123001/imageConversion.py File imageConversion.py (right): http://codereview.adblockplus.org/11544056/diff/123001/imageConversion.py#new... imageConversion.py:72: return image On 2013/10/30 13:36:01, Wladimir Palant wrote: > Isn't this function equivalent to |return image.convert("LA", dither=None)|? I > asked in the earlier review round but it doesn't look like you replied to that > comment. See http://codereview.adblockplus.org/11544056/patch/32001/33004#new-line-67. Also note that the "dither" argument is ignored, except for conversion to mode "P" (palette). http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py#newc... packagerChrome.py:306: files[path] = re.sub(r"\b(?<!Google )Chrome\b", params['type'].capitalize(), data) On 2013/10/30 13:36:01, Wladimir Palant wrote: > Ouch, we should really get rid of this hack... How about the following approach: > > * Change "enable_only_the" string into "Enable only the filter lists you need. > Too many can make your browser unresponsive." > * Replace "description" string by description-chrome, description-opera and > description-safari. > * Make the description string configurable in the metadata file: > > [general] > description = __MSG_description-chrome__ > > Would be great if we could make these changes for the current translation round. I don't like that hack too, and would like to see that going to be replaced by a proper solution. But I think we should do so after the Safari release. http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py#newc... packagerSafari.py:33: catalog = json.loads(files['_locales/%s/messages.json' % defaultLocale]) On 2013/10/30 13:36:01, Wladimir Palant wrote: > Missing .decode("utf-8") here? The file data will be stored as an ASCII string. str.decode("utf-8") isn't necessary. You could just pass an encoding to json.loads(). However if you don't, utf-8 will be assumed. So its fine as it is.
http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py File packagerChrome.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerChrome.py#newc... packagerChrome.py:306: files[path] = re.sub(r"\b(?<!Google )Chrome\b", params['type'].capitalize(), data) On 2013/10/30 16:12:10, sebastian wrote: > But I think we should do so after the Safari release. Actually, given that you made the hack likely to explode in translations - I think that we should fix it *before* the Safari release (it's a trivial change). http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/123001/packagerSafari.py#newc... packagerSafari.py:33: catalog = json.loads(files['_locales/%s/messages.json' % defaultLocale]) On 2013/10/30 16:12:10, sebastian wrote: > str.decode("utf-8") isn't necessary. You could just pass an encoding to > json.loads(). However if you don't, utf-8 will be assumed. So its fine as it is. You are right, I wasn't aware of json.loads() having a special behavior for non-Unicode strings.
LGTM
On 2013/10/30 18:21:30, Wladimir Palant wrote: > LGTM Note that removing the translation hack and making preprocessing of the first-run page happen on all platforms (with standard Jinja2 syntax) should ideally still happen before this release.
I don't really like how this assumes that the extension description will always be dependent on the build target (I suggested making description configurable) but it will do for now. LGTM if you fix the issue below. http://codereview.adblockplus.org/11544056/diff/383001/packagerSafari.py File packagerSafari.py (right): http://codereview.adblockplus.org/11544056/diff/383001/packagerSafari.py#newc... packagerSafari.py:125: lambda m: '%s%s%s' % (m.group(1), prefix, m.group(3).lstrip('/')), Why look for the entire tag if all you need is the leading slash inside the attribute? Also, if the intention was to match href="..." only inside a tag then it didn't work - this regexp will happily match <>href="/fo"<>. How about: r'(<[^<>]*?\b(?:href|src)\s*=\s*(?:["\']))\/', r'\1%s' % prefix, 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
