RtcVideo{Encoder,Decoder}{Factory,} minor cleanup: consts, range based loops, comments.

RtcVideo{Encoder,Decoder}{Factory,} minor cleanup: consts, range based loops, comments.

BUG=440843
Committed: https://crrev.com/7b615a63765d52238ce061080036d33566372adc
Cr-Commit-Position: refs/heads/master@{#321366}

[mcasas, 2015-03-18 23:44:40]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-03-19 00:45:41]

mcasas@chromium.org changed reviewers:
+ emircan@chromium.org

[mcasas, 2015-03-19 00:45:42]

emircan@ PTAL

[emircan, 2015-03-19 01:16:56]

lgtm % nit

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:47: const
std::vector<media::VideoEncodeAccelerator::SupportedProfile> profiles =
Consider assigning by reference to the rvalue to save from a copy:
const std::vector<media::VideoEncodeAccelerator::SupportedProfile>& profiles

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:59: return new
RTCVideoEncoder(type, gpu_factories_);
Consider using std::find_if for clarity.

[Pawel Osciak, 2015-03-19 01:26:33]

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

[Pawel Osciak, 2015-03-19 01:26:34]

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.h (left):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.h:21: 
Namespace formatting rule examples at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespace_For...
show that empty lines should be there.

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:61: return NULL;
s/NULL/nullptr/

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.h (left):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.h:16: 
Ditto.

[Pawel Osciak, 2015-03-19 01:52:34]

lgtm with the above nits

[mcasas, 2015-03-19 02:54:26]

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.h (left):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.h:21: 
On 2015/03/19 01:26:33, Pawel Osciak wrote:
> Namespace formatting rule examples at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespace_For...
> show that empty lines should be there.

Usually these namespace+forward declarations don't get
such treatment. Strange that is not mentioned in the 
G C++ StyleGuide nor in the Chromium one.

See for instance [1-4].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/dbus/bus.h&sq=pack...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell.h&sq=pac...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:47: const
std::vector<media::VideoEncodeAccelerator::SupportedProfile> profiles =
On 2015/03/19 01:16:55, emircan wrote:
> Consider assigning by reference to the rvalue to save from a copy:
> const std::vector<media::VideoEncodeAccelerator::SupportedProfile>& profiles

Done.

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:59: return new
RTCVideoEncoder(type, gpu_factories_);
On 2015/03/19 01:16:56, emircan wrote:
> Consider using std::find_if for clarity. 

Hmm in this case and since VideoCodec does not have an
appropriate operator==, it turns out ugly(er):

std::vector<VideoCodec>::const_iterator it = std::find_if(
    codecs_.begin(), codecs_.end(),
    [type](const VideoCodec& codec) { return codec.type == type; });
if (it != codecs_.end())
  new RTCVideoEncoder(type, gpu_factories_);

So I'd rather leave it as it's now.

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:61: return NULL;
On 2015/03/19 01:26:33, Pawel Osciak wrote:
> s/NULL/nullptr/

Done.

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.h (left):

https://codereview.chromium.org/1019033003/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.h:16: 
On 2015/03/19 01:26:33, Pawel Osciak wrote:
> Ditto.

Acknowledged.

[mcasas, 2015-03-19 02:55:34]

mcasas@chromium.org changed reviewers:
+ perkj@chromium.org, tommi@chromium.org

[mcasas, 2015-03-19 02:55:35]

tommi@/perkj@ OWNERS RS please

[perkj_chrome, 2015-03-19 08:38:21]

lgtm

[mcasas, 2015-03-19 14:58:10]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-03-19 14:58:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1019033003/#ps40001
(title: "emircan@ and posciak@ comments")

[commit-bot: I haz the power, 2015-03-19 14:58:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019033003/40001

[commit-bot: I haz the power, 2015-03-19 16:16:24]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-03-19 16:17:28]

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