mac/rlz: Remove an incorrect check

mac/rlz: Remove an incorrect check

As a comment in the ScopedRlzValueStoreLock destructor explains:

  // Check that "store_ set" => "file_lock acquired". The converse isn't true,
  // for example if the rlz data file can't be read.

So don't CHECK when that happens, instead treat it like lock acquisition failures:
Silently drop events when that happens. I added a unit test for this scenario.

Also pass O_RDWR to open(), as posix requires one of O_READ, O_WRITE, or O_RDWR.

BUG=143950
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152659

[Nico, 2012-08-21 20:43:44]

I'm not sure why this CHECK started firing only after my flock change, it
should've happened before already.

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.cc
File rlz/lib/rlz_lib_test.cc (right):

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.c...
rlz/lib/rlz_lib_test.cc:33: }
This is a bit gross, but rlz_value_store_mac.h can't be included here since it
relies on ObjC code.

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/mac/lib/rlz_value_...
File rlz/mac/lib/rlz_value_store_mac.mm (right):

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/mac/lib/rlz_value_...
rlz/mac/lib/rlz_value_store_mac.mm:360: return [CreateRlzDirectory()
stringByAppendingPathComponent:kRlzLockfile];
The changes to this function don't change any behavior.

[Mark Mentovai, 2012-08-21 20:59:29]

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.cc
File rlz/lib/rlz_lib_test.cc (right):

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.c...
rlz/lib/rlz_lib_test.cc:33: }
Nico wrote:
> This is a bit gross, but rlz_value_store_mac.h can't be included here since it
> relies on ObjC code.

It doesn’t have to. There’s nothing Objective about that header except for the
pair of classes. Use this standard trick in the header:

#ifdef __OBJC__
@class NSDictionary;
@class NSMutableDictionary;
#else
class NSDictionary;
class NSMutableDictionary;
#endif

Then this file gets far less gross.

I notice that SetRlzStoreDirectory is declared in rlz/lib/rlz_value_store.h.
Maybe you should move it to rlz/mac/lib/rlz_value_store_mac.h too.

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.c...
rlz/lib/rlz_lib_test.cc:890: 0500);
Bump this over to line up with the other argument.

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/mac/lib/rlz_value_...
File rlz/mac/lib/rlz_value_store_mac.mm (right):

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/mac/lib/rlz_value_...
rlz/mac/lib/rlz_value_store_mac.mm:274: O_RDWR | O_CREAT,
This is a very good catch, although O_RDONLY is 0, so this shouldn’t have been a
problem before.

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/mac/lib/rlz_value_...
rlz/mac/lib/rlz_value_store_mac.mm:360: return [CreateRlzDirectory()
stringByAppendingPathComponent:kRlzLockfile];
Nico wrote:
> The changes to this function don't change any behavior.

Obviously not.

[Nico, 2012-08-21 21:05:03]

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.cc
File rlz/lib/rlz_lib_test.cc (right):

https://chromiumcodereview.appspot.com/10828424/diff/1/rlz/lib/rlz_lib_test.c...
rlz/lib/rlz_lib_test.cc:33: }
On 2012/08/21 20:59:29, Mark Mentovai wrote:
> Nico wrote:
> > This is a bit gross, but rlz_value_store_mac.h can't be included here since
it
> > relies on ObjC code.
> 
> It doesn’t have to. There’s nothing Objective about that header except for the
> pair of classes. Use this standard trick in the header:
> 
> #ifdef __OBJC__
> @class NSDictionary;
> @class NSMutableDictionary;
> #else
> class NSDictionary;
> class NSMutableDictionary;
> #endif
> 
> Then this file gets far less gross.

The included of scoped_nsobject.h needs to be conditional too, which in turn
requires the definition of the RlzValueStore class to be made conditional. If
you prefer that, I can do that, but that seemed pretty gross too.

> I notice that SetRlzStoreDirectory is declared in rlz/lib/rlz_value_store.h.
> Maybe you should move it to rlz/mac/lib/rlz_value_store_mac.h too.

Maybe I should move this new functions to rlz/lib/rlz_value_store.h instead. Did
that.

[Mark Mentovai, 2012-08-21 21:06:17]

LGTM

https://chromiumcodereview.appspot.com/10828424/diff/5001/rlz/lib/rlz_lib_tes...
File rlz/lib/rlz_lib_test.cc (right):

https://chromiumcodereview.appspot.com/10828424/diff/5001/rlz/lib/rlz_lib_tes...
rlz/lib/rlz_lib_test.cc:885: 0500);
Fix indentation (I mentioned this already).

[Nico, 2012-08-21 21:08:29]

Thanks!

Fixed the indent.

On Tue, Aug 21, 2012 at 2:06 PM,  <mark@chromium.org> wrote:
> LGTM
>
>
>
https://chromiumcodereview.appspot.com/10828424/diff/5001/rlz/lib/rlz_lib_tes...
> File rlz/lib/rlz_lib_test.cc (right):
>
>
https://chromiumcodereview.appspot.com/10828424/diff/5001/rlz/lib/rlz_lib_tes...
> rlz/lib/rlz_lib_test.cc:885: 0500);
> Fix indentation (I mentioned this already).
>
> https://chromiumcodereview.appspot.com/10828424/

[commit-bot: I haz the power, 2012-08-21 21:08:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10828424/2002

[commit-bot: I haz the power, 2012-08-21 22:53:56]

Change committed as 152659