mac: Replace base::mac::ScopedCFTypeRef with base::ScopedCFTypeRef.

mac: Replace base::mac::ScopedCFTypeRef with base::ScopedCFTypeRef.

This CL was created fully mechanically by running

  git grep -l base::mac::ScopedCFTypeRef | xargs sed -i -e 's/base::mac::ScopedCFTypeRef/base::ScopedCFTypeRef/g'
  git commit -a -m.
  git clang-format HEAD^ --style=Chromium
  git commit -a -m.
  git cl upload -t $TITLE

BUG=251957
TBR=mark@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208245

[Mark Mentovai, 2013-06-21 20:29:40]

The code change itself looks good for sure.

With this change, you can also dump the “using” in base::mac.

The automatic reformatting is impressive, but I’ve got a few questions about
some reformats that I see it doing that are in some cases worse, and in other
cases simply “no better” (but sometimes they’re formatting-only changes, which I
didn’t expect to see for a reformatted diff.) The same few patterns kept showing
up again and again, so rather than scrutinizing each and every file, I stopped
after a while to just ask about these. I didn’t mark each occurrence I saw.

https://codereview.chromium.org/16917011/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: NULL, (const
UInt8*)path.value().c_str(), path.value().length(), true));
The automatic formatter has done this a handful of times, moving arguments that
had been on separate lines into a single line. In some cases it’s OK, but in
others, I think having the arguments on their own lines is more readable. In
this case, the arguments are harder to spot just by looking for commas because
there’s a lot of funny punctuation going on on this line, so I think the line
breaks were helpful.

https://codereview.chromium.org/16917011/diff/1/chrome/browser/mac/install_fr...
File chrome/browser/mac/install_from_dmg.mm (right):

https://codereview.chromium.org/16917011/diff/1/chrome/browser/mac/install_fr...
chrome/browser/mac/install_from_dmg.mm:126: hdix_drive, CFSTR("image-path"),
NULL, 0));
Here’s another, although it’s arguably less of a “readability” issue here, the
existing formatting was just fine.

https://codereview.chromium.org/16917011/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:622: CFStringRef
country =
The only reason I can see that this was touched is that it’s right next to a
line in the diff. But it shouldn’t have otherwise been a ± line in the diff on
its own, so why was it touched?

https://codereview.chromium.org/16917011/diff/1/chrome/common/mac/mock_launch...
File chrome/common/mac/mock_launchd.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/common/mac/mock_launch...
chrome/common/mac/mock_launchd.cc:130: kCFAllocatorDefault, pipe_name_.c_str(),
kCFStringEncodingUTF8));
On something like this, the old argument formatting was nonconformant, but I
still think that it’s more readable to put the line break after socket_path( so
that you can see the CFStringCreateWithCString function call along with at least
its first argument.

Conceptually this statement does the same thing as the previous, except it’s
using a constructor to initialize something instead of an assignment. But I
wouldn’t expect to read
  CFStringRef env_var = base::mac::NSToCFCast(
      GetServiceProcessLaunchDSocketEnvVar());
I’d expect the break after the =.

https://codereview.chromium.org/16917011/diff/1/chrome/common/mac/mock_launch...
chrome/common/mac/mock_launchd.cc:175: base::ScopedCFTypeRef<CFArrayRef>
args(CFArrayCreate(
Same here.

[Nico, 2013-06-21 21:09:06]

Thanks for the review! You found one git-clang-format bug which I'll work with
upstream to get resolved.

As for style updates, as far as I know using clang-format to do cleanup after
largeish refactorings is widely accepted. If you don't like a formatting
produced by clang-format, it's better to file a bug at go/clang-format-bug
(these bugs are actively looked at, but people often ask for contradictory
things, so the clang-format team tries to come up with a consensus that mostly
works for everyone).

(In the case of this change: since the change makes lines shorter, an
alternative would be to not run clang-format, which is fine with me too. But for
scoped_nsobject->base::scoped_nsobject it won't be an option).

https://codereview.chromium.org/16917011/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: NULL, (const
UInt8*)path.value().c_str(), path.value().length(), true));
On 2013/06/21 20:29:40, Mark Mentovai wrote:
> The automatic formatter has done this a handful of times, moving arguments
that
> had been on separate lines into a single line. In some cases it’s OK, but in
> others, I think having the arguments on their own lines is more readable. In
> this case, the arguments are harder to spot just by looking for commas because
> there’s a lot of funny punctuation going on on this line, so I think the line
> breaks were helpful.

I think the rule clang-format uses is "if all parameters fit on a single line
put them on a single line, else use one line per argument" (with
-style=Chromium. with -style=Google, it packs them as tightly as it can.)

https://codereview.chromium.org/16917011/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:622: CFStringRef
country =
On 2013/06/21 20:29:40, Mark Mentovai wrote:
> The only reason I can see that this was touched is that it’s right next to a
> line in the diff. But it shouldn’t have otherwise been a ± line in the diff on
> its own, so why was it touched?

Huh, this is strange. Looking…

I think this is a bug in git-clang-format. Asked here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130617/082383.html

https://codereview.chromium.org/16917011/diff/1/chrome/common/mac/mock_launch...
File chrome/common/mac/mock_launchd.cc (right):

https://codereview.chromium.org/16917011/diff/1/chrome/common/mac/mock_launch...
chrome/common/mac/mock_launchd.cc:130: kCFAllocatorDefault, pipe_name_.c_str(),
kCFStringEncodingUTF8));
On 2013/06/21 20:29:40, Mark Mentovai wrote:
> On something like this, the old argument formatting was nonconformant, but I
> still think that it’s more readable to put the line break after socket_path(
so
> that you can see the CFStringCreateWithCString function call along with at
least
> its first argument.
> 
> Conceptually this statement does the same thing as the previous, except it’s
> using a constructor to initialize something instead of an assignment. But I
> wouldn’t expect to read
>   CFStringRef env_var = base::mac::NSToCFCast(
>       GetServiceProcessLaunchDSocketEnvVar());
> I’d expect the break after the =.

For C++ code, the formatting produced by clang-format has gone through many
rounds of discussions  on c-style, and what it currently does (for
-style=Google) is Google-internal agreed-upon formatting. (-style=Chromium does
a few tweaks on top of that based on
http://www.chromium.org/developers/coding-style). If you disagree with a format
decision, it's best to file a bug at go/clang-format-bug

[Mark Mentovai, 2013-06-21 21:43:08]

Nico wrote:
> I think the rule clang-format uses is "if all parameters fit on a single line
> put them on a single line, else use one line per argument" (with
> -style=Chromium. with -style=Google, it packs them as tightly as it can.)

Is there any provision for “leave it as written if what’s written is valid?”

[Nico, 2013-06-21 21:48:08]

On Fri, Jun 21, 2013 at 2:43 PM, <mark@chromium.org> wrote:

> Nico wrote:
>
>> I think the rule clang-format uses is "if all parameters fit on a single
>> line
>> put them on a single line, else use one line per argument" (with
>> -style=Chromium. with -style=Google, it packs them as tightly as it can.)
>>
>
> Is there any provision for “leave it as written if what’s written is
> valid?”
>

Where "valid" means "no line longer than 80 chars"? What about lines that
become short enough that reflowing is useful? "only reformat lines that are
>= 80 chars and <= ~40"?


>
>
https://codereview.chromium.**org/16917011/<https://codereview.chromium.org/1...
>

[Nico, 2013-06-21 22:02:36]

On Fri, Jun 21, 2013 at 2:48 PM, Nico Weber <thakis@chromium.org> wrote:

> On Fri, Jun 21, 2013 at 2:43 PM, <mark@chromium.org> wrote:
>
>> Nico wrote:
>>
>>> I think the rule clang-format uses is "if all parameters fit on a single
>>> line
>>> put them on a single line, else use one line per argument" (with
>>> -style=Chromium. with -style=Google, it packs them as tightly as it can.)
>>>
>>
>> Is there any provision for “leave it as written if what’s written is
>> valid?”
>>
>
> Where "valid" means "no line longer than 80 chars"? What about lines that
> become short enough that reflowing is useful? "only reformat lines that are
> >= 80 chars and <= ~40"?
>

(To be clear, I think that's a good suggestion. I'm just not sure if
there's a good way to define "valid" that isn't "run clang-format and check
if its output is identical to what's currently there". Reformatting only
lines > 80 cols is a reasonable approximation probably, but you'd end up
with weird-looking lines with that too, so I'm not sure if it's better.)


>
>
>>
>>
https://codereview.chromium.**org/16917011/<https://codereview.chromium.org/1...
>>
>
>

[Mark Mentovai, 2013-06-21 22:15:30]

Right, I see what you mean. I realized that on the way home. I was just trying
to figure out how to write an intelligent request for clang-format.

Anyway, I think that if the git-clang-format bug you reported gets fixed, the
automatic formatting will probably be good enough to check in. Can we wait for
that to get fixed? Since this was just a mechanical job, I’m assuming there’s
not much risk of this patch bitrotting in a way that causes you to actually have
to do substantial work to recover. You can just redo the patch by running the
same commands again, right?

[Nico, 2013-06-21 22:25:02]

On 2013/06/21 22:15:30, Mark Mentovai wrote:
> Right, I see what you mean. I realized that on the way home. I was just trying
> to figure out how to write an intelligent request for clang-format.
> 
> Anyway, I think that if the git-clang-format bug you reported gets fixed, the
> automatic formatting will probably be good enough to check in. Can we wait for
> that to get fixed? Since this was just a mechanical job, I’m assuming there’s
> not much risk of this patch bitrotting in a way that causes you to actually
have
> to do substantial work to recover. You can just redo the patch by running the
> same commands again, right?

Yes, it's easy. However, I applied a fix to git-clang-format locally [1], which
seems to do the trick. Patch set 2 was produced with that fix (affected only 3
files).

[1]:
$ svn diff
Index: tools/clang-format/git-clang-format
===================================================================
--- tools/clang-format/git-clang-format	(revision 184475)
+++ tools/clang-format/git-clang-format	(working copy)
@@ -329,7 +329,7 @@
     if linenum == r.start:
       byte_start = byte_idx
     if linenum == r.start + r.count:
-      byte_ranges.append(Range(byte_start, byte_idx - byte_start))
+      byte_ranges.append(Range(byte_start, byte_idx - byte_start - 1))
       r = next(line_ranges_iter)
     linenum += 1
     byte_idx += len(line)

[Mark Mentovai, 2013-06-24 15:06:28]

LGTM

[commit-bot: I haz the power, 2013-06-24 15:38:38]

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

[commit-bot: I haz the power, 2013-06-24 15:51:34]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-24 18:36:53]

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

[Nico, 2013-06-24 18:49:15]

Committed patchset #2 manually as r208245 (presubmit successful).