Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12186016:
Add BrowserDistribution support for Win64 build (Closed)
|
Created:
7 years, 10 months ago by jschuh Modified:
7 years, 10 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd BrowserDistribution support for Win64 build
This was disabled for win64 nNaCl, but NaCl doesn't compile this file.
BUG=173911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180645
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #Messages
Total messages: 13 (0 generated)
Last blocker to getting chrome building on win64.
i'm not robertshield, but i play him on tv. https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution.cc:295: #if !defined(NACL_WIN64) under what conditions is this file compiled with NACL_WIN64 defined?
https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution.cc:295: #if !defined(NACL_WIN64) On 2013/02/04 19:03:47, grt wrote: > under what conditions is this file compiled with NACL_WIN64 defined? NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) as part of a 32-bit Chrome Windows build. It's done with a bunch of special purpose hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks like this one slipped through.
https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution.cc:295: #if !defined(NACL_WIN64) On 2013/02/04 19:16:41, Justin Schuh wrote: > On 2013/02/04 19:03:47, grt wrote: > > under what conditions is this file compiled with NACL_WIN64 defined? > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) as part > of a 32-bit Chrome Windows build. It's done with a bunch of special purpose > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks like > this one slipped through. I don't see this file ever being compiled with that #define, though. Perhaps you can just remove these ifdefs altogether.
https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_chrome_distribution.cc (right): https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_chrome_distribution.cc:295: #if !defined(NACL_WIN64) On 2013/02/04 19:34:39, grt wrote: > On 2013/02/04 19:16:41, Justin Schuh wrote: > > On 2013/02/04 19:03:47, grt wrote: > > > under what conditions is this file compiled with NACL_WIN64 defined? > > > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) as > part > > of a 32-bit Chrome Windows build. It's done with a bunch of special purpose > > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks like > > this one slipped through. > > I don't see this file ever being compiled with that #define, though. Perhaps you > can just remove these ifdefs altogether. What Greg said, at first glance it looks like everything (including NaCl targets) should just build without this #if
On 2013/02/04 19:34:39, grt wrote: > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > File chrome/installer/util/google_chrome_distribution.cc (right): > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > chrome/installer/util/google_chrome_distribution.cc:295: #if > !defined(NACL_WIN64) > On 2013/02/04 19:16:41, Justin Schuh wrote: > > On 2013/02/04 19:03:47, grt wrote: > > > under what conditions is this file compiled with NACL_WIN64 defined? > > > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) as > part > > of a 32-bit Chrome Windows build. It's done with a bunch of special purpose > > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks like > > this one slipped through. > > I don't see this file ever being compiled with that #define, though. Perhaps you > can just remove these ifdefs altogether. Hmm... are you saying NACL_WIN64 isn't getting propagated from common.gypi? If so, then I'd need to explicitly add this define for the installer_util_nacl_win64 target, because we don't want this code getting compiled in for nacl64.exe.
On 2013/02/04 19:49:14, Justin Schuh wrote: > On 2013/02/04 19:34:39, grt wrote: > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > File chrome/installer/util/google_chrome_distribution.cc (right): > > > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > chrome/installer/util/google_chrome_distribution.cc:295: #if > > !defined(NACL_WIN64) > > On 2013/02/04 19:16:41, Justin Schuh wrote: > > > On 2013/02/04 19:03:47, grt wrote: > > > > under what conditions is this file compiled with NACL_WIN64 defined? > > > > > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) as > > part > > > of a 32-bit Chrome Windows build. It's done with a bunch of special purpose > > > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > > > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks > like > > > this one slipped through. > > > > I don't see this file ever being compiled with that #define, though. Perhaps > you > > can just remove these ifdefs altogether. > > Hmm... are you saying NACL_WIN64 isn't getting propagated from common.gypi? If > so, then I'd need to explicitly add this define for the > installer_util_nacl_win64 target, because we don't want this code getting > compiled in for nacl64.exe. I'm saying that this file isn't one of those compiled into installer_util_nacl_win64. As far as I can tell, google_chrome_distribution.cc is *never* built into any of the magical targets introduced for NaCL 64-bit stuff. So why does it need any #if defined(...)?
On 2013/02/04 20:51:56, grt wrote: > On 2013/02/04 19:49:14, Justin Schuh wrote: > > On 2013/02/04 19:34:39, grt wrote: > > > > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > > File chrome/installer/util/google_chrome_distribution.cc (right): > > > > > > > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > > chrome/installer/util/google_chrome_distribution.cc:295: #if > > > !defined(NACL_WIN64) > > > On 2013/02/04 19:16:41, Justin Schuh wrote: > > > > On 2013/02/04 19:03:47, grt wrote: > > > > > under what conditions is this file compiled with NACL_WIN64 defined? > > > > > > > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) > as > > > part > > > > of a 32-bit Chrome Windows build. It's done with a bunch of special > purpose > > > > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > > > > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks > > like > > > > this one slipped through. > > > > > > I don't see this file ever being compiled with that #define, though. Perhaps > > you > > > can just remove these ifdefs altogether. > > > > Hmm... are you saying NACL_WIN64 isn't getting propagated from common.gypi? If > > so, then I'd need to explicitly add this define for the > > installer_util_nacl_win64 target, because we don't want this code getting > > compiled in for nacl64.exe. > > I'm saying that this file isn't one of those compiled into > installer_util_nacl_win64. As far as I can tell, google_chrome_distribution.cc > is *never* built into any of the magical targets introduced for NaCL 64-bit > stuff. So why does it need any #if defined(...)? Apologies in advance if I'm just being daft and not seeing something obvious. :-)
On 2013/02/04 20:52:31, grt wrote: > On 2013/02/04 20:51:56, grt wrote: > > On 2013/02/04 19:49:14, Justin Schuh wrote: > > > On 2013/02/04 19:34:39, grt wrote: > > > > > > > > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > > > File chrome/installer/util/google_chrome_distribution.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/12186016/diff/4002/chrome/installer/util/goog... > > > > chrome/installer/util/google_chrome_distribution.cc:295: #if > > > > !defined(NACL_WIN64) > > > > On 2013/02/04 19:16:41, Justin Schuh wrote: > > > > > On 2013/02/04 19:03:47, grt wrote: > > > > > > under what conditions is this file compiled with NACL_WIN64 defined? > > > > > > > > > > NACL_WIN64 is enabled when building a 64-bit NaCl container (nacl64.exe) > > as > > > > part > > > > > of a 32-bit Chrome Windows build. It's done with a bunch of special > > purpose > > > > > hacks like this, which unfortunately treated _WIN64 and NACL_WIN64 as > > > > > equivalent. Brad Nelson fixed most of them a few weeks ago, but it looks > > > like > > > > > this one slipped through. > > > > > > > > I don't see this file ever being compiled with that #define, though. > Perhaps > > > you > > > > can just remove these ifdefs altogether. > > > > > > Hmm... are you saying NACL_WIN64 isn't getting propagated from common.gypi? > If > > > so, then I'd need to explicitly add this define for the > > > installer_util_nacl_win64 target, because we don't want this code getting > > > compiled in for nacl64.exe. > > > > I'm saying that this file isn't one of those compiled into > > installer_util_nacl_win64. As far as I can tell, google_chrome_distribution.cc > > is *never* built into any of the magical targets introduced for NaCL 64-bit > > stuff. So why does it need any #if defined(...)? > > Apologies in advance if I'm just being daft and not seeing something obvious. > :-) Nope, my mistake. I misunderstood you and had thought it was listed in the shared sources section. You're right; it's listed as a source only for the installer_util target on 'OS=="win"'. So we can remove it entirely. I'll post a new CL.
Okay, I just removed the macros entirely. Sorry for my confusion there.
LGTM. Hope it works!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jschuh@chromium.org/12186016/4
Message was sent while issue was closed.
Change committed as 180645 |
