Browser process changes for Resource Timing sizes.

content/browser/loader/async_resource_handler_unittest.cc

Index: content/browser/loader/async_resource_handler_unittest.cc
diff --git a/content/browser/loader/async_resource_handler_unittest.cc b/content/browser/loader/async_resource_handler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..36da0fe864b2f42d996936341b29a45c960e8f3e
--- /dev/null
+++ b/content/browser/loader/async_resource_handler_unittest.cc
@@ -0,0 +1,394 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/loader/async_resource_handler.h"
+
+#include <stddef.h>
+#include <stdint.h>
+#include <memory>
+#include <string>
+#include <tuple>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/feature_list.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/shared_memory_handle.h"
+#include "base/memory/weak_ptr.h"
+#include "base/process/process.h"
+#include "base/process/process_handle.h"
+#include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_loader.h"
+#include "content/browser/loader/resource_loader_delegate.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/common/resource_messages.h"
+#include "content/common/resource_request.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/content_features.h"
+#include "content/public/common/process_type.h"
+#include "content/public/common/resource_type.h"
+#include "content/public/test/mock_resource_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "ipc/ipc_message.h"
+#include "ipc/ipc_message_macros.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/page_transition_types.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+// This string is repeated as necessary to create a response body of the
+// required size. It is 8 bytes so that repeated doubling can achieve any power
+// of 2 size exactly.
+const char kBaseData[] = "Hello.  ";
+
+// Response short enough to be inlined into a single IPC.
+const GURL kTestURLShort("test:short");
+
+// Response can be sent in a single chunk (not inlined).
+const GURL kTestURLMedium("test:medium");
+
+// Size of response to kTestURLMedium. It should be a power of 2 and larger than
+// kInlinedLeadingChunkSize.
+const size_t kMediumLength = 4096;
+
+// Response requires two chunks.
+const GURL kTestURLLarge("test:large");
+
+// Size of response to kTestURLLarge. It should be a power of 2 and larger than
+// kMaxAllocationSize.
+const size_t kLargeLength = 64 * 1024;
+
+std::string RepeatData(size_t target_size) {
+  std::string data = kBaseData;
+  data.reserve(target_size);
+  while (data.size() < target_size)
+    data += data;
+  return data;
+}

[kinuko, 2016/07/11 02:35:24]

Why can't we use simple method like std::string(target_size, 'a')?

[Adam Rice, 2016/07/11 05:16:36]

Oh. Yes. Sorry. Done.

+
+// This test job adds a Content-Length header and implements
+// GetTotalReceivedBytes().
+class TestJob : public net::URLRequestTestJob {
+ public:
+  TestJob(net::URLRequest* request, net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  // URLRequestJob implementation:

[kinuko, 2016/07/11 02:35:25]

Should we have a TODO to implement this in URLRequestTestJob?

[Adam Rice, 2016/07/11 05:16:36]

Done.

+  int64_t GetTotalReceivedBytes() const override {
+    std::string http_headers = net::HttpUtil::ConvertHeadersBackToHTTPResponse(
+        response_headers_->raw_headers());
+    return http_headers.size() + offset_;
+  }
+
+ protected:
+  // Override URLRequestTestJob behaviour.
+  void StartAsync() override {

[kinuko, 2016/07/11 02:35:25]

I think it's easier to extend / more readable if we use the version of
net::URLRequestTestJob ctor which takes response_headers and response_data
rather than having multiple test-dependent cases in this StartAsync. This
class's ctor can take take target_size and can be more modularized then.  I
imagine something like following:

TestJob(URLRequest*, NetworkDelegate*, size_t response_data_size)
  : net::URLRequestTestJob(request, network_delegate,
                           GenerateHeader(response_data_size),
                           GenerateData(response_data_size),
                           true /* auto_advance */) {}

GenerateHeader can use a simple Stringf format template and returns the raw
header string (it can be one-line StringPrintf method + string constant then).

We can have a static method to do CreateJob, and TestJobProtocolHandler's ctor
can take base::Callback that calls the method with target_size parameter.

Then we can remove all the URL/size constants above and embed them in each test
code... that could possibly actually reduce the # of lines too. Wdyt?

[Adam Rice, 2016/07/11 05:16:36]

That is simpler. Done.

+    std::string data;
+    if (request_->url() == kTestURLShort) {
+      data = kBaseData;
+    } else if (request_->url() == kTestURLMedium) {
+      data = RepeatData(kMediumLength);
+    } else if (request_->url() == kTestURLLarge) {
+      data = RepeatData(kLargeLength);
+    }
+    if (data.empty()) {
+      URLRequestTestJob::StartAsync();
+      return;
+    }
+
+    std::string headers = test_headers();
+    response_headers_ = new net::HttpResponseHeaders(
+        net::HttpUtil::AssembleRawHeaders(headers.data(), headers.size()));
+
+    response_data_ = data;
+    response_headers_->AddHeader("Content-Length: " +
+                                 base::SizeTToString(data.size()));
+    URLRequestTestJob::StartAsync();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestJob);
+};
+
+class TestJobProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new TestJob(request, network_delegate);
+  }
+};
+
+// A subclass of ResourceMessageFilter that records IPC messages that are sent.
+class RecordingResourceMessageFilter : public ResourceMessageFilter {
+ public:
+  RecordingResourceMessageFilter(ResourceContext* resource_context,
+                                 net::URLRequestContext* request_context)
+      : ResourceMessageFilter(
+            0,
+            PROCESS_TYPE_RENDERER,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            base::Bind(&RecordingResourceMessageFilter::GetContexts,
+                       base::Unretained(this))),
+        resource_context_(resource_context),
+        request_context_(request_context) {
+    set_peer_process_for_testing(base::Process::Current());
+  }
+
+  const std::vector<std::unique_ptr<IPC::Message>>& messages() const {
+    return messages_;
+  }
+
+  // IPC::Sender implementation
+  bool Send(IPC::Message* message) override {
+    if (message->type() == ResourceMsg_SetDataBuffer::ID)
+      ConsumeHandle(*message);
+    messages_.push_back(base::WrapUnique(message));
+    return true;
+  }
+
+ private:
+  ~RecordingResourceMessageFilter() override {}
+
+  void GetContexts(ResourceType resource_type,
+                   int origin_pid,
+                   ResourceContext** resource_context,
+                   net::URLRequestContext** request_context) {
+    *resource_context = resource_context_;
+    *request_context = request_context_;
+  }
+
+  // Unpickle the base::SharedMemoryHandle to avoid warnings about
+  // "MessageAttachmentSet destroyed with unconsumed descriptors".
+  void ConsumeHandle(const IPC::Message& message) {
+    IPC_BEGIN_MESSAGE_MAP(RecordingResourceMessageFilter, message)
+      IPC_MESSAGE_HANDLER(ResourceMsg_SetDataBuffer, OnSetDataBuffer)
+      IPC_MESSAGE_UNHANDLED(NOTREACHED())
+    IPC_END_MESSAGE_MAP()

[kinuko, 2016/07/11 02:35:25]

Isn't it more compact / easier to read if we just write in Send():

// Unpickle the base::SharedMemoryHandle to warnings about
// "MessageAttachmentSet destroyed with unconsumed descriptors".
if (message->type() == ResourceMsg_SetDataBuffer::ID) {
  ResourceMsg_SetDataBuffer::Param params;
  ResourceMsg_SetDataBuffer::Read(message, &params);
}

?

[Adam Rice, 2016/07/11 05:16:36]

I didn't think that would work. I should have tried it. It does work. Thank you.
Done.

+  }
+
+  void OnSetDataBuffer(int request_id,
+                       base::SharedMemoryHandle shm_handle,
+                       int shm_size,
+                       base::ProcessId renderer_pid) {}
+
+  ResourceContext* const resource_context_;
+  net::URLRequestContext* const request_context_;
+  std::vector<std::unique_ptr<IPC::Message>> messages_;
+};
+
+class AsyncResourceHandlerTest : public ::testing::Test {
+ protected:
+  AsyncResourceHandlerTest()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP), context_(true) {}
+
+  void CreateRequest(const GURL& url) {
+    test_job_factory_.SetProtocolHandler(
+        "test", base::MakeUnique<TestJobProtocolHandler>());
+    context_.set_job_factory(&test_job_factory_);
+    context_.Init();
+    std::unique_ptr<net::URLRequest> request =
+        context_.CreateRequest(GURL(url), net::DEFAULT_PRIORITY, nullptr);
+    resource_context_ = base::MakeUnique<MockResourceContext>(&context_);
+    filter_ = make_scoped_refptr(
+        new RecordingResourceMessageFilter(resource_context_.get(), &context_));
+    ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl(

[Mike West, 2016/07/08 13:14:37]

Nit: Wrap this in `// clang format off` at the top and `// clang format on` at
the bottom; otherwise `git cl format` will make you sad.

[Adam Rice, 2016/07/11 02:07:59]

Actually, the comments on each line are sufficient to stop clang-format from
trying to be clever.

[kinuko, 2016/07/11 02:35:24]

Note that ResourceRequestInfo::AllocateForTesting method has an explicit comment
like 'NOTE: Add more parameters if you need to initialize other fields', so I
think we can try adding the filter thing to it too. (No other test code seems to
directly use this ctor)

(As it requires more changes in other test code it can be done in a separate CL
too)

[Adam Rice, 2016/07/11 05:16:36]

ResourceRequestInfo::AllocateForTesting() only sets fields which are exposed by
ResourceRequestInfo. This doesn't include the ResourceContext field. This makes
sense from an encapsulation point of view.

I thought about adding a ResourceRequestInfoImpl::AllocateForTesting() function,
but I concluded it would be better just to simplify the ResourceRequestInfoImpl
constructor.

[kinuko, 2016/07/11 06:43:38]

I see... then we probably need to live with this lengthy ctor.

+        PROCESS_TYPE_RENDERER,                 // process_type
+        0,                                     // child_id
+        0,                                     // route_id
+        -1,                                    // frame_tree_node_id
+        0,                                     // origin_pid
+        0,                                     // request_id
+        0,                                     // render_frame_id
+        false,                                 // is_main_frame
+        false,                                 // parent_is_main_frame
+        RESOURCE_TYPE_IMAGE,                   // resource_type
+        ui::PAGE_TRANSITION_LINK,              // transition_type
+        false,                                 // should_replace_current_entry
+        false,                                 // is_download
+        false,                                 // is_stream
+        false,                                 // allow_download
+        false,                                 // has_user_gesture
+        false,                                 // enable load timing
+        false,                                 // enable upload progress
+        false,                                 // do_not_prompt_for_login
+        blink::WebReferrerPolicyDefault,       // referrer_policy
+        blink::WebPageVisibilityStateVisible,  // visibility_state
+        resource_context_.get(),               // context
+        filter_->GetWeakPtr(),                 // filter
+        false,                                 // report_raw_headers
+        true,                                  // is_async
+        false,                                 // is_using_lofi
+        std::string(),                         // original_headers
+        nullptr);                              // body
+    info->AssociateWithRequest(request.get());
+    std::unique_ptr<AsyncResourceHandler> handler =
+        base::MakeUnique<AsyncResourceHandler>(request.get(), &rdh_);
+    loader_delegate_.reset(new TrivialResourceLoaderDelegate(this));
+    loader_ =
+        base::MakeUnique<ResourceLoader>(std::move(request), std::move(handler),
+                                         nullptr, loader_delegate_.get());
+  }
+
+  void StartRequest() { loader_->StartRequest(); }
+
+  void WaitForFinish() { finish_waiter_.Run(); }

[kinuko, 2016/07/11 02:35:25]

StartRequest and WaitForFinish are called only from CreateStartAndWait, I don't
feel we need to have these methods separately

[Adam Rice, 2016/07/11 05:16:36]

Done.

+
+  void CreateStartAndWait(const GURL& url) {
+    CreateRequest(url);
+    StartRequest();
+    WaitForFinish();
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl rdh_;
+  net::TestURLRequestContext context_;
+  net::URLRequestJobFactoryImpl test_job_factory_;
+  std::unique_ptr<MockResourceContext> resource_context_;
+  scoped_refptr<RecordingResourceMessageFilter> filter_;
+  std::unique_ptr<ResourceLoader> loader_;
+  base::RunLoop finish_waiter_;

[kinuko, 2016/07/11 02:35:24]

Looks like all fields other than filter_ can be actually private?

[Adam Rice, 2016/07/11 05:16:36]

Done.

+
+ private:
+  class TrivialResourceLoaderDelegate : public ResourceLoaderDelegate {

[kinuko, 2016/07/11 02:35:24]

I think AsyncResourceHandlerTest can directly implement ResourceLoaderDelegate? 
Then we can remove 'fixture_' indirection in DidFinishLoading and can remove
loader_delegate_ field from the test class.

[Adam Rice, 2016/07/11 05:16:36]

Done.

+   public:
+    explicit TrivialResourceLoaderDelegate(AsyncResourceHandlerTest* fixture)
+        : fixture_(fixture) {}
+    ResourceDispatcherHostLoginDelegate* CreateLoginDelegate(
+        ResourceLoader* loader,
+        net::AuthChallengeInfo* auth_info) override {
+      return nullptr;
+    }
+
+    bool HandleExternalProtocol(ResourceLoader* loader,
+                                const GURL& url) override {
+      return false;
+    }
+    void DidStartRequest(ResourceLoader* loader) override {}
+    void DidReceiveRedirect(ResourceLoader* loader,
+                            const GURL& new_url) override {}
+    void DidReceiveResponse(ResourceLoader* loader) override {}
+    void DidFinishLoading(ResourceLoader* loader) override {
+      fixture_->DidFinishLoading();
+    }
+
+   private:
+    AsyncResourceHandlerTest* fixture_;
+  };
+
+  void DidFinishLoading() {
+    loader_.reset();
+    finish_waiter_.Quit();
+  }
+
+  std::unique_ptr<TrivialResourceLoaderDelegate> loader_delegate_;
+};
+
+std::unique_ptr<ResourceMsg_DataReceived::Param> UnpackDataReceivedIPC(
+    const IPC::Message* msg) {
+  if (ResourceMsg_DataReceived::ID != msg->type())
+    return nullptr;
+  std::unique_ptr<ResourceMsg_DataReceived::Param> params =
+      base::MakeUnique<ResourceMsg_DataReceived::Param>();
+  ResourceMsg_DataReceived::Read(msg, params.get());
+  return params;
+}
+
+std::unique_ptr<ResourceMsg_InlinedDataChunkReceived::Param>
+UnpackInlinedDataChunkReceivedIPC(const IPC::Message* msg) {
+  if (ResourceMsg_InlinedDataChunkReceived::ID != msg->type())
+    return nullptr;
+  std::unique_ptr<ResourceMsg_InlinedDataChunkReceived::Param> params =
+      base::MakeUnique<ResourceMsg_InlinedDataChunkReceived::Param>();
+  ResourceMsg_InlinedDataChunkReceived::Read(msg, params.get());
+  return params;
+}
+
+TEST_F(AsyncResourceHandlerTest, Construct) {
+  CreateRequest(net::URLRequestTestJob::test_url_1());
+}
+
+TEST_F(AsyncResourceHandlerTest, OneChunkLengths) {
+  CreateStartAndWait(kTestURLMedium);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(4u, messages.size());
+  auto params = UnpackDataReceivedIPC(messages[2].get());
+  ASSERT_TRUE(params);

[kinuko, 2016/07/11 02:35:24]

I have a feeling that these two lines can be replaced with 

ResourceMessage_DataReceived::Param params;
ASSERT_TRUE(ResourceMsg_DataReceived::Read(messages[2].get, &params);

here and following.  Then we can remove UnpackX methods at all.

[Adam Rice, 2016/07/11 05:16:36]

Read() doesn't check the message type, so I had to add ASSERTs for that.
Apart from that, this works.

+
+  int encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(4163, encoded_data_length);
+  int encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(4096, encoded_body_length);
+}
+
+TEST_F(AsyncResourceHandlerTest, InlinedChunkLengths) {
+  // TODO(ricea): Remove this Feature-enabling code once the feature is on by
+  // default.
+  auto feature_list = base::MakeUnique<base::FeatureList>();
+  feature_list->InitializeFromCommandLine(
+      features::kOptimizeLoadingIPCForSmallResources.name, "");
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(std::move(feature_list));
+
+  CreateStartAndWait(kTestURLShort);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(3u, messages.size());
+  auto params = UnpackInlinedDataChunkReceivedIPC(messages[1].get());
+  ASSERT_TRUE(params);
+
+  int encoded_data_length = std::get<2>(*params);
+  EXPECT_EQ(72, encoded_data_length);
+  int encoded_body_length = std::get<3>(*params);
+  EXPECT_EQ(8, encoded_body_length);
+
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(base::MakeUnique<base::FeatureList>());
+}
+
+TEST_F(AsyncResourceHandlerTest, TwoChunksLengths) {
+  CreateStartAndWait(kTestURLLarge);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(5u, messages.size());
+  auto params = UnpackDataReceivedIPC(messages[2].get());
+  ASSERT_TRUE(params);
+
+  int encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(32836, encoded_data_length);
+  int encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(32768, encoded_body_length);
+
+  params = UnpackDataReceivedIPC(messages[3].get());
+  ASSERT_TRUE(params);
+  encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(32768, encoded_data_length);
+  encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(32768, encoded_body_length);
+}
+
+}  // namespace
+
+}  // namespace content