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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 212103002: Check NaCl debug mask (not just flag) before choosing PNaCl debug URL. (Closed)

Created:
6 years, 9 months ago by jvoung (off chromium)
Modified:
6 years, 9 months ago
CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, raymes+watch_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, native-client-reviews_googlegroups.com, noelallen1, binji, ihf+watch_chromium.org
Visibility:
Public.

Description

Check NaCl debug mask (not just flag) before choosing PNaCl debug URL. That way, the debug URL is chosen only if the app will be debugged. BUG=http://code.google.com/p/nativeclient/issues/detail?id=3765 R=bradnelson@google.com, tsepez@chromium.org TBR=bradnelson@chromium.org, teravest@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260067

Patch Set 1 #

Patch Set 2 : have python help continue #

Patch Set 3 : rebase #

Total comments: 3

Patch Set 4 : rebase #

Patch Set 5 : add comments, disable on windows #

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -40 lines) Patch
M chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/test/nacl/nacl_browsertest.cc View 1 2 3 4 4 chunks +89 lines, -25 lines 0 comments Download
M components/nacl/browser/nacl_host_message_filter.h View 1 chunk +2 lines, -0 lines 0 comments Download
M components/nacl/browser/nacl_host_message_filter.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M components/nacl/common/nacl_host_messages.h View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M components/nacl/renderer/ppb_nacl_private_impl.cc View 1 2 3 4 5 2 chunks +13 lines, -4 lines 0 comments Download
M ppapi/api/private/ppb_nacl_private.idl View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M ppapi/c/private/ppb_nacl_private.h View 1 2 3 4 5 2 chunks +4 lines, -4 lines 0 comments Download
M ppapi/native_client/src/trusted/plugin/plugin.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
jvoung (off chromium)
tsepez for nacl_host_messages OWNERS brad for chrome/test/nacl OWNERS teravest for ppapi OWNERS Thanks!
6 years, 9 months ago (2014-03-26 23:16:59 UTC) #1
Tom Sepez
Messages LGTM. https://codereview.chromium.org/212103002/diff/40001/components/nacl/common/nacl_host_messages.h File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/212103002/diff/40001/components/nacl/common/nacl_host_messages.h#newcode109 components/nacl/common/nacl_host_messages.h:109: Nit: maybe a comment as all but ...
6 years, 9 months ago (2014-03-26 23:25:11 UTC) #2
teravest
ppapi/ and components/nacl/renderer/ look good to me.
6 years, 9 months ago (2014-03-27 15:35:50 UTC) #3
bradn
lgtm https://codereview.chromium.org/212103002/diff/40001/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html File chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html (right): https://codereview.chromium.org/212103002/diff/40001/chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html#newcode24 chrome/test/data/nacl/pnacl_debug_url/pnacl_debug_url.html:24: embed.parentNode.removeChild(embed); FWIW I've observed that the debug stub ...
6 years, 9 months ago (2014-03-27 16:57:14 UTC) #4
jvoung (off chromium)
Thanks! Had to disable the test on windows for now... The debug stub socket is ...
6 years, 9 months ago (2014-03-27 20:41:24 UTC) #5
jvoung (off chromium)
The CQ bit was checked by jvoung@chromium.org
6 years, 9 months ago (2014-03-27 20:58:41 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jvoung@chromium.org/212103002/80001
6 years, 9 months ago (2014-03-27 21:05:13 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-27 21:21:59 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
6 years, 9 months ago (2014-03-27 21:21:59 UTC) #9
jvoung (off chromium)
The CQ bit was checked by jvoung@chromium.org
6 years, 9 months ago (2014-03-27 21:46:52 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jvoung@chromium.org/212103002/100001
6 years, 9 months ago (2014-03-27 21:49:35 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-27 22:16:56 UTC) #12
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=58104
6 years, 9 months ago (2014-03-27 22:16:57 UTC) #13
jvoung (off chromium)
6 years, 9 months ago (2014-03-28 02:48:11 UTC) #14
Message was sent while issue was closed. 
Committed patchset #6 manually as r260067 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698