Store PreRead options obtained from the registry in a global variable.

chrome/app/main_dll_loader_win.cc

Index: chrome/app/main_dll_loader_win.cc
diff --git a/chrome/app/main_dll_loader_win.cc b/chrome/app/main_dll_loader_win.cc
index a70a6787d1504d0f2ce48b52ae09ce9325ac2f9b..c959114371ec3f3dfe7369463c87e4398e1daf21 100644
--- a/chrome/app/main_dll_loader_win.cc
+++ b/chrome/app/main_dll_loader_win.cc
@@ -64,35 +64,34 @@ HMODULE LoadModuleWithDirectory(const base::FilePath& module) {
   ::SetCurrentDirectoryW(module.DirName().value().c_str());
 
   // Get pre-read options from the PreRead field trial.
-  bool trial_no_pre_read = false;
-  bool trial_high_priority = false;
-  bool trial_only_if_cold = false;
-  bool trial_prefetch_virtual_memory = false;
-  startup_metric_utils::GetPreReadOptions(
-      BrowserDistribution::GetDistribution()->GetRegistryPath(),
-      &trial_no_pre_read, &trial_high_priority, &trial_only_if_cold,
-      &trial_prefetch_virtual_memory);
+  startup_metric_utils::InitializePreReadOptions(
+      BrowserDistribution::GetDistribution()->GetRegistryPath());
+  const int pre_read_options = startup_metric_utils::GetPreReadOptions();
 
   // Pre-read the binary to warm the memory caches (avoids a lot of random IO).
-  if (!trial_no_pre_read) {
+  if (!(pre_read_options & startup_metric_utils::PRE_READ_OPTION_NO_PRE_READ)) {

[grt (UTC plus 2), 2016/01/21 04:24:00]

i think this would be easier to read if pre_read_option was actually something
like:
struct PreReadOptions {
  bool no_pre_read : 1;
  bool high_priority : 1;
  ...
};
wdyt?

[fdoray, 2016/01/21 16:04:16]

Done. Except that I didn't use :1 in the struct -> if I did, it wouldn't be
possible to take the address of members like I'm doing now in
InitializePreReadOptions [1]. If you really want me to use :1, I could change
InitializePreReadOptions so that it doesn't need to take the address of members.

[1] https://msdn.microsoft.com/en-us/library/yszfawxh.aspx

[grt (UTC plus 2), 2016/01/21 18:15:42]

I find the data-driven loop in InitializePreReadOptions interesting, but not so
compelling that it should dictate the overall interface. Four calls to ReadDW
(or a tiny helper function) is quite clear, whereas the loop takes a little
reading to validate. I'm okay with it as-is, just pointing out that a clean
interface is more important than a clever implementation. :-)

If you'd like to keep the loop, then avoiding the bitfield is fine. In either
case, I think the struct should be passed by value rather than by reference. The
compiler is potentially smart enough to pack the four bools into a 32-bit int.

[gab, 2016/01/21 19:49:27]

FWIW, I think the loop is actually more readable than duplicating the same 5-6
lines of code 4+ times. The loop approach says "here's what I'm doing" from
"this data" instead of having to make sure that the duplicated code has its own
variable names (it could re-use but then you lose the DCHECKs) and properly
replaces all bits of the duplicated code to point to its own things.

I value compact/readable code more than saving a few bits (especially if the
compiler is likely to optimize it out anyways).

[fdoray, 2016/01/21 21:16:08]

I kept the loop, as preferred by gab@.

     base::ThreadPriority previous_priority = base::ThreadPriority::NORMAL;
-    if (trial_high_priority) {
+    if (pre_read_options &
+        startup_metric_utils::PRE_READ_OPTION_HIGH_PRIORITY) {
       previous_priority = base::PlatformThread::GetCurrentThreadPriority();
       base::PlatformThread::SetCurrentThreadPriority(
           base::ThreadPriority::DISPLAY);
     }
 
-    if (trial_only_if_cold) {
+    if (pre_read_options & startup_metric_utils::PRE_READ_OPTION_ONLY_IF_COLD) {
       base::MemoryMappedFile module_memory_map;
       const bool map_initialize_success = module_memory_map.Initialize(module);
       DCHECK(map_initialize_success);
       if (!IsMemoryMappedFileWarm(module_memory_map)) {
-        if (trial_prefetch_virtual_memory)
+        if (pre_read_options &
+            startup_metric_utils::PRE_READ_OPTION_PREFETCH_VIRTUAL_MEMORY) {
           PreReadMemoryMappedFile(module_memory_map, module);
-        else
+        } else {
           PreReadFile(module);
+        }
       }
-    } else if (trial_prefetch_virtual_memory) {
+    } else if (pre_read_options &
+               startup_metric_utils::PRE_READ_OPTION_PREFETCH_VIRTUAL_MEMORY) {
       base::MemoryMappedFile module_memory_map;
       const bool map_initialize_success = module_memory_map.Initialize(module);
       DCHECK(map_initialize_success);
@@ -101,7 +100,7 @@ HMODULE LoadModuleWithDirectory(const base::FilePath& module) {
       PreReadFile(module);
     }
 
-    if (trial_high_priority)
+    if (pre_read_options & startup_metric_utils::PRE_READ_OPTION_HIGH_PRIORITY)
       base::PlatformThread::SetCurrentThreadPriority(previous_priority);
   }