Implement base::win::IsMetroProcess.

Implement base::win::IsMetroProcess. 
Change GetMetroModule callers that don't need the HMODULE.

TBR=stevenjb@chromium.org,brettw@chromium.org,mirandac@chromium.org,sky@chromium.org,kalman@chromium.org

BUG=None
TEST=None

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

[Sigurður Ásgeirsson, 2012-06-15 21:31:53]

Please take a look.
Ananta please look at the implementation.
I've TBR-ed owners as this is a trivial refactoring, so at your leisure.

[ananta, 2012-06-15 21:46:19]

LGTM

https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc
File base/win/metro.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc#newc...
base/win/metro.cc:42: state = kImmersiveTrue;
add a DCHECK(GetMetroModule() != NULL) ?

https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1808: if (base::win::InMetroMode()
!= NULL)
remove != NULL

[Sigurður Ásgeirsson, 2012-06-18 13:54:15]

The assert you asked for seems to be firing inside renderers. Using
IsImmersiveProcess will change the semantics for any caller in a renderer, as
presumably the renderers aren't loading metro_driver now.
I'm not sure whether that's desirable - thoughts?

(I'm about to debug the assert...)

https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc
File base/win/metro.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc#newc...
base/win/metro.cc:42: state = kImmersiveTrue;
On 2012/06/15 21:46:19, ananta wrote:
> add a DCHECK(GetMetroModule() != NULL) ?

Done.

https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:1808: if (base::win::InMetroMode()
!= NULL)
On 2012/06/15 21:46:19, ananta wrote:
> remove != NULL

Done.

https://chromiumcodereview.appspot.com/10560015/diff/6001/base/win/metro.cc
File base/win/metro.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/6001/base/win/metro.cc#n...
base/win/metro.cc:42: DCHECK(GetMetroModule() != NULL);
This assert is apparently firing inside the renderers, I don't understand why
that would be happening (about to debug), but presumably we want the same answer
to the "InMetroMode" question in renders and browsers?

[robertshield, 2012-06-18 15:06:39]

On 2012/06/18 13:54:15, Sigurður Ásgeirsson wrote:
> The assert you asked for seems to be firing inside renderers. Using
> IsImmersiveProcess will change the semantics for any caller in a renderer, as
> presumably the renderers aren't loading metro_driver now.
> I'm not sure whether that's desirable - thoughts?
> 
> (I'm about to debug the assert...)
> 
> https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc
> File base/win/metro.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10560015/diff/1/base/win/metro.cc#newc...
> base/win/metro.cc:42: state = kImmersiveTrue;
> On 2012/06/15 21:46:19, ananta wrote:
> > add a DCHECK(GetMetroModule() != NULL) ?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10560015/diff/1/chrome/browser/ui/view...
> chrome/browser/ui/views/frame/browser_view.cc:1808: if
(base::win::InMetroMode()
> != NULL)
> On 2012/06/15 21:46:19, ananta wrote:
> > remove != NULL
> 
> Done.
> 
> https://chromiumcodereview.appspot.com/10560015/diff/6001/base/win/metro.cc
> File base/win/metro.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10560015/diff/6001/base/win/metro.cc#n...
> base/win/metro.cc:42: DCHECK(GetMetroModule() != NULL);
> This assert is apparently firing inside the renderers, I don't understand why
> that would be happening (about to debug), but presumably we want the same
answer
> to the "InMetroMode" question in renders and browsers?


I'd consider renaming this to "IsMetroProcess" for the time being - since for
now this check referes to whether we are in a metro application. Long term,
metro-y-ness will instead become a property of a Browser/TabContainer.

I am vehemently opposed to the proliferation of IsMetroProcess() checks,
especially in code that doesn't currently live in the browser process. IMO, we
should keep all knowledge of Metro vs Desktop in the browser process to make
things much easier for ourselves in the future. 

Where renderers need to behave differently (e.g. no plugins, touch UI, etc),
they should have this information passed explicitly by the browser process. (Run
the renderer process with --touch-optimized-ui, --no-windowed-plugins, etc.).

[Sigurður Ásgeirsson, 2012-06-18 15:23:17]

Renamed InMetroMode to IsMetroProcess, implemented caching and a DCHECK in
GetMetroModule. Please take another quick look.

[Sigurður Ásgeirsson, 2012-06-18 15:26:12]

Mark, can you please review and approve base change as owner there? The rest is
all trivial refactoring that I'll submit TBR.

[Mark Mentovai, 2012-06-18 15:40:22]

LGTM for base OWNERS approval, but for code review purposes I have an optional
but highly recommended suggestion that you may want to consider.

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc
File base/win/metro.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:15: const HMODULE kNoMetroDriver =
reinterpret_cast<HMODULE>(1);
Optional but highly recommended:

I think that this is kind of hackish.

I’ve been encouraging people to write helper functions and use them in static
initializers, leveraging the automatic guard variable to prevent multiple
initialization. That’d look like:

static HMODULE GetMetroModuleInternal() {  // or anonymous namespace
  HMODULE metro_module = GetModuleHandleA("metro_driver.dll");
  if (metro_module) {
    DCHECK(IsMetroProcess());
  }
  return metro_module;
}

HMODULE GetMetroModule() {
  static HMODULE metro_module = GetMetroModuleInternal();
  return metro_module;
}

Less source code, less object code, and a clearer design too. Win-win!

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:24: // We must be in a metro process if the metro_driver is
loaded.
All four of your comments in this file say “we,” I prefer clearer and more
direct prose by avoiding personal pronouns.

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:37: bool IsMetroProcess() {
I’d follow the same pattern for this function too, which lets you give up the
enum in favor of a simple bool.

[Sigurður Ásgeirsson, 2012-06-18 18:51:13]

Thanks, addressed your comments.

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc
File base/win/metro.cc (right):

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:15: const HMODULE kNoMetroDriver =
reinterpret_cast<HMODULE>(1);
Sadly, as we discussed, VS does not implement thread-safe initialization of
local statics, so this would lead to a race here.
For posterity, I compiled this stub with VS2010: 

extern int foo();
int bar() {
  static int cache = foo();

  return cache;
}

and got the below thread-unsafe object code.

?bar@@YAHXZ (int __cdecl bar(void)):
  00000000: B8 01 00 00 00     mov         eax,1
  00000005: 84 05 00 00 00 00  test        byte ptr [?$S1@?1??bar@@YAHXZ@4IA],al

  0000000B: 75 11              jne         0000001E
  0000000D: 09 05 00 00 00 00  or          dword ptr
[?$S1@?1??bar@@YAHXZ@4IA],eax
  00000013: E8 00 00 00 00     call        ?foo@@YAHXZ
  00000018: A3 00 00 00 00     mov         dword ptr
[?cache@?1??bar@@YAHXZ@4HA],eax
  0000001D: C3                 ret
  0000001E: A1 00 00 00 00     mov         eax,dword ptr
[?cache@?1??bar@@YAHXZ@4HA]
  00000023: C3                 ret

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:24: // We must be in a metro process if the metro_driver is
loaded.
On 2012/06/18 15:40:22, Mark Mentovai wrote:
> All four of your comments in this file say “we,” I prefer clearer and more
> direct prose by avoiding personal pronouns.

Done.

https://chromiumcodereview.appspot.com/10560015/diff/3002/base/win/metro.cc#n...
base/win/metro.cc:37: bool IsMetroProcess() {
On 2012/06/18 15:40:22, Mark Mentovai wrote:
> I’d follow the same pattern for this function too, which lets you give up the
> enum in favor of a simple bool.

Same issue, sadly.

[commit-bot: I haz the power, 2012-06-18 18:52:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/10560015/3030

[commit-bot: I haz the power, 2012-06-18 18:52:54]

Failed to apply patch for chrome/browser/ui/toolbar/wrench_menu_model.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/toolbar/wrench_menu_model.cc
Hunk #1 FAILED at 176.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/toolbar/wrench_menu_model.cc.rej

[commit-bot: I haz the power, 2012-06-18 19:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/10560015/2005

[commit-bot: I haz the power, 2012-06-18 20:40:08]

Change committed as 142801