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

Move preliminary gpu info collection and software rendering flags computation to browser startup time.  This allows us to handle each feature independently through appending renderer process commandline switches (in most cases but not all, because this preliminary run only collects partial gpu info).

BUG=none
TEST=unittest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76971

[Zhenyao Mo, 2011-03-03 01:52:52]

Please review.  I am hoping to get this into M11, so I'd really appreciate it if
it can be reviewed by Friday night.  Thanks a lot!

[apatrick_chromium, 2011-03-03 21:09:06]

I added jamesr to see if he has any suggestions. A few thoughts...

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
File chrome/browser/gpu_data_manager.cc (right):

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:49: uint32 flags =
gpu_feature_flags_.flags();
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.

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:54:
browser_command_line->AppendSwitch(switches::kDisableExperimentalWebGL);
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?

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:99: GpuFeatureFlags
GpuDataManager::GetGpuFeatureFlags() {
Could this be GetBlacklistedFeatureFlags()? Another CL.

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:103: return GpuFeatureFlags();
Why does this return that no features are blacklisted when the command line
accurately reflects the features that are disabled?

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:199: // TODO(zmo): need to ask the user to
restart chrome.
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.

http://codereview.chromium.org/6588138/diff/7001/chrome/common/gpu_messages_i...
File chrome/common/gpu_messages_internal.h (left):

http://codereview.chromium.org/6588138/diff/7001/chrome/common/gpu_messages_i...
chrome/common/gpu_messages_internal.h:138:
IPC_SYNC_MESSAGE_CONTROL1_1(GpuHostMsg_PreliminaryGraphicsInfoCollected,
I'm relieved this is deleted. For future reference, the GPU process must never
send a synchronous IPC message to the browser process. It could lead to a
deadlock because the browser process now sends sync messages to the GPU process.

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

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
File chrome/browser/gpu_data_manager.cc (right):

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:49: uint32 flags =
gpu_feature_flags_.flags();
On 2011/03/03 21:09:07, apatrick_chromium wrote:
> 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.

Yes and will fix this in another CL after M11.

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:54:
browser_command_line->AppendSwitch(switches::kDisableExperimentalWebGL);
On 2011/03/03 21:09:07, apatrick_chromium wrote:
> 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?

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

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:99: GpuFeatureFlags
GpuDataManager::GetGpuFeatureFlags() {
On 2011/03/03 21:09:07, apatrick_chromium wrote:
> Could this be GetBlacklistedFeatureFlags()? Another CL.

Will do after M11.

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:103: return GpuFeatureFlags();
On 2011/03/03 21:09:07, apatrick_chromium wrote:
> Why does this return that no features are blacklisted when the command line
> accurately reflects the features that are disabled?

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.

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:199: // TODO(zmo): need to ask the user to
restart chrome.
On 2011/03/03 21:09:07, apatrick_chromium wrote:
> 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.

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]

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
File chrome/browser/gpu_data_manager.cc (right):

http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
chrome/browser/gpu_data_manager.cc:199: // TODO(zmo): need to ask the user to
restart chrome.
On 2011/03/04 00:24:59, Zhenyao Mo wrote:
> On 2011/03/03 21:09:07, apatrick_chromium wrote:
> > 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.
> 
> 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.

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

[zmo, 2011-03-04 00:32:17]

On 2011/03/04 00:28:14, jamesr wrote:
>
http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
> File chrome/browser/gpu_data_manager.cc (right):
> 
>
http://codereview.chromium.org/6588138/diff/7001/chrome/browser/gpu_data_mana...
> chrome/browser/gpu_data_manager.cc:199: // TODO(zmo): need to ask the user to
> restart chrome.
> On 2011/03/04 00:24:59, Zhenyao Mo wrote:
> > On 2011/03/03 21:09:07, apatrick_chromium wrote:
> > > 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.
> > 
> > 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.
> 
> I thought Alexey fixed this (or possibly is still in the process of fixing
it). 
> Can you check with him?

Sorry about the confusion.  What I meant is that for compositing (and maybe
video), if it goes down the GPU path, currently it can't fail and go back to the
software rendering path.  That's what I heard from Vangelis.  It needs some
major revision to make it possible to switch paths in the middle.

[Zhenyao Mo, 2011-03-04 02:07:43]

Revised.  Please review again.

[apatrick_chromium, 2011-03-04 17:39:39]

LGTM

[Zhenyao Mo, 2011-03-04 18:24:25]

On 2011/03/04 17:39:39, apatrick_chromium wrote:
> LGTM

I just made some more changes to the CL: now if we notice blacklist flags
change, we shut down all existing GPU channels.

Could you have a look again?

Sorry to make changes after you already reviewed.

[zmo, 2011-03-04 18:31:06]

OK don't bother.  I'll land the original CL (patch set 2) first.  Then create
another CL for the further change.

Thank you.

On 2011/03/04 18:24:25, Zhenyao Mo wrote:
> On 2011/03/04 17:39:39, apatrick_chromium wrote:
> > LGTM
> 
> I just made some more changes to the CL: now if we notice blacklist flags
> change, we shut down all existing GPU channels.
> 
> Could you have a look again?
> 
> Sorry to make changes after you already reviewed.