Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by Wladimir Palant
Modified:
4 years, 11 months ago
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
4 years, 11 months ago (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: ...
4 years, 11 months ago (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: ...
4 years, 11 months ago (2014-12-17 13:26:04 UTC) #3
Sebastian Noack
4 years, 11 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5