Issue 10690133: Get mac_logging and foundation_util working on iOS

Issue 10690133: Get mac_logging and foundation_util working on iOS (Closed)

Created:
8 years, 5 months ago by stuartmorgan

Modified:
8 years, 5 months ago

Reviewers:
Mark Mentovai

CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Get mac_logging and foundation_util working on iOS BUG=None TEST=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146117

Patch Set 2 : Address review comments #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+34 lines, -4 lines)			Patch
M	base/base.gypi	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	base/mac/foundation_util.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	base/mac/foundation_util.mm	View	1	5 chunks	+14 lines, -0 lines	0 comments	Download
M	base/mac/mac_logging.h	View	1	1 chunk	+7 lines, -2 lines	0 comments	Download
M	base/mac/mac_logging.cc	View		2 chunks	+10 lines, -2 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Mark Mentovai

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/foundation_util.mm File base/mac/foundation_util.mm (right): https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/foundation_util.mm#newcode35 base/mac/foundation_util.mm:35: // All apps are bundled on iOS Then should ...

8 years, 5 months ago (2012-07-11 12:42:51 UTC) #2

stuartmorgan

8 years, 5 months ago (2012-07-11 13:18:42 UTC) #3

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/foundation_ut...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/foundation_ut...
base/mac/foundation_util.mm:35: // All apps are bundled on iOS
On 2012/07/11 12:42:51, Mark Mentovai wrote:
> Then should this happen before the g_override_am_i_bundled check? Does iOS
ever
> override something to pretend to not be bundled?

Good point. No, nothing does (there's only one call to SetOverrideAmIBundled in
the whole codebase, in fact, and it's 'true'). Done, and I also added a check to
SetOverrideAmIBundled just to make sure that if some tries for some crazy reason
to do it in the future, it will fail.

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/mac_logging.h
File base/mac/mac_logging.h (right):

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/mac_logging.h...
base/mac/mac_logging.h:9: #include "base/compiler_specific.h"
On 2012/07/11 12:42:51, Mark Mentovai wrote:
> You want:
> 
> #include "build/build_config.h"
> 
> for the OS_ macros. Not compiler_specific.h.

Done.

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/mac_logging.h...
base/mac/mac_logging.h:10: #include "base/logging.h"
On 2012/07/11 12:42:51, Mark Mentovai wrote:
> Then you want to put this #include for base/logging.h back where you found it,
> after the system includes. build_config.h gets to go up top because it’s
needed
> for the OS_* defines. logging.h still has to come after the system headers.

Hm, the reason I moved this is that the system headers are now
platform-specific, and our style says those go at the end:

#includes should be ordered per Google style, except platform-specific (#if
defined(..)) headers to follow the cross platform headers in a separate section
-- see the "Platform-specific code" section.

I thought that was true even for system headers, based on things I've seen in
other files. (See, e.g., base/time.h)

https://chromiumcodereview.appspot.com/10690133/diff/1/base/mac/mac_logging.h...
base/mac/mac_logging.h:13: #include "MacTypes.h"
On 2012/07/11 12:42:51, Mark Mentovai wrote:
> Very odd to see this in quotes. It should be <MacTypes.h>.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10690133/5001

8 years, 5 months ago (2012-07-11 13:58:08 UTC) #5

Change committed as 146117

Expand Messages | Collapse Messages