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

Issue 750273002: Fix implicit scoped_refptr<T> to T* conversion in TestMetroViewerProcessHost (Closed)

Created:
6 years, 1 month ago by dcheng
Modified:
6 years ago
Reviewers:
Nico, Daniel Erat
CC:
chromium-reviews, jam, kalyank, sadrul, Nico
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix implicit scoped_refptr<T> to T* conversion in TestMetroViewerProcessHost Since MetroViewerProcessHost ends up ref'ing the TaskRunner, pass it as a scoped_refptr<T> to provide a hint to readers and eliminate the implicit conversion. BUG=110610 Committed: https://crrev.com/f62e00050f20843892035e460db6fe99e0f381b2 Cr-Commit-Position: refs/heads/master@{#305484}

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -4 lines) Patch
M ash/test/test_metro_viewer_process_host.h View 1 chunk +2 lines, -1 line 0 comments Download
M ash/test/test_metro_viewer_process_host.cc View 1 chunk +2 lines, -3 lines 3 comments Download

Messages

Total messages: 15 (3 generated)
dcheng
6 years ago (2014-11-24 18:12:45 UTC) #2
Daniel Erat
lgtm https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc File ash/test/test_metro_viewer_process_host.cc (right): https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc#newcode18 ash/test/test_metro_viewer_process_host.cc:18: : MetroViewerProcessHost(ipc_task_runner), closed_unexpectedly_(false) { nit: back to one ...
6 years ago (2014-11-24 18:53:55 UTC) #3
dcheng
+CCing some people who may have thoughts on C++ style -- if this is actually ...
6 years ago (2014-11-24 19:02:36 UTC) #4
Nico
https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc File ash/test/test_metro_viewer_process_host.cc (right): https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc#newcode18 ash/test/test_metro_viewer_process_host.cc:18: : MetroViewerProcessHost(ipc_task_runner), closed_unexpectedly_(false) { On 2014/11/24 18:53:55, Daniel Erat ...
6 years ago (2014-11-24 19:05:22 UTC) #6
Peter Kasting
On 2014/11/24 19:02:36, dcheng wrote: > +CCing some people who may have thoughts on C++ ...
6 years ago (2014-11-24 19:06:30 UTC) #7
Peter Kasting
On 2014/11/24 19:05:22, Nico wrote: > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc > File ash/test/test_metro_viewer_process_host.cc (right): > > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_process_host.cc#newcode18 > ...
6 years ago (2014-11-24 19:10:01 UTC) #8
dcheng
Would it make sense to have some sort of doc to document clang-format decisions somewhere? ...
6 years ago (2014-11-24 19:17:32 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/750273002/1
6 years ago (2014-11-24 19:29:01 UTC) #11
Peter Kasting
On 2014/11/24 19:17:32, dcheng wrote: > Would it make sense to have some sort of ...
6 years ago (2014-11-24 19:47:23 UTC) #12
commit-bot: I haz the power
Committed patchset #1 (id:1)
6 years ago (2014-11-24 20:13:29 UTC) #13
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/f62e00050f20843892035e460db6fe99e0f381b2 Cr-Commit-Position: refs/heads/master@{#305484}
6 years ago (2014-11-24 20:14:17 UTC) #14
Nico
6 years ago (2014-11-24 21:49:19 UTC) #15
Message was sent while issue was closed.
On Mon, Nov 24, 2014 at 11:10 AM, <pkasting@chromium.org> wrote:

> On 2014/11/24 19:05:22, Nico wrote:
>
> https://codereview.chromium.org/750273002/diff/1/ash/test/
> test_metro_viewer_process_host.cc
>
>> File ash/test/test_metro_viewer_process_host.cc (right):
>>
>
>
> https://codereview.chromium.org/750273002/diff/1/ash/test/
> test_metro_viewer_process_host.cc#newcode18
>
>> ash/test/test_metro_viewer_process_host.cc:18: :
>> MetroViewerProcessHost(ipc_task_runner), closed_unexpectedly_(false) {
>> On 2014/11/24 18:53:55, Daniel Erat wrote:
>> > nit: back to one per line since they don't all fit on the same line as
>> the
>> > signature
>> >
>> > (does clang-format still do this? :-( )
>>
>
>  I think clang-format used to do one-per-line-or-all-on-one-line, and then
>> pkasting decreed that that isn't correct and clang-format changed in
>> response
>>
> to
>
>> that, see: https://code.google.com/p/chromium/issues/detail?id=430111
>>
>
>  So I think this is working as intended.
>>
>
> This has nothing to do with that bug -- it's a completely separate
> formatting
> decision and was made earlier than the change that bug references.
>
> I also really wish you'd not use language like "pkasting decreed X".


Hey, sorry about the language. It wasn't meant with any negative
connotation if that helps – think s/decreed/drove the decision that/ (but
it sounds like that bug is unrelated to this snippet anyways).


>   I am not
> the style dictator for Chromium and when we've made behavior changes the
> rule
> certainly isn't "whatever pkasting wants".  As I noted, in this case I
> actually
> would have rather clang-format done something different, so if it were up
> to my
> decrees, we wouldn't have the behavior we have now.
>
> I am particularly sensitive to this since I think some people do, in fact,
> think
> that the decision-making process involves me acting like a dictator, and I
> don't
> want that viewpoint reinforced.
>
> https://codereview.chromium.org/750273002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698