Issue 1913943003: Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows.

ananta

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

4 years, 8 months ago (2016-04-25 23:58:03 UTC) #1

ananta

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

4 years, 8 months ago (2016-04-26 00:20:38 UTC) #3

scottmg

Looks like the GN build is pulling in user32. Pretty nervous about this given our ...

4 years, 7 months ago (2016-04-26 18:38:02 UTC) #5

scottmg

https://codereview.chromium.org/1913943003/diff/160001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/160001/chrome/install_static/install_util.cc#newcode416 chrome/install_static/install_util.cc:416: return false; If this returns false, then special_build and ...

4 years, 7 months ago (2016-04-26 18:51:30 UTC) #6

scottmg

Should have grt look at the installer/version stuff once we think we're ready, too. :)

4 years, 7 months ago (2016-04-26 18:52:21 UTC) #7

robertshield

very quick drive by, elf code lg, let me know if you'd like a full ...

4 years, 7 months ago (2016-04-26 18:56:33 UTC) #8

ananta

https://codereview.chromium.org/1913943003/diff/160001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/160001/chrome/install_static/install_util.cc#newcode173 chrome/install_static/install_util.cc:173: base::string16* value_str) { On 2016/04/26 18:56:33, robertshield wrote: > ...

4 years, 7 months ago (2016-04-26 20:23:16 UTC) #9

scottmg

OK, lgtm-enough to get grt to scrutinize and be scared now. :) https://codereview.chromium.org/1913943003/diff/180001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc ...

4 years, 7 months ago (2016-04-26 20:44:47 UTC) #10

ananta

https://codereview.chromium.org/1913943003/diff/180001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/180001/chrome/install_static/install_util.cc#newcode358 chrome/install_static/install_util.cc:358: size)) { On 2016/04/26 20:44:47, scottmg wrote: > Confirm ...

4 years, 7 months ago (2016-04-26 21:33:09 UTC) #11

ananta

Description was changed from ========== Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows. ...

4 years, 7 months ago (2016-04-27 22:42:11 UTC) #12

scottmg

https://codereview.chromium.org/1913943003/diff/340001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/1913943003/diff/340001/chrome/app/chrome_crash_reporter_client_win.cc#newcode106 chrome/app/chrome_crash_reporter_client_win.cc:106: std::string version_string = install_static::GetGoogleUpdateVersion(); #include <string> https://codereview.chromium.org/1913943003/diff/340001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc ...

4 years, 7 months ago (2016-04-27 23:29:25 UTC) #14

ananta

https://codereview.chromium.org/1913943003/diff/340001/chrome/app/chrome_crash_reporter_client_win.cc File chrome/app/chrome_crash_reporter_client_win.cc (right): https://codereview.chromium.org/1913943003/diff/340001/chrome/app/chrome_crash_reporter_client_win.cc#newcode106 chrome/app/chrome_crash_reporter_client_win.cc:106: std::string version_string = install_static::GetGoogleUpdateVersion(); On 2016/04/27 23:29:24, scottmg wrote: ...

4 years, 7 months ago (2016-04-28 00:04:25 UTC) #15

ananta

ananta@chromium.org changed reviewers: + sky@chromium.org

4 years, 7 months ago (2016-04-28 00:06:27 UTC) #16

ananta

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

4 years, 7 months ago (2016-04-28 00:19:32 UTC) #18

ananta

+grt for installer changes. Please take a look at the overall change and specifically at ...

4 years, 7 months ago (2016-04-28 00:19:33 UTC) #19

ananta

Description was changed from ========== Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows. ...

4 years, 7 months ago (2016-04-28 20:35:35 UTC) #21

ananta

ananta@chromium.org changed reviewers: + dschuff@chromium.org

4 years, 7 months ago (2016-04-28 21:25:58 UTC) #23

scottmg

https://codereview.chromium.org/1913943003/diff/440001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/440001/chrome/install_static/install_util.cc#newcode281 chrome/install_static/install_util.cc:281: Trace(L"%hs( %ls directory exists)\n", __FUNCTION__, full_path_str); extra space here ...

4 years, 7 months ago (2016-04-28 21:34:01 UTC) #25

ananta

https://codereview.chromium.org/1913943003/diff/440001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/440001/chrome/install_static/install_util.cc#newcode281 chrome/install_static/install_util.cc:281: Trace(L"%hs( %ls directory exists)\n", __FUNCTION__, full_path_str); On 2016/04/28 21:34:00, ...

4 years, 7 months ago (2016-04-28 22:54:40 UTC) #26

scottmg

lg https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc#newcode354 chrome/install_static/install_util.cc:354: // which matches 0 or more instances of ...

4 years, 7 months ago (2016-04-28 23:16:42 UTC) #27

ananta

https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc#newcode354 chrome/install_static/install_util.cc:354: // which matches 0 or more instances of any ...

4 years, 7 months ago (2016-04-29 02:28:06 UTC) #28

Derek Schuff

On 2016/04/29 02:28:06, ananta wrote: > https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc > File chrome/install_static/install_util.cc (right): > > https://codereview.chromium.org/1913943003/diff/460001/chrome/install_static/install_util.cc#newcode354 > ...

4 years, 7 months ago (2016-04-29 07:08:28 UTC) #29

grt (UTC plus 2)

whew! partial review. more to come. https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc#newcode1 chrome/install_static/install_util.cc:1: // Copyright 2014 ...

4 years, 7 months ago (2016-04-29 20:50:57 UTC) #30

ananta

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc#newcode1 chrome/install_static/install_util.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. ...

4 years, 7 months ago (2016-04-29 22:49:25 UTC) #31

grt (UTC plus 2)

more comments from another partial pass. https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc#newcode167 chrome/install_static/install_util.cc:167: LanguageAndCodePage* translation_info = ...

4 years, 7 months ago (2016-05-02 22:22:06 UTC) #32

more comments from another partial pass.

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:167: LanguageAndCodePage* translation_info
= nullptr;
On 2016/04/29 22:49:23, ananta wrote:
> On 2016/04/29 20:50:56, grt wrote:
> > MSDN leads me to believe that this should be:
> >   DWORD *translation_info = nullptr;
> >   ...
> >   *language = LOWORD(*translation_info);
> >   *code_page = HIWORD(*translation_info);
> > (from
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646994(v=vs.85).aspx)
> 
> This is based on
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms647464(v=vs.85).aspx.
> The helper in base also does the same thing.

Okay, thanks for the pointer.

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:192: return false;
On 2016/04/29 22:49:24, ananta wrote:
> On 2016/04/29 20:50:56, grt wrote:
> > based on
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646994(v=vs.85).aspx,
> > should we continue trying to get values under the assumption that they're
> "both
> > language and code page independent"? maybe simplify things and use (0,0) in
> the
> > loop below in this case. seems a bit more robust.
> 
> We could possibly do that. The logic below is the same as base. I am wary of
> changing it for now. Don't want this to cause regressions.

Gotcha. Thinking about it more, why are we even doing these random
shots-in-the-dark? Does the call to VerQueryValue in
GetLanguageAndCodePageFromVersionResource return the exact array of lang/CP
pairs that are present in the file? Should we really iterate over all of those?
Don't bother changing it now, but consider this a note for the future.

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:266: base::string16 result(size, 0);
On 2016/04/29 22:49:24, ananta wrote:
> On 2016/04/29 20:50:56, grt wrote:
> > uber-nit: 0 -> L'\0' to make it super clear that the 2nd arg is a character
> > value.
> 
> Done.

Hah, you did it on the line above that I missed, but not here. ;-)

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:408: bool IsCanary(const wchar_t*
exe_path) {
On 2016/04/29 22:49:24, ananta wrote:
> On 2016/04/29 20:50:56, grt wrote:
> > is it possible/does it make sense to use base::StringPiece16 rather than raw
> > pointers for these fns?
> 
> Sadly no. StringPiece16 comes with logging.h, which causes all kinds of
problems
> in chrome_elf

Acknowledged.

https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client_win.cc (right):

https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client_win.cc:180: return
is_official_chrome_build && install_static::GetCollectStatsConsent();
this logic differs from that in breakpad.cc's InitializeCrashReporting. the
latter supports policy controls on Chromium builds, whereas this forces stats
collection off across the board for them. it seems cleaner to have the special
handling for Google Chrome vs Chromium inside GetCollectStatsConsentImpl so that
consumers of install_static don't need any brand-specific conditional
compilation.

https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc
File chrome_elf/breakpad.cc (right):

https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc...
chrome_elf/breakpad.cc:151: bool use_policy =
since install_static::GetCollectStatsConsent calls
install_static::ReportingIsEnforcedByPolicy, i don't understand why the latter
should be public or have special treatment here. if this code is really correct
with its special treatment of policy, i propose that all Google Chrome vs
Chromium logic be moved into GetCollectStatsConsent (as requested elsewhere),
and that this be changed to:

  bool controlled_by_policy = false;
  bool reporting_enabled =
install_static::GetCollectStatsConsent(&controlled_by_policy);

  if (!controlled_by_policy && IsHeadless()) {
    // This is a headless bot -- connect to the crash test service.
    pipe_name = kChromePipName;
  } else if (reporting_enabled) {
    // Connect to breakpad.
    ...
  } else {
    // This is not a headless bot and reporting is disabled.
    return;
  }

I think ReportingIsEnforcedByPolicy can become a private implementation detail
of install_static, and that |controlled_by_policy| can be an optional out param
of GetCollectStatsConsent.

oh, crumb, i just saw that InitializeCrashpadImpl has its own version of this
logic. something should happen here. it seems that some bits of the code think
that checking policy and checking for consent are two different things, yet
other parts think they're one thing. maybe the shortest path for now is for
GetCollectStatsConsentImpl to not check policy, but i think this is the wrong
thing in the long run: there should be one and only one (well-tested)
implementation of "do i have consent from the user or an administrator to
report?"

ananta

On 2016/05/02 22:22:06, grt wrote: > more comments from another partial pass. > > https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc ...

4 years, 7 months ago (2016-05-02 22:39:52 UTC) #33

On 2016/05/02 22:22:06, grt wrote:
> more comments from another partial pass.
> 
>
https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
> File chrome/install_static/install_util.cc (right):
> 
>
https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
> chrome/install_static/install_util.cc:167: LanguageAndCodePage*
translation_info
> = nullptr;
> On 2016/04/29 22:49:23, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > MSDN leads me to believe that this should be:
> > >   DWORD *translation_info = nullptr;
> > >   ...
> > >   *language = LOWORD(*translation_info);
> > >   *code_page = HIWORD(*translation_info);
> > > (from
> > >
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646994(v=vs.85).aspx)
> > 
> > This is based on
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms647464(v=vs.85).aspx.
> > The helper in base also does the same thing.
> 
> Okay, thanks for the pointer.
> 
>
https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
> chrome/install_static/install_util.cc:192: return false;
> On 2016/04/29 22:49:24, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > based on
> > >
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646994(v=vs.85).aspx,
> > > should we continue trying to get values under the assumption that they're
> > "both
> > > language and code page independent"? maybe simplify things and use (0,0)
in
> > the
> > > loop below in this case. seems a bit more robust.
> > 
> > We could possibly do that. The logic below is the same as base. I am wary of
> > changing it for now. Don't want this to cause regressions.
> 
> Gotcha. Thinking about it more, why are we even doing these random
> shots-in-the-dark? Does the call to VerQueryValue in
> GetLanguageAndCodePageFromVersionResource return the exact array of lang/CP
> pairs that are present in the file? Should we really iterate over all of
those?
> Don't bother changing it now, but consider this a note for the future.
> 
>
https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
> chrome/install_static/install_util.cc:266: base::string16 result(size, 0);
> On 2016/04/29 22:49:24, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > uber-nit: 0 -> L'\0' to make it super clear that the 2nd arg is a
character
> > > value.
> > 
> > Done.
> 
> Hah, you did it on the line above that I missed, but not here. ;-)
> 
>
https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
> chrome/install_static/install_util.cc:408: bool IsCanary(const wchar_t*
> exe_path) {
> On 2016/04/29 22:49:24, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > is it possible/does it make sense to use base::StringPiece16 rather than
raw
> > > pointers for these fns?
> > 
> > Sadly no. StringPiece16 comes with logging.h, which causes all kinds of
> problems
> > in chrome_elf
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
> File chrome/app/chrome_crash_reporter_client_win.cc (right):
> 
>
https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
> chrome/app/chrome_crash_reporter_client_win.cc:180: return
> is_official_chrome_build && install_static::GetCollectStatsConsent();
> this logic differs from that in breakpad.cc's InitializeCrashReporting. the
> latter supports policy controls on Chromium builds, whereas this forces stats
> collection off across the board for them. it seems cleaner to have the special
> handling for Google Chrome vs Chromium inside GetCollectStatsConsentImpl so
that
> consumers of install_static don't need any brand-specific conditional
> compilation.
> 
> https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc
> File chrome_elf/breakpad.cc (right):
> 
>
https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc...
> chrome_elf/breakpad.cc:151: bool use_policy =
> since install_static::GetCollectStatsConsent calls
> install_static::ReportingIsEnforcedByPolicy, i don't understand why the latter
> should be public or have special treatment here. if this code is really
correct
> with its special treatment of policy, i propose that all Google Chrome vs
> Chromium logic be moved into GetCollectStatsConsent (as requested elsewhere),
> and that this be changed to:
> 
>   bool controlled_by_policy = false;
>   bool reporting_enabled =
> install_static::GetCollectStatsConsent(&controlled_by_policy);
> 
>   if (!controlled_by_policy && IsHeadless()) {
>     // This is a headless bot -- connect to the crash test service.
>     pipe_name = kChromePipName;
>   } else if (reporting_enabled) {
>     // Connect to breakpad.
>     ...
>   } else {
>     // This is not a headless bot and reporting is disabled.
>     return;
>   }
> 
> I think ReportingIsEnforcedByPolicy can become a private implementation detail
> of install_static, and that |controlled_by_policy| can be an optional out
param
> of GetCollectStatsConsent.
> 
> oh, crumb, i just saw that InitializeCrashpadImpl has its own version of this
> logic. something should happen here. it seems that some bits of the code think
> that checking policy and checking for consent are two different things, yet
> other parts think they're one thing. maybe the shortest path for now is for
> GetCollectStatsConsentImpl to not check policy, but i think this is the wrong
> thing in the long run: there should be one and only one (well-tested)
> implementation of "do i have consent from the user or an administrator to
> report?"

This file will go away in the near future once we change chrome_elf to use
crash_pad.
We can look into making the ReportingIsEnforcedByPolicy function a private
function then.

ananta

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/install_util.cc#newcode192 chrome/install_static/install_util.cc:192: return false; On 2016/05/02 22:22:06, grt wrote: > On ...

4 years, 7 months ago (2016-05-02 22:43:28 UTC) #34

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
File chrome/install_static/install_util.cc (right):

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:192: return false;
On 2016/05/02 22:22:06, grt wrote:
> On 2016/04/29 22:49:24, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > based on
> > >
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms646994(v=vs.85).aspx,
> > > should we continue trying to get values under the assumption that they're
> > "both
> > > language and code page independent"? maybe simplify things and use (0,0)
in
> > the
> > > loop below in this case. seems a bit more robust.
> > 
> > We could possibly do that. The logic below is the same as base. I am wary of
> > changing it for now. Don't want this to cause regressions.
> 
> Gotcha. Thinking about it more, why are we even doing these random
> shots-in-the-dark? Does the call to VerQueryValue in
> GetLanguageAndCodePageFromVersionResource return the exact array of lang/CP
> pairs that are present in the file? Should we really iterate over all of
those?
> Don't bother changing it now, but consider this a note for the future.

Added a TODO in the function. Will look into this in a subsequent patch

https://codereview.chromium.org/1913943003/diff/520001/chrome/install_static/...
chrome/install_static/install_util.cc:266: base::string16 result(size, 0);
On 2016/05/02 22:22:06, grt wrote:
> On 2016/04/29 22:49:24, ananta wrote:
> > On 2016/04/29 20:50:56, grt wrote:
> > > uber-nit: 0 -> L'\0' to make it super clear that the 2nd arg is a
character
> > > value.
> > 
> > Done.
> 
> Hah, you did it on the line above that I missed, but not here. ;-)

Sorry, fixed.

https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client_win.cc (right):

https://codereview.chromium.org/1913943003/diff/580001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client_win.cc:180: return
is_official_chrome_build && install_static::GetCollectStatsConsent();
On 2016/05/02 22:22:06, grt wrote:
> this logic differs from that in breakpad.cc's InitializeCrashReporting. the
> latter supports policy controls on Chromium builds, whereas this forces stats
> collection off across the board for them. it seems cleaner to have the special
> handling for Google Chrome vs Chromium inside GetCollectStatsConsentImpl so
that
> consumers of install_static don't need any brand-specific conditional
> compilation.

Removed the official build check here.

https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc
File chrome_elf/breakpad.cc (right):

https://codereview.chromium.org/1913943003/diff/580001/chrome_elf/breakpad.cc...
chrome_elf/breakpad.cc:151: bool use_policy =
On 2016/05/02 22:22:06, grt wrote:
> since install_static::GetCollectStatsConsent calls
> install_static::ReportingIsEnforcedByPolicy, i don't understand why the latter
> should be public or have special treatment here. if this code is really
correct
> with its special treatment of policy, i propose that all Google Chrome vs
> Chromium logic be moved into GetCollectStatsConsent (as requested elsewhere),
> and that this be changed to:
> 
>   bool controlled_by_policy = false;
>   bool reporting_enabled =
> install_static::GetCollectStatsConsent(&controlled_by_policy);
> 
>   if (!controlled_by_policy && IsHeadless()) {
>     // This is a headless bot -- connect to the crash test service.
>     pipe_name = kChromePipName;
>   } else if (reporting_enabled) {
>     // Connect to breakpad.
>     ...
>   } else {
>     // This is not a headless bot and reporting is disabled.
>     return;
>   }
> 
> I think ReportingIsEnforcedByPolicy can become a private implementation detail
> of install_static, and that |controlled_by_policy| can be an optional out
param
> of GetCollectStatsConsent.
> 
> oh, crumb, i just saw that InitializeCrashpadImpl has its own version of this
> logic. something should happen here. it seems that some bits of the code think
> that checking policy and checking for consent are two different things, yet
> other parts think they're one thing. maybe the shortest path for now is for
> GetCollectStatsConsentImpl to not check policy, but i think this is the wrong
> thing in the long run: there should be one and only one (well-tested)
> implementation of "do i have consent from the user or an administrator to
> report?"

Once chrome_elf is changed to use crashpad, we don't need to worry about
breakpad. That change will come next. I think we can leave this as is for now as
this file should go away

grt (UTC plus 2)

https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc#newcode324 chrome/install_static/install_util.cc:324: DWORD file_attributes = GetFileAttributes(full_path_str); nit: ::GetFileAttributes as above https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc#newcode637 ...

4 years, 7 months ago (2016-05-03 10:47:55 UTC) #35

grt (UTC plus 2)

https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc#newcode637 chrome/install_static/install_util.cc:637: void GetChromeChannelName(bool is_per_user_install, On 2016/05/03 10:47:55, grt wrote: > ...

4 years, 7 months ago (2016-05-03 15:54:55 UTC) #36

ananta

https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/1913943003/diff/620001/chrome/install_static/install_util.cc#newcode324 chrome/install_static/install_util.cc:324: DWORD file_attributes = GetFileAttributes(full_path_str); On 2016/05/03 10:47:55, grt wrote: ...

4 years, 7 months ago (2016-05-03 21:40:57 UTC) #37

ananta

PTAL when you get a chance. Would like to land this to get some data ...

4 years, 7 months ago (2016-05-04 23:01:35 UTC) #39

grt (UTC plus 2)

Final comments (I swear). Thanks for being patient. I am apprehensive about landing such a ...

4 years, 7 months ago (2016-05-05 20:36:52 UTC) #40

ananta

There is very little test coverage for crashpad. I added a todo for myself and ...

4 years, 7 months ago (2016-05-05 20:57:35 UTC) #41

grt (UTC plus 2)

https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc#newcode189 chrome_elf/chrome_elf_util_unittest.cc:189: install_static::GetChromeChannelName(!system_level_, true, &channel); please add test coverage for the ...

4 years, 7 months ago (2016-05-06 15:05:01 UTC) #42

ananta

https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc#newcode189 chrome_elf/chrome_elf_util_unittest.cc:189: install_static::GetChromeChannelName(!system_level_, true, &channel); On 2016/05/06 15:05:01, grt (slow) wrote: ...

4 years, 7 months ago (2016-05-06 19:36:03 UTC) #43

ananta

https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc#newcode319 chrome_elf/chrome_elf_util_unittest.cc:319: EXPECT_EQ(install_static::kChromeChannelStable, channel); On 2016/05/06 19:36:03, ananta wrote: > On ...

4 years, 7 months ago (2016-05-06 21:30:11 UTC) #44

grt (UTC plus 2)

https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc#newcode319 chrome_elf/chrome_elf_util_unittest.cc:319: EXPECT_EQ(install_static::kChromeChannelStable, channel); On 2016/05/06 21:30:11, ananta wrote: > On ...

4 years, 7 months ago (2016-05-09 18:54:11 UTC) #45

ananta

https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/740001/chrome_elf/chrome_elf_util_unittest.cc#newcode319 chrome_elf/chrome_elf_util_unittest.cc:319: EXPECT_EQ(install_static::kChromeChannelStable, channel); On 2016/05/09 18:54:11, grt (slow) wrote: > ...

4 years, 7 months ago (2016-05-09 22:21:07 UTC) #46

grt (UTC plus 2)

lgtm (fingers crossed) https://codereview.chromium.org/1913943003/diff/800001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/800001/chrome_elf/chrome_elf_util_unittest.cc#newcode282 chrome_elf/chrome_elf_util_unittest.cc:282: // We don't check for modifiers ...

4 years, 7 months ago (2016-05-10 16:04:16 UTC) #47

ananta

Thanks for the review https://codereview.chromium.org/1913943003/diff/800001/chrome_elf/chrome_elf_util_unittest.cc File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/1913943003/diff/800001/chrome_elf/chrome_elf_util_unittest.cc#newcode282 chrome_elf/chrome_elf_util_unittest.cc:282: // We don't check for ...

4 years, 7 months ago (2016-05-10 19:28:18 UTC) #48

ananta

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, sky@chromium.org, dschuff@chromium.org, grt@chromium.org ...

4 years, 7 months ago (2016-05-10 19:40:15 UTC) #50

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-10 19:40:45 UTC) #51

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-10 19:48:50 UTC) #52

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/180654)

4 years, 7 months ago (2016-05-10 19:48:52 UTC) #53

ananta

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

4 years, 7 months ago (2016-05-10 20:04:47 UTC) #54

Lei Zhang

On 2016/05/10 20:04:49, ananta wrote: > +thestig for install_static/DEPS Do you actually need that DEPS ...

4 years, 7 months ago (2016-05-10 20:40:44 UTC) #57

ananta

On 2016/05/10 20:40:44, Lei Zhang wrote: > On 2016/05/10 20:04:49, ananta wrote: > > +thestig ...

4 years, 7 months ago (2016-05-10 21:02:32 UTC) #58

Lei Zhang

On 2016/05/10 21:02:32, ananta wrote: > On 2016/05/10 20:40:44, Lei Zhang wrote: > > On ...

4 years, 7 months ago (2016-05-10 21:11:26 UTC) #59

ananta

On 2016/05/10 21:11:26, Lei Zhang wrote: > On 2016/05/10 21:02:32, ananta wrote: > > On ...

4 years, 7 months ago (2016-05-10 21:27:40 UTC) #60

Lei Zhang

On 2016/05/10 21:27:40, ananta wrote: > Added some rules. PTAL ok, lgtm

4 years, 7 months ago (2016-05-10 21:36:28 UTC) #61

ananta

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, scottmg@chromium.org, robertshield@chromium.org, dschuff@chromium.org, ...

4 years, 7 months ago (2016-05-10 21:46:46 UTC) #63

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-10 21:47:25 UTC) #64

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-10 23:17:10 UTC) #65

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/226612)

4 years, 7 months ago (2016-05-10 23:17:12 UTC) #66

ananta

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, thestig@chromium.org, scottmg@chromium.org, robertshield@chromium.org, ...

4 years, 7 months ago (2016-05-10 23:44:54 UTC) #68

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-10 23:45:29 UTC) #69

ananta

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, thestig@chromium.org, scottmg@chromium.org, robertshield@chromium.org, ...

4 years, 7 months ago (2016-05-11 00:32:30 UTC) #71

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-11 00:33:03 UTC) #72

commit-bot: I haz the power

Description was changed from ========== Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows. ...

4 years, 7 months ago (2016-05-11 02:36:40 UTC) #73

Message was sent while issue was closed.

Description was changed from

==========
Remove dependencies on chrome\installer from the ChromeCrashReporterClient class
on Windows.

This is step 2 of the work underway to move the crash pad registration from
chrome.exe to chrome_elf.

Most of the changes in this patch are addition of helper functions to the
install_static_util library
to retrieve the channel name, user data directory, chrome version information
etc. I added TODOs to
these functions with comments that we need to consolidate these with similar
functions in Chrome\installer.

As install_static is pulled in by chrome_elf, we cannot depend
on functionality in base which brings in dependencies on user32.
This includes utf8 to utf16 conversion, pattern matching functions, etc. I added
implementations of the functions
we need to install_util.cc.

Once we test this out in the field and things work well, we will move the crash
pad registration into chrome_elf

BUG=604923
==========

to

==========
Remove dependencies on chrome\installer from the ChromeCrashReporterClient class
on Windows.

This is step 2 of the work underway to move the crash pad registration from
chrome.exe to chrome_elf.

Most of the changes in this patch are addition of helper functions to the
install_static_util library
to retrieve the channel name, user data directory, chrome version information
etc. I added TODOs to
these functions with comments that we need to consolidate these with similar
functions in Chrome\installer.

As install_static is pulled in by chrome_elf, we cannot depend
on functionality in base which brings in dependencies on user32.
This includes utf8 to utf16 conversion, pattern matching functions, etc. I added
implementations of the functions
we need to install_util.cc.

Once we test this out in the field and things work well, we will move the crash
pad registration into chrome_elf

BUG=604923
==========

commit-bot: I haz the power

Description was changed from ========== Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows. ...

4 years, 7 months ago (2016-05-11 02:39:02 UTC) #75

Message was sent while issue was closed.

Description was changed from

==========
Remove dependencies on chrome\installer from the ChromeCrashReporterClient class
on Windows.

This is step 2 of the work underway to move the crash pad registration from
chrome.exe to chrome_elf.

Most of the changes in this patch are addition of helper functions to the
install_static_util library
to retrieve the channel name, user data directory, chrome version information
etc. I added TODOs to
these functions with comments that we need to consolidate these with similar
functions in Chrome\installer.

As install_static is pulled in by chrome_elf, we cannot depend
on functionality in base which brings in dependencies on user32.
This includes utf8 to utf16 conversion, pattern matching functions, etc. I added
implementations of the functions
we need to install_util.cc.

Once we test this out in the field and things work well, we will move the crash
pad registration into chrome_elf

BUG=604923
==========

to

==========
Remove dependencies on chrome\installer from the ChromeCrashReporterClient class
on Windows.

This is step 2 of the work underway to move the crash pad registration from
chrome.exe to chrome_elf.

Most of the changes in this patch are addition of helper functions to the
install_static_util library
to retrieve the channel name, user data directory, chrome version information
etc. I added TODOs to
these functions with comments that we need to consolidate these with similar
functions in Chrome\installer.

As install_static is pulled in by chrome_elf, we cannot depend
on functionality in base which brings in dependencies on user32.
This includes utf8 to utf16 conversion, pattern matching functions, etc. I added
implementations of the functions
we need to install_util.cc.

Once we test this out in the field and things work well, we will move the crash
pad registration into chrome_elf

BUG=604923

Committed: https://crrev.com/8d6f29a4043e293c44371496e9d729e2dfacd04e
Cr-Commit-Position: refs/heads/master@{#392834}
==========

commit-bot: I haz the power

Patchset 43 (id:??) landed as https://crrev.com/8d6f29a4043e293c44371496e9d729e2dfacd04e Cr-Commit-Position: refs/heads/master@{#392834}

4 years, 7 months ago (2016-05-11 02:39:04 UTC) #76

tapted

4 years, 7 months ago (2016-05-11 03:57:14 UTC) #77

Message was sent while issue was closed.

A revert of this CL (patchset #43 id:880001) has been created in
https://codereview.chromium.org/1963373003/ by tapted@chromium.org.

The reason for reverting is: Suspected for breaking compile on Official Win
build since

https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b...

with

installer_util.lib(installer_util.google_chrome_distribution.obj) :error
LNK2001: unresolved external symbol "bool __cdecl
install_static::GetDefaultCrashDumpLocation(..).

Issue 1913943003: Remove dependencies on chrome\installer from the ChromeCrashReporterClient class on Windows. (Closed)