Relocate Windows ProcessNeedsProfileDir to install_static

Relocate Windows ProcessNeedsProfileDir to install_static

This is pre-work to enable adding the user data directory to
InstallDetails in the context of http://crrev.com/2867063002

BUG=718437
Review-Url: https://codereview.chromium.org/2884333004
Cr-Commit-Position: refs/heads/master@{#472780}
Committed: https://chromium.googlesource.com/chromium/src/+/8b69ce5fc7a7e267f2f893df6954c3787d5f2d2f

[manzagop (departed), 2017-05-17 17:39:40]

Patchset #1 (id:1) has been deleted

[manzagop (departed), 2017-05-17 17:39:55]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:41:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-05-17 17:41:29]

manzagop@chromium.org changed reviewers:
+ grt@chromium.org, scottmg@chromium.org

[manzagop (departed), 2017-05-17 17:41:29]

Hi Greg, Scott,

This is as discussed in https://crrev.com/2867063002 . Does this make sense?
You'll notice a few interrogations phrased as "DO NOT SUBMIT".

Thanks,

Pierre-Antoine

[scottmg, 2017-05-17 17:58:37]

scottmg@chromium.org changed reviewers:
+ robertshield@chromium.org

[scottmg, 2017-05-17 17:58:37]

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (left):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:457: typedef bool
(*IsSandboxedProcessFunc)();
I don't really know, but I suspect it was only a shorthand way of determining
whether this dll was loaded in the browser. Maybe Robert remembers if there's a
more important reason.

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:507: g_process_type !=
ProcessType::UNINITIALIZED;
Does this get hit? Wouldn't the assert trigger?

[commit-bot: I haz the power, 2017-05-17 18:05:19]

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

[commit-bot: I haz the power, 2017-05-17 18:05:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[robertshield, 2017-05-17 18:24:54]

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (left):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:457: typedef bool
(*IsSandboxedProcessFunc)();
On 2017/05/17 17:58:37, scottmg wrote:
> I don't really know, but I suspect it was only a shorthand way of determining
> whether this dll was loaded in the browser. Maybe Robert remembers if there's
a
> more important reason.

I'm pretty sure that's the long and the short of it though memory has since
faded. Some spelunking suggested that this code originated in the blacklist code
that didn't want to be loaded in non-browser processes. It then migrated here
and still survives. 

Removing the check is fine IFF we know for sure that there are no non-browser
process types that don't have "--type" on the command line.

[manzagop (departed), 2017-05-17 18:59:06]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[manzagop (departed), 2017-05-17 18:59:14]

Thanks! PTAL.

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (left):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:457: typedef bool
(*IsSandboxedProcessFunc)();
On 2017/05/17 18:24:54, robertshield wrote:
> On 2017/05/17 17:58:37, scottmg wrote:
> > I don't really know, but I suspect it was only a shorthand way of
determining
> > whether this dll was loaded in the browser. Maybe Robert remembers if
there's
> a
> > more important reason.
> 
> I'm pretty sure that's the long and the short of it though memory has since
> faded. Some spelunking suggested that this code originated in the blacklist
code
> that didn't want to be loaded in non-browser processes. It then migrated here
> and still survives. 
> 
> Removing the check is fine IFF we know for sure that there are no non-browser
> process types that don't have "--type" on the command line.

A quick look at live processes on a windows box shows no other. Is there any
other verification I could do?

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:507: g_process_type !=
ProcessType::UNINITIALIZED;
On 2017/05/17 17:58:37, scottmg wrote:
> Does this get hit? Wouldn't the assert trigger?

Ah right: I'd mentally interpreted is as DCHECK.

I was wondering whether if the previous logic should be unchanged but no reason.

Done.

[commit-bot: I haz the power, 2017-05-17 19:00:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robertshield, 2017-05-17 19:30:05]

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
File chrome/install_static/install_util.cc (left):

https://codereview.chromium.org/2884333004/diff/20001/chrome/install_static/i...
chrome/install_static/install_util.cc:457: typedef bool
(*IsSandboxedProcessFunc)();
On 2017/05/17 18:59:14, manzagop wrote:
> On 2017/05/17 18:24:54, robertshield wrote:
> > On 2017/05/17 17:58:37, scottmg wrote:
> > > I don't really know, but I suspect it was only a shorthand way of
> determining
> > > whether this dll was loaded in the browser. Maybe Robert remembers if
> there's
> > a
> > > more important reason.
> > 
> > I'm pretty sure that's the long and the short of it though memory has since
> > faded. Some spelunking suggested that this code originated in the blacklist
> code
> > that didn't want to be loaded in non-browser processes. It then migrated
here
> > and still survives. 
> > 
> > Removing the check is fine IFF we know for sure that there are no
non-browser
> > process types that don't have "--type" on the command line.
> 
> A quick look at live processes on a windows box shows no other. Is there any
> other verification I could do?

This suggests that and empty --process-type value == browser process: 

https://codesearch.chromium.org/chromium/src/chrome/app/chrome_main_delegate....

so I reckon we're good.

[commit-bot: I haz the power, 2017-05-17 19:59:21]

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

[commit-bot: I haz the power, 2017-05-17 19:59:21]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2017-05-17 20:10:36]

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:68: constexpr wchar_t kNaClBrokerProcess[]
= L"nacl-broker";
#if !defined(DISABLE_NACL)

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:296: ProcessType
GetProcessType(std::wstring process_type) {
const&

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:297: if (process_type.empty()) {
no braces and no "else":

  if (foo)
    return foo;
  if (bar)
    return bar;

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:314: // On windows we don't want
subprocesses other than the browser process and
nit: Windows

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:318: if (process_type ==
ProcessType::BROWSER_PROCESS ||
to protect against future types being added to the enum but not here, i would go
with:
  switch (process_type) {
    case foo:
    case bar:
#if !defined...
    case bletch:
#endif
      return true;
    case theother:
      return false;
  }
  assert(false);
  return false;

by listing every case explicitly and omitting a "default" clause, future
additions to the enum will throw a compile error until this is adjusted
accordingly.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:507: bool ProcessNeedsProfileDir(const
std::string& process_type) {
part of me wants this function to be an export from chrome_elf that is imported
by chrome{,_child}.dll so that it doesn't need to recompute the process type. i
can see that that's a bigger change, though, and isn't really a huge savings.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.h:264: extern ProcessType g_process_type;
it looks like neither this nor the enum are used outside of the .cc file. would
you move both into an unnamed namespace in the .cc file?

[manzagop (departed), 2017-05-17 21:12:14]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 21:13:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-05-17 21:14:35]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 21:15:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[manzagop (departed), 2017-05-17 21:15:56]

Thanks! Another look?

Also: is it expected that I get some install_static_unittests failures on a
clean checkout (corp setup)?

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:68: constexpr wchar_t kNaClBrokerProcess[]
= L"nacl-broker";
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> #if !defined(DISABLE_NACL)

Done.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:296: ProcessType
GetProcessType(std::wstring process_type) {
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> const&

Err... Done.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:297: if (process_type.empty()) {
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> no braces and no "else":
> 
>   if (foo)
>     return foo;
>   if (bar)
>     return bar;

Done.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:314: // On windows we don't want
subprocesses other than the browser process and
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> nit: Windows

Done.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:318: if (process_type ==
ProcessType::BROWSER_PROCESS ||
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> to protect against future types being added to the enum but not here, i would
go
> with:
>   switch (process_type) {
>     case foo:
>     case bar:
> #if !defined...
>     case bletch:
> #endif
>       return true;
>     case theother:
>       return false;
>   }
>   assert(false);
>   return false;
> 
> by listing every case explicitly and omitting a "default" clause, future
> additions to the enum will throw a compile error until this is adjusted
> accordingly.

Done. It's just clang that throws an error in that case, right?

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.cc:507: bool ProcessNeedsProfileDir(const
std::string& process_type) {
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> part of me wants this function to be an export from chrome_elf that is
imported
> by chrome{,_child}.dll so that it doesn't need to recompute the process type.
i
> can see that that's a bigger change, though, and isn't really a huge savings.

Acknowledged.

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/2884333004/diff/40001/chrome/install_static/i...
chrome/install_static/install_util.h:264: extern ProcessType g_process_type;
On 2017/05/17 20:10:36, grt (UTC plus 2) wrote:
> it looks like neither this nor the enum are used outside of the .cc file.
would
> you move both into an unnamed namespace in the .cc file?

Done.

[grt (UTC plus 2), 2017-05-17 21:17:59]

On 2017/05/17 21:15:56, manzagop wrote:
> Also: is it expected that I get some install_static_unittests failures on a
> clean checkout (corp setup)?

Yeah, I've noticed that...

[grt (UTC plus 2), 2017-05-17 21:21:15]

lgtm

[manzagop (departed), 2017-05-17 21:25:25]

Thanks for the review!

[manzagop (departed), 2017-05-17 21:26:03]

manzagop@chromium.org changed reviewers:
+ thestig@chromium.org

[manzagop (departed), 2017-05-17 21:26:04]

Hi Lei,

Could you have an OWNERS' look at chrome\common\chrome_paths_win.cc?

Thanks!

Pierre-Antoine

[Lei Zhang, 2017-05-17 21:32:06]

lgtm

[manzagop (departed), 2017-05-17 21:36:26]

Thanks!

[commit-bot: I haz the power, 2017-05-18 00:14:41]

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

[commit-bot: I haz the power, 2017-05-18 00:14:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[manzagop (departed), 2017-05-18 11:37:34]

The CQ bit was checked by manzagop@chromium.org

[commit-bot: I haz the power, 2017-05-18 11:37:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 12:23:41]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495107454501920,
"parent_rev": "22e2a3236b3e53b87a8aecb9d1e56a2a9e50e7c2", "commit_rev":
"8b69ce5fc7a7e267f2f893df6954c3787d5f2d2f"}

[commit-bot: I haz the power, 2017-05-18 12:23:53]

Description was changed from

==========
Relocate Windows ProcessNeedsProfileDir to install_static

This is pre-work to enable adding the user data directory to
InstallDetails in the context of http://crrev.com/2867063002

BUG=718437
==========

to

==========
Relocate Windows ProcessNeedsProfileDir to install_static

This is pre-work to enable adding the user data directory to
InstallDetails in the context of http://crrev.com/2867063002

BUG=718437

Review-Url: https://codereview.chromium.org/2884333004
Cr-Commit-Position: refs/heads/master@{#472780}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b69ce5fc7a7e267f2f893df6954...
==========

[commit-bot: I haz the power, 2017-05-18 12:23:54]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8b69ce5fc7a7e267f2f893df6954...