Implemented multisampling control in software rendering list. Move prelimina...

chrome/browser/gpu_data_manager.cc

Index: chrome/browser/gpu_data_manager.cc
===================================================================
--- chrome/browser/gpu_data_manager.cc	(revision 76655)
+++ chrome/browser/gpu_data_manager.cc	(working copy)
@@ -13,21 +13,24 @@
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/gpu/gpu_info_collector.h"
 #include "content/browser/gpu_blacklist.h"
 #include "grit/browser_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 
 GpuDataManager::GpuDataManager()
         : gpu_feature_flags_set_(false),
+          gpu_feature_flags_applied_(false),
           gpu_blacklist_cache_(NULL) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(g_browser_process);
-  PrefService* prefs = g_browser_process->local_state();
+  PrefService* local_state = g_browser_process->local_state();
   // If we bring up chrome normally, prefs should never be NULL; however, we
   // we handle the case where prefs == NULL for certain tests.
-  if (prefs) {
-    prefs->RegisterDictionaryPref(prefs::kGpuBlacklist);
-    gpu_blacklist_cache_ = prefs->GetMutableDictionary(prefs::kGpuBlacklist);
+  if (local_state) {
+    local_state->RegisterDictionaryPref(prefs::kGpuBlacklist);
+    gpu_blacklist_cache_ =
+        local_state->GetMutableDictionary(prefs::kGpuBlacklist);
     DCHECK(gpu_blacklist_cache_);
 
     gpu_blacklist_updater_ = new GpuBlacklistUpdater();
@@ -37,6 +40,36 @@
 
   LoadGpuBlacklist();
   UpdateGpuBlacklist();
+
+  GPUInfo gpu_info;
+  gpu_info_collector::CollectPreliminaryGraphicsInfo(&gpu_info);
+  UpdateGpuInfo(gpu_info);
+  UpdateGpuFeatureFlags();
+
+  uint32 flags = gpu_feature_flags_.flags();

[apatrick_chromium, 2011/03/03 21:09:07]

Just to check, the flag being set means the feature is blacklisted? If that is
the case, it would be clearer if this was called blacklisted_features_ or
similar. Another CL.

[Zhenyao Mo, 2011/03/04 00:24:59]

Yes and will fix this in another CL after M11.

+  CommandLine* browser_command_line = CommandLine::ForCurrentProcess();
+  DCHECK(browser_command_line);
+  if ((flags & GpuFeatureFlags::kGpuFeatureWebgl) &&
+      !browser_command_line->HasSwitch(switches::kDisableExperimentalWebGL))
+    browser_command_line->AppendSwitch(switches::kDisableExperimentalWebGL);

[apatrick_chromium, 2011/03/03 21:09:07]

It seems strange to modify the browser process command line, especially as a
side effect of constructing an object. I don't see precedent for doing this
elsewhere in Chrome. Would it be possible to have a functions on GpuDataManager
that return whether WebGL and multisampling are enabled and use them rather than
HasSwitch(switches::kDisable...) elsewhere in the code?

[Zhenyao Mo, 2011/03/04 00:24:59]

Per our offline discussion, will move this commandline flags manipulation to
render process startup.

+  if ((flags & GpuFeatureFlags::kGpuFeatureMultisampling) &&
+      !browser_command_line->HasSwitch(switches::kDisableGLMultisampling))
+    browser_command_line->AppendSwitch(switches::kDisableGLMultisampling);
+  // If we have kGpuFeatureAcceleratedCompositing, we disable all GPU features.
+  if (flags & GpuFeatureFlags::kGpuFeatureAcceleratedCompositing) {
+    const char* switches[] = {
+        switches::kDisableAcceleratedCompositing,
+        switches::kDisableAcceleratedLayers,
+        switches::kDisableAcceleratedVideo,
+        switches::kDisableExperimentalWebGL
+    };
+    const int switch_count = sizeof(switches) / sizeof(char*);
+    for (int i = 0; i < switch_count; ++i) {
+      if (!browser_command_line->HasSwitch(switches[i]))
+        browser_command_line->AppendSwitch(switches[i]);
+    }
+  }
+  gpu_feature_flags_applied_ = true;
 }
 
 GpuDataManager::~GpuDataManager() { }
@@ -66,6 +99,8 @@
 GpuFeatureFlags GpuDataManager::GetGpuFeatureFlags() {

[apatrick_chromium, 2011/03/03 21:09:07]

Could this be GetBlacklistedFeatureFlags()? Another CL.

[Zhenyao Mo, 2011/03/04 00:24:59]

Will do after M11.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UpdateGpuFeatureFlags();
+  if (gpu_feature_flags_applied_)
+    return GpuFeatureFlags();

[apatrick_chromium, 2011/03/03 21:09:07]

Why does this return that no features are blacklisted when the command line
accurately reflects the features that are disabled?

[Zhenyao Mo, 2011/03/04 00:24:59]

This is because certain features are already disabled independently through
commandline flags.  Here we don't wanna return them again because the caller
will refuse GPU channel.  I'll think of a way to make this clearer.

   return gpu_feature_flags_;
 }
 
@@ -155,8 +190,15 @@
   gpu_feature_flags_.set_flags(0);
 
   if (gpu_blacklist != NULL) {
-    gpu_feature_flags_ = gpu_blacklist->DetermineGpuFeatureFlags(
+    GpuFeatureFlags flags = gpu_blacklist->DetermineGpuFeatureFlags(
         GpuBlacklist::kOsAny, NULL, gpu_info_);
+    // If any new bit is set, we need to apply flags again.
+    if (gpu_feature_flags_applied_ &&
+        ((~(gpu_feature_flags_.flags())) & flags.flags())) {
+      gpu_feature_flags_applied_ = false;
+      // TODO(zmo): need to ask the user to restart chrome.

[apatrick_chromium, 2011/03/03 21:09:07]

Why can't deal with this by terminating the GPU process and reporting all
contexts as lost and refusing to restart it? We can recover from that. It
doesn't seem necessary to put the user through the inconvenience of restarting
the whole browser.

[Zhenyao Mo, 2011/03/04 00:24:59]

At the moment renderer can't recover and go through a software rendering path if
it hasn't begin with it.  We definitely should make this possible though.  Just
not in this CL.  As for asking users to restart, seems like it's not really
necessary.  Will remove the comments.

[jamesr, 2011/03/04 00:28:14]

I thought Alexey fixed this (or possibly is still in the process of fixing it). 
Can you check with him?

+    }
+    gpu_feature_flags_ = flags;
     uint32 max_entry_id = gpu_blacklist->max_entry_id();
     if (gpu_feature_flags_.flags() != 0) {
       // If gpu is blacklisted, no further GPUInfo will be collected.