Pass the device scale factor in the command line to renderer processes.

Pass the device scale factor in the command line to renderer processes.

This is to ensure that we don't use GDI to query the device scale factor in renderer processes.
This is in preparation for setting the MITIGATION_WIN32K_DISABLE policy on renderer processes, which
will lock them out from making user32/gdi32 calls which enter win32k.sys.

BUG=365160
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274766

[ananta, 2014-06-03 01:04:01]

https://codereview.chromium.org/308003020/diff/1/ui/gfx/win/dpi.cc
File ui/gfx/win/dpi.cc (left):

https://codereview.chromium.org/308003020/diff/1/ui/gfx/win/dpi.cc#oldcode175
ui/gfx/win/dpi.cc:175: DCHECK_NE(0.0f, g_device_scale_factor);
Removed this DCHECK as we need to call this to see if it has been set which
causes the DCHECK to fire. Not needed as this was added as a precaution early
in the high dpi work for Windows.

[sky, 2014-06-03 15:59:06]

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
content/app/content_main_runner.cc:77: #include
"base/strings/string_number_conversions.h"
nit: newline between 76/77.

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
content/app/content_main_runner.cc:669: }
Can you keep the DCHECK in GetDeviceScaleFactor() and have a call to
InitDeviceScaleFactor() here if no explicit scale factor?

https://codereview.chromium.org/308003020/diff/1/content/public/common/conten...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/308003020/diff/1/content/public/common/conten...
content/public/common/content_switches.h:128: CONTENT_EXPORT extern const char
kUseDeviceScaleFactor[];
Why the Use here? How about kDeviceScaleFactor? Also, keep alphabetical.

[ananta, 2014-06-03 19:18:12]

Added a call to InitDeviceScaleFactor in content_main_runner.cc. Added a check
in resource_bundle to see if the scale factor is already initialized prior to
calling InitDeviceScaleFactor.

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
content/app/content_main_runner.cc:77: #include
"base/strings/string_number_conversions.h"
On 2014/06/03 15:59:06, sky wrote:
> nit: newline between 76/77.

Done.

https://codereview.chromium.org/308003020/diff/1/content/public/common/conten...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/308003020/diff/1/content/public/common/conten...
content/public/common/content_switches.h:128: CONTENT_EXPORT extern const char
kUseDeviceScaleFactor[];
On 2014/06/03 15:59:06, sky wrote:
> Why the Use here? How about kDeviceScaleFactor? Also, keep alphabetical.

Done.

[ananta, 2014-06-03 19:18:36]

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/1/content/app/content_main_run...
content/app/content_main_runner.cc:669: }
On 2014/06/03 15:59:06, sky wrote:
> Can you keep the DCHECK in GetDeviceScaleFactor() and have a call to
> InitDeviceScaleFactor() here if no explicit scale factor?

Done.

[jschuh, 2014-06-03 19:36:06]

lgtm

[sky, 2014-06-03 20:11:44]

https://codereview.chromium.org/308003020/diff/30001/content/app/content_main...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/30001/content/app/content_main...
content/app/content_main_runner.cc:668: if
(base::StringToDouble(scale_factor_string, &scale_factor))
Don't you need to handle the else care here?

https://codereview.chromium.org/308003020/diff/30001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/308003020/diff/30001/content/public/common/co...
content/public/common/content_switches.h:122: #if defined(OS_WIN)
Move this section to the bottom of the file with the other conditionals.

https://codereview.chromium.org/308003020/diff/30001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/308003020/diff/30001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:610: if (!gfx::win::GetDeviceScaleFactor())
Wouldn't this hit the DCHECK?

[ananta, 2014-06-03 20:43:24]

https://codereview.chromium.org/308003020/diff/30001/content/app/content_main...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/30001/content/app/content_main...
content/app/content_main_runner.cc:668: if
(base::StringToDouble(scale_factor_string, &scale_factor))
On 2014/06/03 20:11:43, sky wrote:
> Don't you need to handle the else care here?

Done.

https://codereview.chromium.org/308003020/diff/30001/content/public/common/co...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/308003020/diff/30001/content/public/common/co...
content/public/common/content_switches.h:122: #if defined(OS_WIN)
On 2014/06/03 20:11:43, sky wrote:
> Move this section to the bottom of the file with the other conditionals.

Done.

https://codereview.chromium.org/308003020/diff/30001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/308003020/diff/30001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.cc:610: if (!gfx::win::GetDeviceScaleFactor())
On 2014/06/03 20:11:43, sky wrote:
> Wouldn't this hit the DCHECK?

This wouldn't because the scale factor would be initialized in ContentMainRunner
before this code runs.

[sky, 2014-06-03 22:01:21]

LGTM

[sky, 2014-06-03 23:29:05]

LGTM

[ananta, 2014-06-03 23:43:06]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-06-03 23:44:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/308003020/70001

[ananta, 2014-06-03 23:53:00]

+jam for content owners stamp

[jam, 2014-06-04 00:29:15]

https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
content/app/content_main_runner.cc:664: bool init_device_scale_factor = true;
this command line flag is only passed to the renderer, so the code to respond
should only be called in content/renderer. Can you put this in RendererMain, or
did you put this early because there's something between this line and that
which requires this?

[ananta, 2014-06-04 00:34:32]

On 2014/06/04 00:29:15, jam wrote:
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> content/app/content_main_runner.cc:664: bool init_device_scale_factor = true;
> this command line flag is only passed to the renderer, so the code to respond
> should only be called in content/renderer. Can you put this in RendererMain,
or
> did you put this early because there's something between this line and that
> which requires this?
Yes. Resourcebundle

[jam, 2014-06-04 00:41:12]

On 2014/06/04 00:34:32, ananta wrote:
> On 2014/06/04 00:29:15, jam wrote:
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > File content/app/content_main_runner.cc (right):
> > 
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > content/app/content_main_runner.cc:664: bool init_device_scale_factor =
true;
> > this command line flag is only passed to the renderer, so the code to
respond
> > should only be called in content/renderer. Can you put this in RendererMain,
> or
> > did you put this early because there's something between this line and that
> > which requires this?
> Yes. Resourcebundle

what's the callstack that's reaching there?

[ananta, 2014-06-04 00:53:47]

On 2014/06/04 00:41:12, jam wrote:
> On 2014/06/04 00:34:32, ananta wrote:
> > On 2014/06/04 00:29:15, jam wrote:
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > File content/app/content_main_runner.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > content/app/content_main_runner.cc:664: bool init_device_scale_factor =
> true;
> > > this command line flag is only passed to the renderer, so the code to
> respond
> > > should only be called in content/renderer. Can you put this in
RendererMain,
> > or
> > > did you put this early because there's something between this line and
that
> > > which requires this?
> > Yes. Resourcebundle
> 
> what's the callstack that's reaching there?
The resource bundle is initialized in the sandbox warmup code

[jam, 2014-06-04 01:28:16]

On 2014/06/04 00:53:47, ananta wrote:
> On 2014/06/04 00:41:12, jam wrote:
> > On 2014/06/04 00:34:32, ananta wrote:
> > > On 2014/06/04 00:29:15, jam wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > > File content/app/content_main_runner.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > > content/app/content_main_runner.cc:664: bool init_device_scale_factor =
> > true;
> > > > this command line flag is only passed to the renderer, so the code to
> > respond
> > > > should only be called in content/renderer. Can you put this in
> RendererMain,
> > > or
> > > > did you put this early because there's something between this line and
> that
> > > > which requires this?
> > > Yes. Resourcebundle
> > 
> > what's the callstack that's reaching there?
> The resource bundle is initialized in the sandbox warmup code

can you provide the callstack? I'm trying to figure out if there's a way to
avoid putting this stuff in content/app, and that'll be helpful.

[ananta, 2014-06-04 01:41:56]

On 2014/06/04 01:28:16, jam wrote:
> On 2014/06/04 00:53:47, ananta wrote:
> > On 2014/06/04 00:41:12, jam wrote:
> > > On 2014/06/04 00:34:32, ananta wrote:
> > > > On 2014/06/04 00:29:15, jam wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > > > File content/app/content_main_runner.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/308003020/diff/70001/content/app/content_main...
> > > > > content/app/content_main_runner.cc:664: bool init_device_scale_factor
=
> > > true;
> > > > > this command line flag is only passed to the renderer, so the code to
> > > respond
> > > > > should only be called in content/renderer. Can you put this in
> > RendererMain,
> > > > or
> > > > > did you put this early because there's something between this line and
> > that
> > > > > which requires this?
> > > > Yes. Resourcebundle
> > > 
> > > what's the callstack that's reaching there?
> > The resource bundle is initialized in the sandbox warmup code
> 
> can you provide the callstack? I'm trying to figure out if there's a way to
> avoid putting this stuff in content/app, and that'll be helpful.

ContentMainRunner calls delegate_->PreSandboxStartup which sets up the
ResourceBundle. This class tries to initialize the
device scale factor via the gdi32 APIS which won't work with the win32k.sys
mitigation. The ChromeMainDelegate::PreSandboxStartup function here has the code
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_...

[jam, 2014-06-04 02:04:29]

lgtm

[commit-bot: I haz the power, 2014-06-04 03:58:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-04 04:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:02:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[ananta, 2014-06-04 04:14:20]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:15:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/308003020/90001

[commit-bot: I haz the power, 2014-06-04 11:10:54]

Change committed as 274766