content: Remove 16 exit time destructors and 15 static initializers.

content: Remove 16 exit time destructors and 15 static initializers.

BUG=101600, 94925
TEST=none
TBR=ben

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108982

[Nico, 2011-11-07 22:09:20]

mark: review
ben: OWNERS

Nearly done with non-third-party stuff.

[Mark Mentovai, 2011-11-07 22:34:14]

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:51: static const
MapEntry roles[] = {
While poking through here, I noticed…

This is both static and in an anonymous namespace. Remove the |static|.

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:224:
webAccessibilityToNativeRole, ());
This would properly be named web_accessibility_to_native_role, because you’re in
a mixed Objective-C++ files and you’re outside of an @interface,
@implementation, @protocol, etc. See
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Objec...
.

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:225: if
(webAccessibilityToNativeRole.empty()) {
I don’t like this pattern, because it treats “empty” as the
“initialized” sentinel, but the compiler already provides a sentinel for static
initialization. It’s less fragile to track something in one location and for the
meaning to be correct. Even though we have thread-safe statics off and I’m
assuming we don’t need thread safety here, “!empty” may not always equal
“initialized.”

I prefer something like this, and the compiled machine code will likely even be
a tiny bit more optimal:

std::map<WebAccessibility::Role, NSString*>* WebAccessibilityToNativeRoleMap() {
  std::map<WebAccessibility::Role, NSString*>* map = new std::map…;
  // Now you can even move the data in-line into this function.
  // initialize and return map;
}

NSString* NativeRoleFromWebAccessibilityRole(
    const WebAccessibility::Role& role) {
  static std::map<WebAccessibility::Role, NSString*>* map =
      WebAccessibilityToNativeRoleMap();
  // map().find, etc.
}

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:243:
webAccessibilityToNativeSubrole, ());
Same.

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:244: if
(webAccessibilityToNativeSubrole.empty()) {
And same.

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
File content/browser/browsing_instance.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
content/browser/browsing_instance.h:136: static base::LazyInstance<
private + static + in a .h file suggests that this can be made a non-member in
an anonymous namespace of the .cc file. It’s an implementation detail that
doesn’t need to be exposed here.

This occurs elsewhere in this change and I may not have marked each occurrence.

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/devto...
File content/browser/debugger/devtools_http_protocol_handler.cc (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/devto...
content/browser/debugger/devtools_http_protocol_handler.cc:128: static
base::LazyInstance<IdToTabContentsMap,
Note that I have no real concern with private + static + not in an .h file, as
in this case, when the class in question is restricted to an anonymous namespace
in a .cc file. You don’t get any further “hiding” advantage by taking the
variable out of the class, although I’d have no problem if you wanted to do
that, either.

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/rende...
File content/browser/debugger/render_view_devtools_agent_host.cc (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/rende...
content/browser/debugger/render_view_devtools_agent_host.cc:25:
instances_(base::LINKER_INITIALIZED);
This should not be named instances_ now.

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
File content/browser/net/url_request_mock_http_job.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
content/browser/net/url_request_mock_http_job.h:51: static FilePath& BasePath();
You’re making this return a non-const reference, and expecting a caller to
overwrite the value. Callers may not expect that. It’s worth a comment.

private + static, does this need to be here or can it move to an anonymous
namespace in the .cc file as an implementation detail?

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
content/browser/net/url_request_slow_download_job.h:62: static
base::LazyInstance<SlowJobsSet,
private + static again.

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
File content/browser/net/url_request_slow_http_job.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
content/browser/net/url_request_slow_http_job.h:40: static FilePath& BasePath();
Same comments as the other base_path_-to-BasePath transition I saw you doing.

[Nico, 2011-11-07 23:29:21]

Thanks!

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:224:
webAccessibilityToNativeRole, ());
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> This would properly be named web_accessibility_to_native_role, because you’re
in
> a mixed Objective-C++ files and you’re outside of an @interface,
> @implementation, @protocol, etc. See
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Objec...
> .

Done.

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:225: if
(webAccessibilityToNativeRole.empty()) {
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> I don’t like this pattern, because it treats “empty” as the
> “initialized” sentinel, but the compiler already provides a sentinel for
static
> initialization. It’s less fragile to track something in one location and for
the
> meaning to be correct. Even though we have thread-safe statics off and I’m
> assuming we don’t need thread safety here, “!empty” may not always equal
> “initialized.”
> 
> I prefer something like this, and the compiled machine code will likely even
be
> a tiny bit more optimal:
> 
> std::map<WebAccessibility::Role, NSString*>* WebAccessibilityToNativeRoleMap()
{
>   std::map<WebAccessibility::Role, NSString*>* map = new std::map…;
>   // Now you can even move the data in-line into this function.
>   // initialize and return map;
> }
> 
> NSString* NativeRoleFromWebAccessibilityRole(
>     const WebAccessibility::Role& role) {
>   static std::map<WebAccessibility::Role, NSString*>* map =
>       WebAccessibilityToNativeRoleMap();
>   // map().find, etc.
> }

Done-ish

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:243:
webAccessibilityToNativeSubrole, ());
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> Same.

Done.

http://codereview.chromium.org/8493016/diff/32/content/browser/accessibility/...
content/browser/accessibility/browser_accessibility_cocoa.mm:244: if
(webAccessibilityToNativeSubrole.empty()) {
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> And same.

Done.

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
File content/browser/browsing_instance.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
content/browser/browsing_instance.h:136: static base::LazyInstance<
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> private + static + in a .h file suggests that this can be made a non-member in
> an anonymous namespace of the .cc file. It’s an implementation detail that
> doesn’t need to be exposed here.

I tried that everywhere, but it wasn't trivially possible in all cases. Here,
the type ContextSiteInstanceMap was private to the class, and for some reason I
thought it was used in other places in the .h as well. In this case, I could
indeed move everything to the cc (done), but it wasn't always possible.

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/rende...
File content/browser/debugger/render_view_devtools_agent_host.cc (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/debugger/rende...
content/browser/debugger/render_view_devtools_agent_host.cc:25:
instances_(base::LINKER_INITIALIZED);
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> This should not be named instances_ now.

Done.

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
File content/browser/net/url_request_mock_http_job.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
content/browser/net/url_request_mock_http_job.h:51: static FilePath& BasePath();
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> You’re making this return a non-const reference, and expecting a caller to
> overwrite the value. Callers may not expect that. It’s worth a comment.

Done.

> private + static, does this need to be here or can it move to an anonymous
> namespace in the .cc file as an implementation detail?

I didn't do it because some comments referenced it. I updated the comments and
moved it over.

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/net/url_reques...
content/browser/net/url_request_slow_download_job.h:62: static
base::LazyInstance<SlowJobsSet,
On 2011/11/07 22:34:15, Mark Mentovai wrote:
> private + static again.

Since the comment for FinishPendingRequests() references this, I wanted to keep
it here. (I can't move FinishPrendingRequests() to the .cc because it calls
private methods of this class.)

[Nico, 2011-11-07 23:37:40]

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
File content/browser/browsing_instance.h (right):

http://codereview.chromium.org/8493016/diff/32/content/browser/browsing_insta...
content/browser/browsing_instance.h:136: static base::LazyInstance<
On 2011/11/07 23:29:21, Nico wrote:
> On 2011/11/07 22:34:15, Mark Mentovai wrote:
> > private + static + in a .h file suggests that this can be made a non-member
in
> > an anonymous namespace of the .cc file. It’s an implementation detail that
> > doesn’t need to be exposed here.
> 
> I tried that everywhere, but it wasn't trivially possible in all cases. Here,
> the type ContextSiteInstanceMap was private to the class, and for some reason
I
> thought it was used in other places in the .h as well. In this case, I could
> indeed move everything to the cc (done), but it wasn't always possible.

Ah, the problem was that the type depends on SiteInstanceMap, which is a private
typedef. Undid this.

[Mark Mentovai, 2011-11-08 00:31:28]

OK, LGTM.

[commit-bot: I haz the power, 2011-11-08 03:51:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/8493016/1054

[commit-bot: I haz the power, 2011-11-08 07:48:24]

Change committed as 108982

[Ben Goodger (Google), 2011-11-08 17:39:20]

LGTM2