Rename media::GpuVideoDecodeAcceleratorFactoryImpl to media::GpuVideoDecodeAcceleratorFactory

Rename media::GpuVideoDecodeAcceleratorFactoryImpl to media::GpuVideoDecodeAcceleratorFactory

BUG=597150
Committed: https://crrev.com/b52f7da5b93799d6129ce2ad6594625c3261d032
Cr-Commit-Position: refs/heads/master@{#407963}

[piman, 2016-07-22 19:19:28]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 19:20:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-07-22 19:21:41]

piman@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[piman, 2016-07-22 19:21:42]

Per suggestion from https://codereview.chromium.org/2171473003/, but moved to a
separate CL so that we can add to .git-blame-ignore-revs

[DaleCurtis, 2016-07-22 19:25:18]

lgtm

[commit-bot: I haz the power, 2016-07-22 22:28:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 22:28:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Pawel Osciak, 2016-07-26 00:57:25]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2016-07-26 00:57:26]

lgtm % nit

https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
File media/gpu/gpu_video_decode_accelerator_factory.cc (right):

https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
media/gpu/gpu_video_decode_accelerator_factory.cc:72: // Query VDAs for their
capabilities and construct a set of supported
I've always been wondering about this, but should the comment rather be aligned
to the current code indent (one to the right) to which it refers, as it was
before, not the preprocessor macro?

[piman, 2016-07-26 05:48:39]

On Mon, Jul 25, 2016 at 5:57 PM, <posciak@chromium.org> wrote:

> lgtm % nit
>
>
>
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> File media/gpu/gpu_video_decode_accelerator_factory.cc (right):
>
>
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> media/gpu/gpu_video_decode_accelerator_factory.cc:72: // Query VDAs for
> their capabilities and construct a set of supported
> I've always been wondering about this, but should the comment rather be
> aligned to the current code indent (one to the right) to which it
> refers, as it was before, not the preprocessor macro?
>

This is what 'git cl format' produced.

Do you care enough about this that we should diverge from clang-format?



> https://chromiumcodereview.appspot.com/2178603002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Pawel Osciak, 2016-07-26 06:11:54]

On 2016/07/26 05:48:39, piman - slow reviews until 8-8 wrote:
> On Mon, Jul 25, 2016 at 5:57 PM, <mailto:posciak@chromium.org> wrote:
> 
> > lgtm % nit
> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> > File media/gpu/gpu_video_decode_accelerator_factory.cc (right):
> >
> >
> >
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> > media/gpu/gpu_video_decode_accelerator_factory.cc:72: // Query VDAs for
> > their capabilities and construct a set of supported
> > I've always been wondering about this, but should the comment rather be
> > aligned to the current code indent (one to the right) to which it
> > refers, as it was before, not the preprocessor macro?
> >
> 
> This is what 'git cl format' produced.
> 

Yes, I've always used to revert this change back.

> Do you care enough about this that we should diverge from clang-format?
> 

Personally I prefer alignment to the current indent level as well as the context
(which is not the preprocessor macro), I'm not sure if git cl format is right in
what it is doing, but I don't feel very strongly about this. Please feel free to
follow your preference.

[piman, 2016-07-26 06:32:54]

On Mon, Jul 25, 2016 at 11:11 PM, <posciak@chromium.org> wrote:

> On 2016/07/26 05:48:39, piman - slow reviews until 8-8 wrote:
> > On Mon, Jul 25, 2016 at 5:57 PM, <mailto:posciak@chromium.org> wrote:
> >
> > > lgtm % nit
> > >
> > >
> > >
> > >
> >
>
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> > > File media/gpu/gpu_video_decode_accelerator_factory.cc (right):
> > >
> > >
> > >
> >
>
>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
> > > media/gpu/gpu_video_decode_accelerator_factory.cc:72: // Query VDAs for
> > > their capabilities and construct a set of supported
> > > I've always been wondering about this, but should the comment rather be
> > > aligned to the current code indent (one to the right) to which it
> > > refers, as it was before, not the preprocessor macro?
> > >
> >
> > This is what 'git cl format' produced.
> >
>
> Yes, I've always used to revert this change back.
>
> > Do you care enough about this that we should diverge from clang-format?
> >
>
> Personally I prefer alignment to the current indent level as well as the
> context
> (which is not the preprocessor macro), I'm not sure if git cl format is
> right in
> what it is doing, but I don't feel very strongly about this. Please feel
> free to
> follow your preference.
>

I think the indented version in this case could make sense, because the
comment is about the c++ code, not the #ifdef, but if it was about the
latter (e.g. "Disable on Win32 because reasons XYZ"), it would make sense
to have the comment aligned with the #ifdef.
clang-format can't understand which one it is, so it picks one (and
aligning to the #ifdef is probably the most useful... oftentimes the
comment about the c++ code would be inside the #ifdef, but if it's outside
it more likely than not refers to the #ifdef itself).

My suggestion is to either let clang-format do what it wants to do or to
disable it on this comment by adding the "// clang-format off" - otherwise
it's just make-work for anyone who touches this file.


>
> https://chromiumcodereview.appspot.com/2178603002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-07-26 20:49:59]

The CQ bit was checked by piman@chromium.org

[piman, 2016-07-26 20:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/2178603002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-26 20:50:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-07-26 20:50:53]

On Mon, Jul 25, 2016 at 11:32 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Jul 25, 2016 at 11:11 PM, <posciak@chromium.org> wrote:
>
>> On 2016/07/26 05:48:39, piman - slow reviews until 8-8 wrote:
>> > On Mon, Jul 25, 2016 at 5:57 PM, <mailto:posciak@chromium.org> wrote:
>> >
>> > > lgtm % nit
>> > >
>> > >
>> > >
>> > >
>> >
>>
>>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
>> > > File media/gpu/gpu_video_decode_accelerator_factory.cc (right):
>> > >
>> > >
>> > >
>> >
>>
>>
https://chromiumcodereview.appspot.com/2178603002/diff/1/media/gpu/gpu_video_...
>> > > media/gpu/gpu_video_decode_accelerator_factory.cc:72: // Query VDAs
>> for
>> > > their capabilities and construct a set of supported
>> > > I've always been wondering about this, but should the comment rather
>> be
>> > > aligned to the current code indent (one to the right) to which it
>> > > refers, as it was before, not the preprocessor macro?
>> > >
>> >
>> > This is what 'git cl format' produced.
>> >
>>
>> Yes, I've always used to revert this change back.
>>
>> > Do you care enough about this that we should diverge from clang-format?
>> >
>>
>> Personally I prefer alignment to the current indent level as well as the
>> context
>> (which is not the preprocessor macro), I'm not sure if git cl format is
>> right in
>> what it is doing, but I don't feel very strongly about this. Please feel
>> free to
>> follow your preference.
>>
>
> I think the indented version in this case could make sense, because the
> comment is about the c++ code, not the #ifdef, but if it was about the
> latter (e.g. "Disable on Win32 because reasons XYZ"), it would make sense
> to have the comment aligned with the #ifdef.
> clang-format can't understand which one it is, so it picks one (and
> aligning to the #ifdef is probably the most useful... oftentimes the
> comment about the c++ code would be inside the #ifdef, but if it's outside
> it more likely than not refers to the #ifdef itself).
>
> My suggestion is to either let clang-format do what it wants to do or to
> disable it on this comment by adding the "// clang-format off" - otherwise
> it's just make-work for anyone who touches this file.
>

I went with the former - I don't think it's worth fighting clang-format.

>
>
>>
>> https://chromiumcodereview.appspot.com/2178603002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-07-26 23:24:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-26 23:25:57]

Description was changed from

==========
Rename media::GpuVideoDecodeAcceleratorFactoryImpl to
media::GpuVideoDecodeAcceleratorFactory

BUG=597150
==========

to

==========
Rename media::GpuVideoDecodeAcceleratorFactoryImpl to
media::GpuVideoDecodeAcceleratorFactory

BUG=597150

Committed: https://crrev.com/b52f7da5b93799d6129ce2ad6594625c3261d032
Cr-Commit-Position: refs/heads/master@{#407963}
==========

[commit-bot: I haz the power, 2016-07-26 23:25:58]

Patchset 2 (id:??) landed as
https://crrev.com/b52f7da5b93799d6129ce2ad6594625c3261d032
Cr-Commit-Position: refs/heads/master@{#407963}