Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(366)

Issue 17379019: Seperate NaCl switches to their own file. (Closed)

Created:
7 years, 6 months ago by yael.aharon
Modified:
7 years, 6 months ago
CC:
chromium-reviews, native-client-reviews_googlegroups.com, Nico
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Separate NaCl switches to their own file. This is part of an effort to componentize NaCl code. BUG=244791 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207951

Patch Set 1 : #

Total comments: 4

Patch Set 2 : Add kNaClDebugMask and address nit #

Patch Set 3 : Move nacl_switches to components #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -40 lines) Patch
M chrome/app/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/app/breakpad_mac.mm View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/app/chrome_main_delegate.cc View 1 2 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/app/nacl_fork_delegate_linux.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/nacl_host/nacl_broker_host_win.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/nacl_host/nacl_process_host.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/nacl_host/test/nacl_gdb_browsertest.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_paths_win.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 chunks +0 lines, -7 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 chunks +0 lines, -30 lines 0 comments Download
M chrome/common/nacl_cmd_line.cc View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common_constants.gyp View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/nacl/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/nacl/nacl_broker_listener.cc View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/nacl/nacl_exe_win_64.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/nacl/nacl_main.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/nacl/nacl_main_platform_delegate_mac.mm View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/nacl/nacl_main_platform_delegate_win.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
A + components/nacl/OWNERS View 1 2 0 chunks +-1 lines, --1 lines 0 comments Download
A components/nacl/common/nacl_switches.h View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
A components/nacl/common/nacl_switches.cc View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A components/nacl_common.gypi View 1 2 1 chunk +41 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
yael.aharon1
Separated nacl switches to their own file. This is broken off from https://codereview.chromium.org/16881004/ This is ...
7 years, 6 months ago (2013-06-21 01:56:13 UTC) #1
Mark Seaborn
https://codereview.chromium.org/17379019/diff/9001/chrome/app/chrome_main_delegate.cc File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/17379019/diff/9001/chrome/app/chrome_main_delegate.cc#newcode542 chrome/app/chrome_main_delegate.cc:542: process_type == switches::kNaClLoaderProcess || Why not put "#if !defined(DISABLE_NACL)" ...
7 years, 6 months ago (2013-06-21 13:35:13 UTC) #2
yael.aharon1
On 2013/06/21 13:35:13, Mark Seaborn wrote: > https://codereview.chromium.org/17379019/diff/9001/chrome/app/chrome_main_delegate.cc > File chrome/app/chrome_main_delegate.cc (right): > > https://codereview.chromium.org/17379019/diff/9001/chrome/app/chrome_main_delegate.cc#newcode542 ...
7 years, 6 months ago (2013-06-21 13:55:55 UTC) #3
yael.aharon1
I added the missing switch (turns out it was missing from https://codereview.chromium.org/16881004/ too). Also fixed ...
7 years, 6 months ago (2013-06-21 14:06:44 UTC) #4
jam
lgtm you're going to have to move these new files and update the rest of ...
7 years, 6 months ago (2013-06-21 16:12:07 UTC) #5
yael.aharon1
joi@, can you please check the addition of nacl folder and nacl_common.gypi ? This has ...
7 years, 6 months ago (2013-06-21 16:48:16 UTC) #6
Jói
//components LGTM.
7 years, 6 months ago (2013-06-21 17:00:03 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yael.aharon@intel.com/17379019/20001
7 years, 6 months ago (2013-06-21 17:17:37 UTC) #8
commit-bot: I haz the power
Change committed as 207951
7 years, 6 months ago (2013-06-21 23:17:55 UTC) #9
Dirk Pranke
This change broke some of the chromium.webkit mac builders, which still use xcode. Apparently none ...
7 years, 6 months ago (2013-06-22 00:05:47 UTC) #10
yael.aharon1
On 2013/06/22 00:05:47, Dirk Pranke wrote: > This change broke some of the chromium.webkit mac ...
7 years, 6 months ago (2013-06-22 00:58:31 UTC) #11
Nico
On Fri, Jun 21, 2013 at 5:05 PM, <dpranke@chromium.org> wrote: > This change broke some ...
7 years, 6 months ago (2013-06-22 01:02:11 UTC) #12
Nico
7 years, 6 months ago (2013-06-22 01:02:12 UTC) #13
On Fri, Jun 21, 2013 at 5:05 PM, <dpranke@chromium.org> wrote:

> This change broke some of the chromium.webkit mac builders, which still use
> xcode. Apparently none of the default try servers, nor the main waterfall
> bots
> still use xcode, which is rather unfortunate given that that's what we
> ship ...
>

There's an opt-in try bot (mac_xcodebuild or mac_xcode I think). Since
these problems are rare, that's the right cost-benefit tradeoff.

The clobber builders on the main waterfall use xcodebuild. I agree it's
unfortunate that there are no incremental xcode bots. I asked iannucci to
change some of them back, not sure why that hasn't happened yet. I'll ping
that bug.


>
> I reverted the change in r207964: https://codereview.chromium.**
> org/16867015 <https://codereview.chromium.org/16867015>
>
>
https://chromiumcodereview.**appspot.com/17379019/<https://chromiumcodereview...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.


Powered by Google App Engine
This is Rietveld 408576698