Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1969313005:
[headless] Embed pak file into binary. (Closed)
Description[headless] Embed pak file into binary.
Embed headless_lib.pak into headless chromium binary.
Also make necessary changes to ui::ResourceBundle and ui::DataPack
to make this possible.
R=alexclarke@chromium.org,skyostil@chromium.org
BUG=610673
Review-Url: https://codereview.chromium.org/1969313005
Cr-Commit-Position: refs/heads/master@{#449263}
Committed: https://chromium.googlesource.com/chromium/src/+/6ed1a70ef94814ec92966802be57c3a36d71b7f9
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes according to comments #
Total comments: 10
Patch Set 3 : Fix. #
Total comments: 22
Patch Set 4 : Fixes according to comments. #
Total comments: 2
Patch Set 5 : Fixes according to comments. #
Total comments: 24
Patch Set 6 : Changes according to review comments. #Patch Set 7 : Rebased #Patch Set 8 : Rebased #Patch Set 9 : Rebased #Patch Set 10 : Rebased #Patch Set 11 : Resurrected #
Total comments: 20
Patch Set 12 : address comments from sky@ #Patch Set 13 : fix build/args/headless.gn #
Total comments: 4
Patch Set 14 : updated years in copyright #
Messages
Total messages: 43 (17 generated)
https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.h File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:54: bool LoadFromBuffer(base::StringPiece buffer); bikeshed: Should this be InitializeFromBuffer? Sami WDYT? https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:87: // The memory-mapped data. // The memory-mapped data. Might be null if initialized from a memory buffer. https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:88: std::unique_ptr<base::MemoryMappedFile> mmap_; Not sure if this is overkill, but I would be tempted to add an inner class that abstracts this, something like: class DataSource { virtual size_t length() const = 0; virtual const uint8_t* data() const = 0; }; Then have: class MemoryMappedDataSource : public DataSource { size_t length() const override { return mmap_->length(); } const uint8_t* data() const override { return mmap_->data(); } private: std::unique_ptr<base::MemoryMappedFile> mmap_; }; And the corresponding one for your in memory buffers. This would amount to the same thing as your code, but I suspect it would be easier to maintain, particularly if some third way of loading a file comes along. https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/resource_b... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/resource_b... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); Is base::StringPiece the right type? It's a binary blob :)
altimin@chromium.org changed reviewers: + sadrul@chromium.org
PTAL https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.h File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:54: bool LoadFromBuffer(base::StringPiece buffer); On 2016/05/12 17:31:04, alex clarke wrote: > bikeshed: Should this be InitializeFromBuffer? Sami WDYT? I don't think so — we already have LoadFromPath, LoadFromFile and LoadFromFileRegion. For the sake of consistency I'd like it to be LoadFromBuffer. https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:87: // The memory-mapped data. On 2016/05/12 17:31:04, alex clarke wrote: > // The memory-mapped data. Might be null if initialized from a memory buffer. Acknowledged. https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/data_pack.... ui/base/resource/data_pack.h:88: std::unique_ptr<base::MemoryMappedFile> mmap_; On 2016/05/12 17:31:04, alex clarke wrote: > Not sure if this is overkill, but I would be tempted to add an inner class that > abstracts this, something like: > > class DataSource { > virtual size_t length() const = 0; > virtual const uint8_t* data() const = 0; > }; > > Then have: > > class MemoryMappedDataSource : public DataSource { > size_t length() const override { return mmap_->length(); } > const uint8_t* data() const override { return mmap_->data(); } > > private: > std::unique_ptr<base::MemoryMappedFile> mmap_; > }; > > And the corresponding one for your in memory buffers. > > This would amount to the same thing as your code, but I suspect it would be > easier to maintain, particularly if some third way of loading a file comes > along. I thought about this and decided that's an overkill. But since you're thinking about this too, we probably should do it. https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/resource_b... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/1/ui/base/resource/resource_b... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); On 2016/05/12 17:31:04, alex clarke wrote: > Is base::StringPiece the right type? It's a binary blob :) It's the most appropriate type, it seems. Also, DataPack and ResourceBundle already use StringPiece for storing binary blobs.
Looks great! https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:50: # Consider zipping file here, it can reduce size up to 80%. TODO(altimin): https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data.py File headless/lib/embed_data.py (right): https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:9: HEADER="""#include "headless/lib/util/embedded_file.h" Could you add include guards and a copyright header here? https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:18: SOURCE="""#include "{header_file}" Copyright header here too please. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:51: return len(contents), "{" + ",".join(contents) + "}" Single quotes instead of double please. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:53: def GenerateHeader(args): Two blank lines between top-level entries (here and below).
PTAL. https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:50: # Consider zipping file here, it can reduce size up to 80%. On 2016/05/16 10:00:20, Sami wrote: > TODO(altimin): Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data.py File headless/lib/embed_data.py (right): https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:9: HEADER="""#include "headless/lib/util/embedded_file.h" On 2016/05/16 10:00:20, Sami wrote: > Could you add include guards and a copyright header here? Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:18: SOURCE="""#include "{header_file}" On 2016/05/16 10:00:20, Sami wrote: > Copyright header here too please. Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:51: return len(contents), "{" + ",".join(contents) + "}" On 2016/05/16 10:00:20, Sami wrote: > Single quotes instead of double please. Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:53: def GenerateHeader(args): On 2016/05/16 10:00:20, Sami wrote: > Two blank lines between top-level entries (here and below). Done.
PTAL. https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1969313005/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:50: # Consider zipping file here, it can reduce size up to 80%. On 2016/05/16 10:00:20, Sami wrote: > TODO(altimin): Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data.py File headless/lib/embed_data.py (right): https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:9: HEADER="""#include "headless/lib/util/embedded_file.h" On 2016/05/16 10:00:20, Sami wrote: > Could you add include guards and a copyright header here? Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:18: SOURCE="""#include "{header_file}" On 2016/05/16 10:00:20, Sami wrote: > Copyright header here too please. Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:51: return len(contents), "{" + ",".join(contents) + "}" On 2016/05/16 10:00:20, Sami wrote: > Single quotes instead of double please. Done. https://codereview.chromium.org/1969313005/diff/20001/headless/lib/embed_data... headless/lib/embed_data.py:53: def GenerateHeader(args): On 2016/05/16 10:00:20, Sami wrote: > Two blank lines between top-level entries (here and below). Done.
Thanks, headless/ lgtm.
https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:68: ~MemoryMappedDataSource() override{}; space between override and {}. No ; at the end. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:75: std::unique_ptr<base::MemoryMappedFile> mmap_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:82: ~BufferDataSource() override{}; same as above https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:92: }; ditto https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:151: // in order to simplify code. This comment feels out of place? When loading from a buffer, data_source_ doesn't refer to a mmapped file, right? https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.h:39: virtual const uint8_t* data() const = 0; These are virtual methods, I believe the style-guide asks that we call them Length() and Data() instead. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.h:62: bool LoadFromBuffer(base::StringPiece buffer); const &? https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:366: is_test_resources_ = true; Add a comment why this is a test-resource? https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); const & https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:365: base::StringPiece locale_data); const &
PTAL. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:68: ~MemoryMappedDataSource() override{}; On 2016/05/17 15:07:50, sadrul wrote: > space between override and {}. No ; at the end. Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:75: std::unique_ptr<base::MemoryMappedFile> mmap_; On 2016/05/17 15:07:50, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:82: ~BufferDataSource() override{}; On 2016/05/17 15:07:50, sadrul wrote: > same as above Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:92: }; On 2016/05/17 15:07:50, sadrul wrote: > ditto Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:151: // in order to simplify code. On 2016/05/17 15:07:50, sadrul wrote: > This comment feels out of place? When loading from a buffer, data_source_ > doesn't refer to a mmapped file, right? Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.h:39: virtual const uint8_t* data() const = 0; On 2016/05/17 15:07:50, sadrul wrote: > These are virtual methods, I believe the style-guide asks that we call them > Length() and Data() instead. Done. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/data_p... ui/base/resource/data_pack.h:62: bool LoadFromBuffer(base::StringPiece buffer); On 2016/05/17 15:07:50, sadrul wrote: > const &? It's recommended to pass base::StringPiece by value: go/stringpiecebyvalue. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:366: is_test_resources_ = true; On 2016/05/17 15:07:50, sadrul wrote: > Add a comment why this is a test-resource? Actually, I can use your advice here. Content shell uses ResourceBundle::InitSharedInstanceWithPakPath, which calls ResourceBuffer::LoadTestResources, which sets is_test_resources_ = true. Since headless shell resembles content shell, we've used InitSharedInstanceWithPakPath initially, and now switching to InitSharedInstanceWithPakBuffer and LoadTestResourcesFromBuffer. If you have ideas how to make it better, I'd be glad to hear them. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); On 2016/05/17 15:07:51, sadrul wrote: > const & See comment above. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:365: base::StringPiece locale_data); On 2016/05/17 15:07:51, sadrul wrote: > const & See comment above.
sadrul@chromium.org changed reviewers: + sky@chromium.org
+sky@ lgtm. But please wait for sky@'s review as well. https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:366: is_test_resources_ = true; On 2016/05/17 15:40:08, altimin wrote: > On 2016/05/17 15:07:50, sadrul wrote: > > Add a comment why this is a test-resource? > > Actually, I can use your advice here. Content shell uses > ResourceBundle::InitSharedInstanceWithPakPath, which calls > ResourceBuffer::LoadTestResources, which sets is_test_resources_ = true. > > Since headless shell resembles content shell, we've used > InitSharedInstanceWithPakPath initially, and now switching to > InitSharedInstanceWithPakBuffer and LoadTestResourcesFromBuffer. > > If you have ideas how to make it better, I'd be glad to hear them. It looks like |is_test_resources_| is used to determine whether it's OK to fall back to lower-res images. I feel like it would make sense to have an explicit API on ResourceBundle to set that, instead of inferring this info based on what function call was made to initialize the RB. But that's not an issue with this CL. If headless is OK with allowing fall-back to lower-res images, then this can be set here. Please add a comment explaining that. https://codereview.chromium.org/1969313005/diff/60001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/60001/ui/base/resource/data_p... ui/base/resource/data_pack.h:34: class DataSource { I suppose this class could move into DataPack, since it's an internal implementation detail of DataPack, and doesn't need to be exposed in the public API
https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/40001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:366: is_test_resources_ = true; On 2016/05/19 18:28:26, sadrul wrote: > On 2016/05/17 15:40:08, altimin wrote: > > On 2016/05/17 15:07:50, sadrul wrote: > > > Add a comment why this is a test-resource? > > > > Actually, I can use your advice here. Content shell uses > > ResourceBundle::InitSharedInstanceWithPakPath, which calls > > ResourceBuffer::LoadTestResources, which sets is_test_resources_ = true. > > > > Since headless shell resembles content shell, we've used > > InitSharedInstanceWithPakPath initially, and now switching to > > InitSharedInstanceWithPakBuffer and LoadTestResourcesFromBuffer. > > > > If you have ideas how to make it better, I'd be glad to hear them. > > It looks like |is_test_resources_| is used to determine whether it's OK to fall > back to lower-res images. I feel like it would make sense to have an explicit > API on ResourceBundle to set that, instead of inferring this info based on what > function call was made to initialize the RB. But that's not an issue with this > CL. > > If headless is OK with allowing fall-back to lower-res images, then this can be > set here. Please add a comment explaining that. Done. https://codereview.chromium.org/1969313005/diff/60001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/60001/ui/base/resource/data_p... ui/base/resource/data_pack.h:34: class DataSource { On 2016/05/19 18:28:26, sadrul wrote: > I suppose this class could move into DataPack, since it's an internal > implementation detail of DataPack, and doesn't need to be exposed in the public > API Done.
https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:69: virtual ~DataSource() {}; no ; https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:79: MemoryMappedDataSource(std::unique_ptr<base::MemoryMappedFile> mmap) explicit https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:82: ~MemoryMappedDataSource() override{}; no ;, and space after override https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:94: class BufferDataSource : public DataPack::DataSource { StringPieceDataSource https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:96: BufferDataSource(base::StringPiece buffer) : buffer_(buffer) {} explicit https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:98: ~BufferDataSource() override{}; nit: no ; and space https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:103: return reinterpret_cast<const uint8_t*>(buffer_.data()); Why do you need the reinterpret_cast here? https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.h:56: bool LoadFromBuffer(base::StringPiece buffer); LoadFromStringPiece? Also, I think it's worth mentioning life time. Specifically that this code does *not* make a copy of the data. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:211: InitSharedInstance(NULL); nullptr https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:145: // Initialize the ResourceBundle using data pack from given buffer. Also document lifetime here. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); I dislike exposing functions in the public class that are intended purely for tests and not marked as such. As a precursor to this patch move the test setup routines (InitSharedInstanceWithPakPath and LoadTestResources) into a test only class that is friended by ResourceBundle.
PTAL https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:69: virtual ~DataSource() {}; On 2016/05/19 20:25:54, sky wrote: > no ; Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:79: MemoryMappedDataSource(std::unique_ptr<base::MemoryMappedFile> mmap) On 2016/05/19 20:25:54, sky wrote: > explicit Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:82: ~MemoryMappedDataSource() override{}; On 2016/05/19 20:25:54, sky wrote: > no ;, and space after override Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:94: class BufferDataSource : public DataPack::DataSource { On 2016/05/19 20:25:54, sky wrote: > StringPieceDataSource And again, I believe that essence of this thing is a buffer, StringPiece being implementation detail. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:96: BufferDataSource(base::StringPiece buffer) : buffer_(buffer) {} On 2016/05/19 20:25:54, sky wrote: > explicit Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:98: ~BufferDataSource() override{}; On 2016/05/19 20:25:54, sky wrote: > nit: no ; and space Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:103: return reinterpret_cast<const uint8_t*>(buffer_.data()); On 2016/05/19 20:25:54, sky wrote: > Why do you need the reinterpret_cast here? Because base::StringPiece has int8_t (aka char) inside, and we need uint8_t. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.h:56: bool LoadFromBuffer(base::StringPiece buffer); On 2016/05/19 20:25:54, sky wrote: > LoadFromStringPiece? > Also, I think it's worth mentioning life time. Specifically that this code does > *not* make a copy of the data. I believe that LoadFromBuffer describes it more precisely, usage of StringPiece for storing buffer is just an implementation detail. Comment added. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:211: InitSharedInstance(NULL); On 2016/05/19 20:25:54, sky wrote: > nullptr Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:145: // Initialize the ResourceBundle using data pack from given buffer. On 2016/05/19 20:25:54, sky wrote: > Also document lifetime here. Done. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); On 2016/05/19 20:25:54, sky wrote: > I dislike exposing functions in the public class that are intended purely for > tests and not marked as such. > > As a precursor to this patch move the test setup routines > (InitSharedInstanceWithPakPath and LoadTestResources) into a test only class > that is friended by ResourceBundle. Actually, this is not test-only function. InitSharedInstanceWithPakFileRegion is used by content shell, and by newly introduced headless shell too. If you have any ideas how it should be done instead, please let me know.
https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/data_p... ui/base/resource/data_pack.cc:94: class BufferDataSource : public DataPack::DataSource { On 2016/05/19 23:04:47, altimin wrote: > On 2016/05/19 20:25:54, sky wrote: > > StringPieceDataSource > > And again, I believe that essence of this thing is a buffer, StringPiece being > implementation detail. IMO BufferDataSource is vague. It is immediately apparent what StringPieceDataSource is and does. Similar comment applies to other places. https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:146: static void InitSharedInstanceWithPakBuffer(base::StringPiece buffer); On 2016/05/19 23:04:47, altimin wrote: > On 2016/05/19 20:25:54, sky wrote: > > I dislike exposing functions in the public class that are intended purely for > > tests and not marked as such. > > > > As a precursor to this patch move the test setup routines > > (InitSharedInstanceWithPakPath and LoadTestResources) into a test only class > > that is friended by ResourceBundle. > > Actually, this is not test-only function. InitSharedInstanceWithPakFileRegion is > used by content shell, and by newly introduced headless shell too. Content shell is primarily there for tests, and not a shipping product. Additionally this sets 'is_test_resources_' to true and calls LoadTestResources. > If you have any ideas how it should be done instead, please let me know. I still stand by what I've outlined.
Would you mind rebasing this?
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Resurrected this patch after some time. skyostil@, sky@ PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:71: class DataPack::DataSource { Add description. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:75: virtual size_t Length() const = 0; Please use GetLength and GetData. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:88: size_t Length() const override { return mmap_->length(); } Generally we prefix overrides with the class the overrides come from, e.g.: // DataPack::DataSource: https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:104: size_t Length() const override { return buffer_.length(); } Same comment about adding where overrides come from here. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:107: return reinterpret_cast<const uint8_t*>(buffer_.data()); Can't this be a static_cast? https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:128: std::unique_ptr<base::MemoryMappedFile> mmap(new base::MemoryMappedFile); MakeUnique where using unique_ptr. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:168: data_source_.reset(); I think this code would be less fragile if you passed the std::unique_ptr<DataSource> to this function and it's only set as a member if you to the last branch. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:266: reinterpret_cast<const char*>(data_source_->Data() + target->file_offset), Can this be a static_cast? https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.h:36: class DataSource; Move to private section. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/resou... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/resou... ui/base/resource/resource_bundle.h:344: void LoadTestResourcesFromBuffer(base::StringPiece data, Is this used?
Thanks for quick review, PTAL. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... File ui/base/resource/data_pack.cc (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:71: class DataPack::DataSource { On 2017/02/08 22:22:58, sky wrote: > Add description. Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:75: virtual size_t Length() const = 0; On 2017/02/08 22:22:58, sky wrote: > Please use GetLength and GetData. Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:88: size_t Length() const override { return mmap_->length(); } On 2017/02/08 22:22:58, sky wrote: > Generally we prefix overrides with the class the overrides come from, e.g.: > > // DataPack::DataSource: Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:104: size_t Length() const override { return buffer_.length(); } On 2017/02/08 22:22:58, sky wrote: > Same comment about adding where overrides come from here. Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:107: return reinterpret_cast<const uint8_t*>(buffer_.data()); On 2017/02/08 22:22:58, sky wrote: > Can't this be a static_cast? It's impossible to cast from const int8_t* to const uint8_t* with static_cast. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:128: std::unique_ptr<base::MemoryMappedFile> mmap(new base::MemoryMappedFile); On 2017/02/08 22:22:58, sky wrote: > MakeUnique where using unique_ptr. Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:168: data_source_.reset(); On 2017/02/08 22:22:58, sky wrote: > I think this code would be less fragile if you passed the > std::unique_ptr<DataSource> to this function and it's only set as a member if > you to the last branch. Great suggestion, thanks! Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.cc:266: reinterpret_cast<const char*>(data_source_->Data() + target->file_offset), On 2017/02/08 22:22:58, sky wrote: > Can this be a static_cast? See above. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... File ui/base/resource/data_pack.h (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/data_... ui/base/resource/data_pack.h:36: class DataSource; On 2017/02/08 22:22:58, sky wrote: > Move to private section. Done. https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/resou... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/1969313005/diff/200001/ui/base/resource/resou... ui/base/resource/resource_bundle.h:344: void LoadTestResourcesFromBuffer(base::StringPiece data, On 2017/02/08 22:22:58, sky wrote: > Is this used? No, thanks.
LGTM
+jochen@ for build/args/headless.gn
altimin@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ for build/args/headless.gn for real
lgtm
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. https://codereview.chromium.org/1969313005/diff/240001/headless/lib/embed_dat... File headless/lib/embed_data.py (right): https://codereview.chromium.org/1969313005/diff/240001/headless/lib/embed_dat... headless/lib/embed_data.py:9: COPYRIGHT="""// Copyright 2016 The Chromium Authors. All rights reserved. ++year https://codereview.chromium.org/1969313005/diff/240001/headless/lib/util/embe... File headless/lib/util/embedded_file.h (right): https://codereview.chromium.org/1969313005/diff/240001/headless/lib/util/embe... headless/lib/util/embedded_file.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. ++year
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, jochen@chromium.org, sky@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1969313005/#ps260001 (title: "updated years in copyright")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/1969313005/diff/240001/headless/lib/embed_dat... File headless/lib/embed_data.py (right): https://codereview.chromium.org/1969313005/diff/240001/headless/lib/embed_dat... headless/lib/embed_data.py:9: COPYRIGHT="""// Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/09 10:46:58, Sami wrote: > ++year Done. https://codereview.chromium.org/1969313005/diff/240001/headless/lib/util/embe... File headless/lib/util/embedded_file.h (right): https://codereview.chromium.org/1969313005/diff/240001/headless/lib/util/embe... headless/lib/util/embedded_file.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/09 10:46:58, Sami wrote: > ++year Done.
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486638610161700,
"parent_rev": "60e243955ccc6ac466c074d812ae406dec337fe3", "commit_rev":
"6ed1a70ef94814ec92966802be57c3a36d71b7f9"}
Message was sent while issue was closed.
Description was changed from ========== [headless] Embed pak file into binary. Embed headless_lib.pak into headless chromium binary. Also make necessary changes to ui::ResourceBundle and ui::DataPack to make this possible. R=alexclarke@chromium.org,skyostil@chromium.org BUG=610673 ========== to ========== [headless] Embed pak file into binary. Embed headless_lib.pak into headless chromium binary. Also make necessary changes to ui::ResourceBundle and ui::DataPack to make this possible. R=alexclarke@chromium.org,skyostil@chromium.org BUG=610673 Review-Url: https://codereview.chromium.org/1969313005 Cr-Commit-Position: refs/heads/master@{#449263} Committed: https://chromium.googlesource.com/chromium/src/+/6ed1a70ef94814ec92966802be57... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/6ed1a70ef94814ec92966802be57... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
