Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
227943003:
Add experiment to measure time to hash extension content as we read it (Closed)
|
Created:
6 years, 8 months ago by asargent_no_longer_on_chrome Modified:
6 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd experiment to measure time to hash extension content as we read it
This required adding 2 new methods to URLRequestFileJob to let
subclasses get notified when seeks and reads complete, so that our
URLRequestExtensionJob class is able to compute the hash of the content
read and know what bytes from the file those correspond to (e.g. if there
was a Range specified and we didn't read all the bytes).
BUG=360909
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263571
Patch Set 1 : #
Total comments: 32
Patch Set 2 : responded to review comments #
Total comments: 2
Patch Set 3 : fixed unit test per last comment #Patch Set 4 : merged latest trunk, fixed compile problem #
Messages
Total messages: 14 (0 generated)
rvargas: please review net/ stuff Thanks!
looks good https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_protocols.cc (right): https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_protocols.cc:315: hashing_time_ += timer.Elapsed(); hashing_time_ = https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_protocols.cc:346: UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.OnReadCompleteResult", result); What do you expect to get out of this histogram? https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job.cc:106: base::Unretained(dest))); This cannot be unretained because the stream is allowed to release its reference before calling the callback (in fact, it must), so the buffer has to be protected somehow by this code. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job.cc:197: void URLRequestFileJob::OnSeekComplete(int64 result) {} nit: use { } and an empty line between the methods. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job_unittest.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. I think we don't use (c) anynmore https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:30: seek_position_(0) {} nit: { } (this is not a single line anymore) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:61: : path_(path), observer_(observer) {} nit: { } (this is not a single line anymore) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:109: base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND); Why not CREATE and WRITE? Appending to a file seems like unexpected behavior for a test. It's better to fail earlier. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:110: int bytes_written = file.WriteAtCurrentPos(content.c_str(), content.length()); file.Write(0, ...) ? (assuming CREATE and WRITE) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:110: int bytes_written = file.WriteAtCurrentPos(content.c_str(), content.length()); should check that the file is valid before using it. (may crash otherwise) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:113: file.Close(); no need to close. btw, this whole block should probably be replaced with base::WriteFile()... and maybe base::CreateTemporaryFileInDir() https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:120: class URLRequestFileJobEventsTest : public testing::Test { nit: I think this class is too long to have an inline implementation (which is easier to write but harder to read) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:174: ASSERT_LT((unsigned)range->end, content.length()); nit:c++ cast https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:174: ASSERT_LT((unsigned)range->end, content.length()); + assert start <= end ? https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:196: EXPECT_TRUE(delegate_.data_received() == expected_content); Doesn't this beeps and prints garbage when it fails? https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:224: result.append(1, static_cast<char>(i % 256)); does it make sense to preallocate the string (considering this is a test)? dunno. Just thinking out loud. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:249: } // namespace nit: do you mind keeping the actual tests outside of the namespaces? Do you need the net namespace at all? Seems like there would be very few explicit net:: otherwise.
Responded to comments. Please take another look. https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_protocols.cc (right): https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_protocols.cc:315: hashing_time_ += timer.Elapsed(); On 2014/04/08 16:53:45, rvargas wrote: > hashing_time_ = Done. https://codereview.chromium.org/227943003/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_protocols.cc:346: UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.OnReadCompleteResult", result); On 2014/04/08 16:53:45, rvargas wrote: > What do you expect to get out of this histogram? I'm interested to see the distribution of common read sizes, as that may inform the block size we pick for signing content hashes. (The ExtensionUrlRequest.TotalKbRead value below may have more impact on that decision, but having both numbers seems interesting). https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job.cc:106: base::Unretained(dest))); On 2014/04/08 16:53:45, rvargas wrote: > This cannot be unretained because the stream is allowed to release its reference > before calling the callback (in fact, it must), so the buffer has to be > protected somehow by this code. Ok, I guess I was confused by both the code flow and this statement in the io_buffer.h comments: " // Whenever you call an asynchronous operation that takes an IOBuffer, // ownership is implicitly transferred to the called function, until the // operation has completed (at which point it transfers back to the caller). " In this case, the part of the logical call chain of interest is: ... (1) URLRequestJob::ReadRawDataHelper (2) URLRequestFileJob::ReadRawData (3) FileStream::Read (4) (async reading happens and posts a task to run the callback when finished) (5) URLRequestFileJob::DidRead ... URLRequestFileJob::OnReadComplete (6) URLRequestJob::NotifyReadComplete (7) URLRequestJob::OnRawReadComplete At step (2) URLRequestJob::ReadRawDataHelper, the URLRequestJob sets it's |raw_read_buffer_| scoped_refptr member variable to hold a reference to the buffer, and doesn't clear it until URLRequestJob::OnRawReadComplete (or automatically in the destructor). The callback for URLRequestFileJob::DidRead that we pass to FileStream::Read is bound with a WeakPtr, so unless I missed something it seemed like there isn't currently any way that we could end up getting URLRequestFileJob::DidRead called where the buffer isn't still held by the raw_read_buffer_ member variable. I guess future refactoring in URLRequestFileJob could break this assumption though? I've changed the interface so that URLRequestFileJob::DidRead takes a scoped_refptr instead of a naked pointer to the IOBuffer, and then drops its reference once it's called URLRequestFileJob::OnReadComplete. Let me know if this seems better to you. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job.cc:197: void URLRequestFileJob::OnSeekComplete(int64 result) {} On 2014/04/08 16:53:45, rvargas wrote: > nit: use { > } > and an empty line between the methods. Done. (Note that 'git cl format' does {} without newline separation, but I'm happy to conform to local style here and override it). https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job_unittest.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/04/08 16:53:45, rvargas wrote: > I think we don't use (c) anynmore Done. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:30: seek_position_(0) {} On 2014/04/08 16:53:45, rvargas wrote: > nit: { > } > (this is not a single line anymore) Done. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:61: : path_(path), observer_(observer) {} On 2014/04/08 16:53:45, rvargas wrote: > nit: { > } > (this is not a single line anymore) Done. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:113: file.Close(); On 2014/04/08 16:53:45, rvargas wrote: > no need to close. > > btw, this whole block should probably be replaced with base::WriteFile()... and > maybe base::CreateTemporaryFileInDir() Ah, much simpler, thanks for the tip. Done. (this obsoletes the fixes for the above 3 comments). https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:120: class URLRequestFileJobEventsTest : public testing::Test { On 2014/04/08 16:53:45, rvargas wrote: > nit: I think this class is too long to have an inline implementation (which is > easier to write but harder to read) Good point. I've moved the JobObserverImpl and Range classes out to be standalone since they didn't really need to be nested classes, and switched to separating declaration/definition for the rest of the class. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:174: ASSERT_LT((unsigned)range->end, content.length()); On 2014/04/08 16:53:45, rvargas wrote: > nit:c++ cast Done. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:174: ASSERT_LT((unsigned)range->end, content.length()); On 2014/04/08 16:53:45, rvargas wrote: > + assert start <= end ? Done. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:196: EXPECT_TRUE(delegate_.data_received() == expected_content); On 2014/04/08 16:53:45, rvargas wrote: > Doesn't this beeps and prints garbage when it fails? Nope- notice I used EXPECT_TRUE on single boolean expression instead of EXPECT_EQ with 2 parameters. I probably had to learn to do this the hard way already, after locally producing the beeps and garbage you mention. =) https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:224: result.append(1, static_cast<char>(i % 256)); On 2014/04/08 16:53:45, rvargas wrote: > does it make sense to preallocate the string (considering this is a test)? > dunno. Just thinking out loud. Yes, good idea. Done.
Just one relevant comment on the test. LGTM after that. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job.cc:106: base::Unretained(dest))); On 2014/04/09 20:59:59, Antony Sargent wrote: > On 2014/04/08 16:53:45, rvargas wrote: > > This cannot be unretained because the stream is allowed to release its > reference > > before calling the callback (in fact, it must), so the buffer has to be > > protected somehow by this code. > > Ok, I guess I was confused by both the code flow and this statement in the > io_buffer.h comments: > > " > // Whenever you call an asynchronous operation that takes an IOBuffer, > // ownership is implicitly transferred to the called function, until the > // operation has completed (at which point it transfers back to the caller). > " yeah, the comment definitely can be improved. > > In this case, the part of the logical call chain of interest is: > > ... > (1) URLRequestJob::ReadRawDataHelper > (2) URLRequestFileJob::ReadRawData > (3) FileStream::Read > (4) (async reading happens and posts a task to run the callback when finished) > (5) URLRequestFileJob::DidRead > ... URLRequestFileJob::OnReadComplete > (6) URLRequestJob::NotifyReadComplete > (7) URLRequestJob::OnRawReadComplete > > > At step (2) URLRequestJob::ReadRawDataHelper, the URLRequestJob sets it's > |raw_read_buffer_| scoped_refptr member variable to hold a reference to the > buffer, and doesn't clear it until URLRequestJob::OnRawReadComplete (or > automatically in the destructor). > > The callback for URLRequestFileJob::DidRead that we pass to FileStream::Read is > bound with a WeakPtr, so unless I missed something it seemed like there isn't > currently any way that we could end up getting URLRequestFileJob::DidRead called > where the buffer isn't still held by the raw_read_buffer_ member variable. Yes, that's true. The issue is that then this code relies on an implementation detail of the caller (not _that_ bad given that it is the parent class). The normal pattern is that callers of Foo(IOBuffer*, size, callback) may or may not keep a reference after the call returns, even if it returns IO_PENDING. On the other hand, the callee receives a naked pointer, and if it returns synchronously doesn't have to do anything... if it needs the buffer after that point (returning IO_PENDING), then it has to store the pointer somewhere and add a ref. The last detail is that the implementation of Foo must release any reference before invoking the callback, because when the callback runs the caller should be able to do whatever it wants to with its buffer. (the same way that the caller is not allowed to touch the buffer at all while the operation is pending, which is what that comment above is trying to state). > > I guess future refactoring in URLRequestFileJob could break this assumption > though? I've changed the interface so that URLRequestFileJob::DidRead takes a > scoped_refptr instead of a naked pointer to the IOBuffer, and then drops its > reference once it's called URLRequestFileJob::OnReadComplete. Let me know if > this seems better to you. > It is better. It may be a little confusing to have a callback receiving a scoped_refptr (if I forget that the method is a completion callback as opposed to something that operates on the buffer... I mean, the callback cannot store the pointer for later). To be honest, that was the first solution I considered when I wrote the comment. Another option would be to have a member variable with the buffer. But I'm happy enough as it is. https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_file_job_unittest.cc (right): https://codereview.chromium.org/227943003/diff/20001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:196: EXPECT_TRUE(delegate_.data_received() == expected_content); On 2014/04/09 20:59:59, Antony Sargent wrote: > On 2014/04/08 16:53:45, rvargas wrote: > > Doesn't this beeps and prints garbage when it fails? > > Nope- notice I used EXPECT_TRUE on single boolean expression instead of > EXPECT_EQ with 2 parameters. I probably had to learn to do this the hard way > already, after locally producing the beeps and garbage you mention. =) > > yes, I missed that! https://codereview.chromium.org/227943003/diff/40001/net/url_request/url_requ... File net/url_request/url_request_file_job_unittest.cc (right): https://codereview.chromium.org/227943003/diff/40001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:110: if (!base::CreateTemporaryFileInDir(directory->path(), path)) doing this makes some of the code above redundant (and the |filename| argument)
+rockot -> please review chrome/browser/extensions/ https://codereview.chromium.org/227943003/diff/40001/net/url_request/url_requ... File net/url_request/url_request_file_job_unittest.cc (right): https://codereview.chromium.org/227943003/diff/40001/net/url_request/url_requ... net/url_request/url_request_file_job_unittest.cc:110: if (!base::CreateTemporaryFileInDir(directory->path(), path)) On 2014/04/09 23:48:50, rvargas wrote: > doing this makes some of the code above redundant (and the |filename| argument) Yep, good catch. I've further simplified it - made the directory required to be created already and passed by const ref, and got rid of the filename argument.
lgtm
Still lgtm
The CQ bit was checked by asargent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asargent@chromium.org/227943003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/extensions/extension_protocols.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
can't find file to patch at input line 6
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: chrome/browser/extensions/extension_protocols.cc
|diff --git a/chrome/browser/extensions/extension_protocols.cc
b/chrome/browser/extensions/extension_protocols.cc
|index
e3e9865cb89f808ed4bd5c33ea136bf0525567ce..5bf3660ee662c0ee110585a2f4d4b8e4d5e04c75
100644
|--- a/chrome/browser/extensions/extension_protocols.cc
|+++ b/chrome/browser/extensions/extension_protocols.cc
--------------------------
No file to patch. Skipping patch.
8 out of 8 hunks ignored
Patch: chrome/browser/extensions/extension_protocols.cc
Index: chrome/browser/extensions/extension_protocols.cc
diff --git a/chrome/browser/extensions/extension_protocols.cc
b/chrome/browser/extensions/extension_protocols.cc
index
e3e9865cb89f808ed4bd5c33ea136bf0525567ce..5bf3660ee662c0ee110585a2f4d4b8e4d5e04c75
100644
--- a/chrome/browser/extensions/extension_protocols.cc
+++ b/chrome/browser/extensions/extension_protocols.cc
@@ -5,6 +5,8 @@
#include "chrome/browser/extensions/extension_protocols.h"
#include <algorithm>
+#include <string>
+#include <vector>
#include "base/base64.h"
#include "base/compiler_specific.h"
@@ -14,6 +16,7 @@
#include "base/logging.h"
#include "base/memory/weak_ptr.h"
#include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
#include "base/metrics/histogram.h"
#include "base/path_service.h"
#include "base/sha1.h"
@@ -33,6 +36,8 @@
#include "chrome/common/url_constants.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/resource_request_info.h"
+#include "crypto/secure_hash.h"
+#include "crypto/sha2.h"
#include "extensions/browser/info_map.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
@@ -45,6 +50,7 @@
#include "extensions/common/manifest_handlers/web_accessible_resources_info.h"
#include "extensions/common/manifest_handlers/webview_info.h"
#include "grit/component_extension_resources_map.h"
+#include "net/base/io_buffer.h"
#include "net/base/mime_util.h"
#include "net/base/net_errors.h"
#include "net/http/http_request_headers.h"
@@ -285,20 +291,31 @@ class URLRequestExtensionJob : public
net::URLRequestFileJob {
const std::string& content_security_policy,
bool send_cors_header,
bool follow_symlinks_anywhere)
- : net::URLRequestFileJob(
- request, network_delegate, base::FilePath(),
- BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
- base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
- directory_path_(directory_path),
- // TODO(tc): Move all of these files into resources.pak so we don't break
- // when updating on Linux.
- resource_(extension_id, directory_path, relative_path),
- content_security_policy_(content_security_policy),
- send_cors_header_(send_cors_header),
- weak_factory_(this) {
+ : net::URLRequestFileJob(
+ request,
+ network_delegate,
+ base::FilePath(),
+
BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
+ base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
+ seek_position_(0),
+ bytes_read_(0),
+ directory_path_(directory_path),
+ // TODO(tc): Move all of these files into resources.pak so we don't
+ // break when updating on Linux.
+ resource_(extension_id, directory_path, relative_path),
+ content_security_policy_(content_security_policy),
+ send_cors_header_(send_cors_header),
+ weak_factory_(this) {
if (follow_symlinks_anywhere) {
resource_.set_follow_symlinks_anywhere();
}
+ const std::string& group =
+ base::FieldTrialList::FindFullName("ExtensionContentHashMeasurement");
+ if (group == "Yes") {
+ base::ElapsedTimer timer;
+ hash_.reset(crypto::SecureHash::Create(crypto::SecureHash::SHA256));
+ hashing_time_ = timer.Elapsed();
+ }
}
virtual void GetResponseInfo(net::HttpResponseInfo* info) OVERRIDE {
@@ -310,7 +327,8 @@ class URLRequestExtensionJob : public net::URLRequestFileJob
{
base::Time* last_modified_time = new base::Time();
bool posted = BrowserThread::PostBlockingPoolTaskAndReply(
FROM_HERE,
- base::Bind(&ReadResourceFilePathAndLastModifiedTime, resource_,
+ base::Bind(&ReadResourceFilePathAndLastModifiedTime,
+ resource_,
directory_path_,
base::Unretained(read_file_path),
base::Unretained(last_modified_time)),
@@ -321,8 +339,35 @@ class URLRequestExtensionJob : public
net::URLRequestFileJob {
DCHECK(posted);
}
+ virtual void OnSeekComplete(int64 result) OVERRIDE {
+ DCHECK_EQ(seek_position_, 0);
+ seek_position_ = result;
+ }
+
+ virtual void OnReadComplete(net::IOBuffer* buffer, int result) OVERRIDE {
+ UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.OnReadCompleteResult", result);
+ if (result > 0) {
+ bytes_read_ += result;
+ if (hash_.get()) {
+ base::ElapsedTimer timer;
+ hash_->Update(buffer->data(), result);
+ hashing_time_ += timer.Elapsed();
+ }
+ }
+ }
+
private:
- virtual ~URLRequestExtensionJob() {}
+ virtual ~URLRequestExtensionJob() {
+ if (hash_.get()) {
+ base::ElapsedTimer timer;
+ std::string hash_bytes(crypto::kSHA256Length, 0);
+ hash_->Finish(string_as_array(&hash_bytes), hash_bytes.size());
+ hashing_time_ += timer.Elapsed();
+ UMA_HISTOGRAM_TIMES("ExtensionUrlRequest.HashTimeMs", hashing_time_);
+ }
+ UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.TotalKbRead", bytes_read_ /
1024);
+ UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.SeekPosition", seek_position_);
+ }
void OnFilePathAndLastModifiedTimeRead(base::FilePath* read_file_path,
base::Time* last_modified_time) {
@@ -334,6 +379,18 @@ class URLRequestExtensionJob : public
net::URLRequestFileJob {
URLRequestFileJob::Start();
}
+ // A hash of the contents we've read from the file.
+ scoped_ptr<crypto::SecureHash> hash_;
+
+ // The position we seeked to in the file.
+ int64 seek_position_;
+
+ // The number of bytes of content we read from the file.
+ int bytes_read_;
+
+ // Used to count the total time it takes to do hashing operations.
+ base::TimeDelta hashing_time_;
+
net::HttpResponseInfo response_info_;
base::FilePath directory_path_;
extensions::ExtensionResource resource_;
The CQ bit was checked by asargent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asargent@chromium.org/227943003/100001
Message was sent while issue was closed.
Change committed as 263571 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
