|
|
DescriptionFix 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
Messages
Total messages: 15 (3 generated)
dcheng@chromium.org changed reviewers: + derat@chromium.org
lgtm https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... File ash/test/test_metro_viewer_process_host.cc (right): https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... ash/test/test_metro_viewer_process_host.cc:18: : MetroViewerProcessHost(ipc_task_runner), closed_unexpectedly_(false) { 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? :-( )
+CCing some people who may have thoughts on C++ style -- if this is actually the behavior we want for Chromium, we should file a bug upstream. https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... File ash/test/test_metro_viewer_process_host.cc (right): https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... 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? :-( ) Yes. I'd prefer not to fight clang-format, because that means I'll have to keep fighting clang-format in every patch that ever touches this code. I don't think this is actually against the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I... states that "[c]onstructor initializer lists can be all on one line". It does not mention anything about the initializer list having to be on the same line as all the parameters. I see no Chromium-specific style guidelines that would apply in this situation.
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... File ash/test/test_metro_viewer_process_host.cc (right): https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... 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.
On 2014/11/24 19:02:36, dcheng wrote: > +CCing some people who may have thoughts on C++ style -- if this is actually the > behavior we want for Chromium, we should file a bug upstream. > > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... > File ash/test/test_metro_viewer_process_host.cc (right): > > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... > 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? :-( ) > > Yes. I'd prefer not to fight clang-format, because that means I'll have to keep > fighting clang-format in every patch that ever touches this code. > > I don't think this is actually against the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I... > states that "[c]onstructor initializer lists can be all on one line". It does > not mention anything about the initializer list having to be on the same line as > all the parameters. I see no Chromium-specific style guidelines that would apply > in this situation. This was discussed previously (I'm a fan of one-per-line in this case too) and we decided to allow clang-format's current behavior, basically for the reasons stated above.
On 2014/11/24 19:05:22, Nico wrote: > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... > File ash/test/test_metro_viewer_process_host.cc (right): > > https://codereview.chromium.org/750273002/diff/1/ash/test/test_metro_viewer_p... > 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". 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.
Would it make sense to have some sort of doc to document clang-format decisions somewhere? I think we have one for the internal Google C++ style decisions, and having an external one would make my life a lot easier (and probably scale better).
The CQ bit was checked by dcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/750273002/1
On 2014/11/24 19:17:32, dcheng wrote: > Would it make sense to have some sort of doc to document clang-format decisions > somewhere? I think we have one for the internal Google C++ style decisions, and > having an external one would make my life a lot easier (and probably scale > better). Well, in a case like this, we're simply not making a Chromium exception to the default Google style. If we were going to justify particular decisions, I think we'd probably do so for cases where we actually do something different? I dunno, maybe I'm mostly just lazy and don't want to volunteer to create such a doc.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f62e00050f20843892035e460db6fe99e0f381b2 Cr-Commit-Position: refs/heads/master@{#305484}
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. |