Expose a function to get the path to a DLL.

Expose a function to get the path to a DLL.

We can reduce startup time by pre-reading chrome_child.dll as soon as
possible. For that, we need a way to get the full path to
chrome_child.dll from the browser.

This CL exposes GetModulePath(), a function that returns the full path
to a DLL. Also, since we don't really want to expose the MainDllLoader
class to the browser, we move it to a separate file.

BUG=547794
Committed: https://crrev.com/e4adea20236d1cee76f0c61798b1613e07a7f4c1
Cr-Commit-Position: refs/heads/master@{#357567}

[fdoray, 2015-10-28 15:38:36]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2015-10-28 15:38:36]

Could you review this CL? Thanks.

[grt (UTC plus 2), 2015-10-29 18:51:50]

https://codereview.chromium.org/1402353011/diff/1/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/app/client_util.cc#n...
chrome/app/client_util.cc:40: base::File file(file_path, base::File::FLAG_OPEN |
base::File::FLAG_READ);
nit: return base::File(...).IsValid();
also, just to confirm: are these the same perms used by the loader to bring an
image into the process?

https://codereview.chromium.org/1402353011/diff/1/chrome/app/main_dll_loader_...
File chrome/app/main_dll_loader_win.h (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/app/main_dll_loader_...
chrome/app/main_dll_loader_win.h:17: struct SandboxInterfaceInfo;
unused

https://codereview.chromium.org/1402353011/diff/1/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:523: 'target_name': 'client_util',
rather than introduce a new target, how about putting this into installer_util
(chrome/installer/util)? this would be accessible by both chrome_initial and
browser.

[fdoray, 2015-10-30 14:45:45]

grt@: Could you take another look? Thanks.

https://codereview.chromium.org/1402353011/diff/1/chrome/app/client_util.cc
File chrome/app/client_util.cc (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/app/client_util.cc#n...
chrome/app/client_util.cc:40: base::File file(file_path, base::File::FLAG_OPEN |
base::File::FLAG_READ);
On 2015/10/29 18:51:50, grt wrote:
> nit: return base::File(...).IsValid();
> also, just to confirm: are these the same perms used by the loader to bring an
> image into the process?


Done. And yes, I confirmed with ProcMon that I use the same parameters as the
loader.

https://codereview.chromium.org/1402353011/diff/1/chrome/app/main_dll_loader_...
File chrome/app/main_dll_loader_win.h (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/app/main_dll_loader_...
chrome/app/main_dll_loader_win.h:17: struct SandboxInterfaceInfo;
On 2015/10/29 18:51:50, grt wrote:
> unused

Done.

https://codereview.chromium.org/1402353011/diff/1/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1402353011/diff/1/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:523: 'target_name': 'client_util',
On 2015/10/29 18:51:50, grt wrote:
> rather than introduce a new target, how about putting this into installer_util
> (chrome/installer/util)? this would be accessible by both chrome_initial and
> browser.

Done.

[grt (UTC plus 2), 2015-10-30 15:45:59]

lgtm w/ a pair of nits

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
chrome/installer/util/module_util_win.h:15: // Returns the full path to a module
named |module_name|, or an empty path if
this comment isn't quite right. an empty path is returned if the current module
doesn't have a version resource. the function does not validate that the file
exists if it peeks into a version directory. i think it's fine to change the
comment to match the impl rather than the other way around.

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
chrome/installer/util/module_util_win.h:20: base::FilePath GetModulePath(const
base::char16* module_name,
nit: pass module_name by const-ref

[grt (UTC plus 2), 2015-10-30 15:57:58]

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
chrome/installer/util/module_util_win.h:20: base::FilePath GetModulePath(const
base::char16* module_name,
On 2015/10/30 15:45:59, grt wrote:
> nit: pass module_name by const-ref

oops, that doesn't make sense (i read it as string16 rather than char16). how
about using StringPiece16 here? this will allow callers to pass either a char16*
or a string16.

[fdoray, 2015-10-30 17:22:15]

All done. Thanks for the review.

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
chrome/installer/util/module_util_win.h:15: // Returns the full path to a module
named |module_name|, or an empty path if
On 2015/10/30 15:45:59, grt wrote:
> this comment isn't quite right. an empty path is returned if the current
module
> doesn't have a version resource. the function does not validate that the file
> exists if it peeks into a version directory. i think it's fine to change the
> comment to match the impl rather than the other way around.

Done.

https://codereview.chromium.org/1402353011/diff/80001/chrome/installer/util/m...
chrome/installer/util/module_util_win.h:20: base::FilePath GetModulePath(const
base::char16* module_name,
On 2015/10/30 15:57:58, grt wrote:
> On 2015/10/30 15:45:59, grt wrote:
> > nit: pass module_name by const-ref
> 
> oops, that doesn't make sense (i read it as string16 rather than char16). how
> about using StringPiece16 here? this will allow callers to pass either a
char16*
> or a string16.

Done.

[fdoray, 2015-10-30 17:23:39]

fdoray@chromium.org changed reviewers:
+ dpranke@chromium.org

[fdoray, 2015-10-30 17:23:40]

+dpranke@: Could you review the BUILD.gn file?

[fdoray, 2015-10-30 17:27:07]

-dpranke@, who is not an owner for chrome/BUILD.gn

+jochen@: Could you review chrome/BUILD.gn? Thanks.

[fdoray, 2015-10-30 17:27:29]

fdoray@chromium.org changed reviewers:
+ jochen@chromium.org
- dpranke@chromium.org

[fdoray, 2015-10-30 17:27:30]

-dpranke@, who is not an owner for chrome/BUILD.gn

+jochen@: Could you review chrome/BUILD.gn? Thanks.

[grt (UTC plus 2), 2015-10-30 17:45:37]

https://codereview.chromium.org/1402353011/diff/100001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/100001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:17: // the current module doesn't have a
version resource. The searched module must
still not quite right: it's an empty path if there's no version resource and the
desired module isn't in the current directory, right? :-)

[fdoray, 2015-10-30 18:25:25]

grt@: Done.
jochen@: Could you review chrome/BUILD.gn? Thanks.

https://codereview.chromium.org/1402353011/diff/100001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/100001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:17: // the current module doesn't have a
version resource. The searched module must
On 2015/10/30 17:45:37, grt wrote:
> still not quite right: it's an empty path if there's no version resource and
the
> desired module isn't in the current directory, right? :-)

Yes, you're right. I hope I got it right this time :)

[grt (UTC plus 2), 2015-10-30 19:33:14]

https://codereview.chromium.org/1402353011/diff/120001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/120001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:16: // Returns the full path to a module
named |module_name|, or an empty path if
this is hard to document. :-) what do you think of:
// Returns the full path to |module_name|. Both dev builds (where |module_name|
// is in the current executable's directory) and proper installs (where
// |module_name| is in a versioned sub-directory of the current executable's
// directory) are suported. In the latter case, |version| is populated with the
// name of the versioned sub-directory (i.e., the current executable's
// version). Returns an empty path if the current executable appears to reside
// in a proper install but version information is missing. The identified file
// is not guaranteed to exist.

[fdoray, 2015-10-30 21:12:49]

grt@: Done.
jochen@: Could you review chrome/BUILD.gn? Thanks.

https://codereview.chromium.org/1402353011/diff/120001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/120001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:16: // Returns the full path to a module
named |module_name|, or an empty path if
On 2015/10/30 19:33:14, grt wrote:
> this is hard to document. :-) what do you think of:
> // Returns the full path to |module_name|. Both dev builds (where
|module_name|
> // is in the current executable's directory) and proper installs (where
> // |module_name| is in a versioned sub-directory of the current executable's
> // directory) are suported. In the latter case, |version| is populated with
the
> // name of the versioned sub-directory (i.e., the current executable's
> // version). Returns an empty path if the current executable appears to reside
> // in a proper install but version information is missing. The identified file
> // is not guaranteed to exist.

Done.

[grt (UTC plus 2), 2015-11-02 15:33:39]

lgtm w/ a nit

https://codereview.chromium.org/1402353011/diff/140001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/140001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:24: base::FilePath GetModulePath(const
base::StringPiece16& module_name,
nit: Prefer passing StringPieces by value (from base/strings/string_piece.h).

[fdoray, 2015-11-02 17:36:26]

grt@: Done.
jochen@: Could you review chrome/BUILD.gn? Thanks.

https://codereview.chromium.org/1402353011/diff/140001/chrome/installer/util/...
File chrome/installer/util/module_util_win.h (right):

https://codereview.chromium.org/1402353011/diff/140001/chrome/installer/util/...
chrome/installer/util/module_util_win.h:24: base::FilePath GetModulePath(const
base::StringPiece16& module_name,
On 2015/11/02 15:33:39, grt wrote:
> nit: Prefer passing StringPieces by value (from base/strings/string_piece.h).

Done.

[jochen (gone - plz use gerrit), 2015-11-03 07:20:44]

lgtm

[fdoray, 2015-11-03 17:37:32]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-11-03 17:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1402353011/#ps160001
(title: "grt comment #17 (pass StringPiece16 by value)")

[commit-bot: I haz the power, 2015-11-03 17:41:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1402353011/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1402353011/160001

[commit-bot: I haz the power, 2015-11-03 18:27:37]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-03 18:29:31]

Patchset 9 (id:??) landed as
https://crrev.com/e4adea20236d1cee76f0c61798b1613e07a7f4c1
Cr-Commit-Position: refs/heads/master@{#357567}