LGTM with one suggestion http://codereview.chromium.org/2815042/diff/1/2 File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right): http://codereview.chromium.org/2815042/diff/1/2#newcode121 chrome/browser/cocoa/content_blocked_bubble_controller.mm:121: COMPILE_ASSERT(arraysize(nibPaths) + 1 == CONTENT_SETTINGS_NUM_TYPES, ...
4 years, 11 months ago
(2010-07-01 21:34:05 UTC)
#2
LGTM with one suggestion
http://codereview.chromium.org/2815042/diff/1/2
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):
http://codereview.chromium.org/2815042/diff/1/2#newcode121
chrome/browser/cocoa/content_blocked_bubble_controller.mm:121:
COMPILE_ASSERT(arraysize(nibPaths) + 1 == CONTENT_SETTINGS_NUM_TYPES,
given you have the CHECK_NE, it's probably better to put a "" string in nibPaths
instead of the +1 otherwise anything that comes along later will be broken.
4 years, 11 months ago
(2010-07-01 21:43:25 UTC)
#3
Thanks.
http://codereview.chromium.org/2815042/diff/1/2
File chrome/browser/cocoa/content_blocked_bubble_controller.mm (right):
http://codereview.chromium.org/2815042/diff/1/2#newcode121
chrome/browser/cocoa/content_blocked_bubble_controller.mm:121:
COMPILE_ASSERT(arraysize(nibPaths) + 1 == CONTENT_SETTINGS_NUM_TYPES,
On 2010/07/01 21:34:05, John Gregg wrote:
> given you have the CHECK_NE, it's probably better to put a "" string in
nibPaths
> instead of the +1 otherwise anything that comes along later will be broken.
Done.
Issue 2815042: Add notifications content settings type.
(Closed)
Created 4 years, 11 months ago by Nico
Modified 4 years ago
Reviewers: John Gregg
Base URL:
Comments: 2