======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/base/common/byte_store.cc?a=1 File //depot/googleclient/gears/opensource/gears/base/common/byte_store.cc (snapshot 1) ------------------------------------ Line 347: if (read_buffer_.Size() < static_cast<int64>(buffer_size)) { On 4:15 pm, cprince wrote: > I'm confused -- buffer_size is already an int64. Why have the casts here? > Or did you mean to cast .Size() up to <int64>?
Ah, brainfart on my behalf. Fixed to use MemoryBuffer::size_type, which is the type of Size() and Resize(). ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/base/firefox/install.rdf.m4?a=1 File //depot/googleclient/gears/opensource/gears/base/firefox/install.rdf.m4 (snapshot 1) ------------------------------------ Line 83: <em:targetPlatform>Linux_x86_64-gcc3</em:targetPlatform> On 4:19 pm, cprince wrote: > I wonder if this should set <targetPlatform> too, for consistency? > If there was some good reason not to, that's fine too. I'm not sure what you mean -- I am setting <em:targetPlatform>, and I don't think I can drop the "em". The 32-bit Linux Gears installer sets <em:targetPlatform> twice (to quirk for Ubuntu Edgy 6.10), but that doesn't seem to be needed for Ubunty Hardy 8.04. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/blob/buffer_blob.cc?a=1 File //depot/googleclient/gears/opensource/gears/blob/buffer_blob.cc (snapshot 1) ------------------------------------ Line 41: assert(0 <= num_bytes && num_bytes <= kint32max); On 4:33 pm, cprince wrote: > "num_bytes >= 0 && num_bytes <=" would be clearer, I think. I disagree, I think this way is clearer, partly because it's closer to the mathematical expression a < b < c (which translates, in C, to a < b && b < c). ------------------------------------ Line 45: int64 BufferBlob::Read(uint8 *destination, int64 offset, On 4:58 pm, cprince wrote: > But what you sort of want is a forced-64-bit size_t. ("size64_t" ?) This seems > like a valid argument for using uint64 for these args, even though it's > otherwise frowned upon by Google style. And after doing that, many of these > Blob diffs could be reverted, I think? Eh? "size64_t"!?? Either you want int64 or uint64, or you want ssize_t or size_t? (ssize_t is the signed equivalent of size_t). As for 32-bit versus 64-bit, Blobs have to be 64-bit, since files can be larger than 4GB. As for signed versus unsigned, it's less clear. I'm still leaning towards signed, partly because JavaScript doesn't have a notion of unsigned, and partly because we use negative numbers for error codes (e.g. a failed Read() returns -1). As you've noted, there's a bunch of diffs in this CL to cope with the signed/unsigned mismatch (in particular, Blobs speak signed, std::vector speaks size_t). If we wanted to change Blobs to speak unsigned, then there will certainly be changes in this CL that we could remove, but there very well could be other changes that we'd have to add, where the code currently assumes signed-ness. Finally, leaving things as the status quo (i.e. signed) is a smaller change, and smaller changes are usually better changes. ------------------------------------ Line 50: int64 buffer_size = static_cast<int64>(buffer_.size()); On 4:55 pm, cprince wrote: > Making buffer_size be a size_t seems a little closer to the intent. size_t is closer to the std::vector::size type, but the point of buffer_size being int64 is that I can then do 64-bit arithmetic on it (e.g. subtract offset from it). ------------------------------------ Line 51: assert(0 <= buffer_size && buffer_size <= kint32max); On 4:28 pm, cprince wrote: > This looks like a new constraint. buffer_.size() should be allowed to be > greater than kint32max, shouldn't it? Otherwise, why have int64 params to all > Blob functions? Oops, I forgot to mention this in the CL description (which I've now fixed). Yes, this is a new constraint -- restricting the BufferBlob's size to kint32max makes it easier to reason about whether or not that size fits into a (possibly 32-bit) size_t. It only affects BufferBlobs, not all Blobs, since a 2GB upper bound should be more than enough (yeah, famous last words) for an *in-memory* construction. I've also added a TODO to clarify how BufferBlobs behave as you hit the upper bound. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/tools/config.mk?a=1 File //depot/googleclient/gears/opensource/gears/tools/config.mk (snapshot 1) ------------------------------------ Line 123: ifeq ($(ARCH),) On 4:48 pm, cprince wrote: > Oh, I see, this only takes effect for (OS,linux). The lack of indentation makes > this hard to grok. Consider indenting or adding a comment. Done. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/gears/tools/rules.mk?a=1 File //depot/googleclient/gears/opensource/gears/tools/rules.mk (snapshot 1) ------------------------------------ Line 220: # we created merged installers On 4:50 pm, cprince wrote: > Nit: add trailing period on sentence. Done. ======================================================================== http://mondrian.corp.google.com/file/10624727///depot/googleclient/gears/opensource/third_party/gecko_1.9/linux/gecko_sdk/lib64/libxpcom.so?a= File //depot/googleclient/gears/opensource/third_party/gecko_1.9/linux/gecko_sdk/lib64/libxpcom.so ------------------------------------ On Sun Mar 29 22:52:23 2009 PDT, cprince wrote: > > The binary libraries (e.g. libxpcom.so, libxul.so) that are new > > to third_party are copied straight from Ubuntu's /usr/lib64/firefox. > > Does Mozilla distribute a version of these? Using libs from a mozilla URL, > instead of a local path, feels more "right" to me. AFAICT, Mozilla does not yet do an official 64-bit release. ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/10624727
