Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6590020945182720: Issue 1707 - Allow importing Chrome-style locales in Firefox extensions (Closed)

Created:
Dec. 16, 2014, 10:42 p.m. by Wladimir Palant
Modified:
Dec. 17, 2014, 3:01 p.m.
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

The implementation isn`t quite the same as for Chrome, the locale mapping in particular is unnecessary. However, filling up locales got more complicated because importing data into the fallback locale has to happen first.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Addressed comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -9 lines) Patch
M packagerGecko.py View 1 4 chunks +49 lines, -9 lines 0 comments Download

Messages

Total messages: 4
Wladimir Palant
Dec. 16, 2014, 10:42 p.m. (2014-12-16 22:42:09 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/packagerGecko.py File packagerGecko.py (right): http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/packagerGecko.py#newcode67 packagerGecko.py:67: return '/'.join(os.path.relpath(path, baseDir).split(os.sep)) Nit: x.split(os.sep) -> os.path.split(x) http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/packagerGecko.py#newcode168 packagerGecko.py:168: ...
Dec. 17, 2014, 8:18 a.m. (2014-12-17 08:18:11 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/packagerGecko.py File packagerGecko.py (right): http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/packagerGecko.py#newcode67 packagerGecko.py:67: return '/'.join(os.path.relpath(path, baseDir).split(os.sep)) On 2014/12/17 08:18:12, Sebastian Noack wrote: ...
Dec. 17, 2014, 1:26 p.m. (2014-12-17 13:26:04 UTC) #3
Sebastian Noack
Dec. 17, 2014, 1:59 p.m. (2014-12-17 13:59:07 UTC) #4
http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/pack...
File packagerGecko.py (right):

http://codereview.adblockplus.org/6590020945182720/diff/5629499534213120/pack...
packagerGecko.py:168: if not os.path.exists(source):
On 2014/12/17 13:26:04, Wladimir Palant wrote:
> On 2014/12/17 08:18:12, Sebastian Noack wrote:
> > Nit: I'd prefer to catch the IOError when openening the file, instead
checking
> > whether the file exists before, to avoid race conditions.
> 
> Sure, but that's not a scenario where race conditions can happen - and a
> try/catch block is pretty verbose. So I'd rather not change this.

Theoretically, the contents of the file system can still change between the
os.path.exists() and io.open() call. Sure, this is a rather theoretical case,
and I'm not going to mention that FS operations are kinda expensive, and
therefore performance critical code should do as few round-trips as possible. ;)

But still, catching the IOError would be the correct thing to do here IMO.
However, I guess it's not important. So LGTM if you prefer to don't do so.

Powered by Google App Engine
This is Rietveld