Build fix in protected_media_identifier_permission_context.cc

Build fix in protected_media_identifier_permission_context.cc

Build failed due to uninitialied values.
../../chrome/browser/media/protected_media_identifier_permission_context.cc:187:8: error: 'persist' may be used uninitialized in this function [-Werror=maybe-uninitialized]
../../chrome/browser/media/protected_media_identifier_permission_context.cc:205:32: error: 'content_setting' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Committed: https://crrev.com/1cf71d24bd3ca8531067ab239f290f4fc0d5f7c6
Cr-Commit-Position: refs/heads/master@{#319088}

[dshwang, 2015-03-04 08:53:22]

dongseong.hwang@intel.com changed reviewers:
+ miguelg@chromium.org, mlamouri@chromium.org

[dshwang, 2015-03-04 08:53:23]

https://codereview.chromium.org/955383003 breaks chrome os build. 
Could you review?

[dshwang, 2015-03-04 08:59:39]

dongseong.hwang@intel.com changed reviewers:
+ thakis@chromium.org
- miguelg@chromium.org

[dshwang, 2015-03-04 08:59:40]

Nico, could you review?

[mlamouri (slow - plz ping), 2015-03-04 10:27:10]

I guess lgtm if that fixes the build for you. However, I assume this is not from
a builder. Which compiler is that?

[dshwang, 2015-03-04 11:19:07]

On 2015/03/04 10:27:10, Mounir Lamouri wrote:
> I guess lgtm if that fixes the build for you. However, I assume this is not
from
> a builder. Which compiler is that?

It fixed my build. I use gcc. chromium-built-in clang might not complain as
builders are fine.

[Nico, 2015-03-04 18:03:23]

I think the idea was that folks who uses gcc use -GYP_DEFINES=werror=, and then
this shouldn't be needed. (This warning seems to have a high false-positive
rate, so we wouldn't enable a warning like this.)

But if this is the only instance of this warning, I suppose this change lgtm

[mlamouri (slow - plz ping), 2015-03-04 18:05:16]

On 2015/03/04 at 18:03:23, thakis wrote:
> I think the idea was that folks who uses gcc use -GYP_DEFINES=werror=, and
then this shouldn't be needed. (This warning seems to have a high false-positive
rate, so we wouldn't enable a warning like this.)
> 
> But if this is the only instance of this warning, I suppose this change lgtm

Note that it's the only instance because every time one is introduced, someone
fixes it. If we need to have a policy to allow that kind of code, we should do
it regardless of the fact that our code base is not full of those.

[dshwang, 2015-03-04 18:08:55]

On 2015/03/04 18:05:16, Mounir Lamouri wrote:
> On 2015/03/04 at 18:03:23, thakis wrote:
> > I think the idea was that folks who uses gcc use -GYP_DEFINES=werror=, and
> then this shouldn't be needed. (This warning seems to have a high
false-positive
> rate, so we wouldn't enable a warning like this.)
> > 
> > But if this is the only instance of this warning, I suppose this change lgtm

Thx for reviewing.

> Note that it's the only instance because every time one is introduced, someone
> fixes it. If we need to have a policy to allow that kind of code, we should do
> it regardless of the fact that our code base is not full of those.

IMO, we should consider warning as error. If we allow some warnings, everybody
will be confused which warning is allowed?
It begin to break your windows.

[dshwang, 2015-03-04 18:08:59]

The CQ bit was checked by dongseong.hwang@intel.com

[Nico, 2015-03-04 18:10:04]

On Wed, Mar 4, 2015 at 10:05 AM, <mlamouri@chromium.org> wrote:

> On 2015/03/04 at 18:03:23, thakis wrote:
>
>> I think the idea was that folks who uses gcc use -GYP_DEFINES=werror=, and
>>
> then this shouldn't be needed. (This warning seems to have a high
> false-positive
> rate, so we wouldn't enable a warning like this.)
>
>  But if this is the only instance of this warning, I suppose this change
>> lgtm
>>
>
> Note that it's the only instance because every time one is introduced,
> someone
> fixes it. If we need to have a policy to allow that kind of code, we
> should do
> it regardless of the fact that our code base is not full of those.
>

Eh, I think the current setup is fine. I don't want to have an explicit
policy for allowing changes like this (since I don't think having warnings
with a high false-positive rate on by default is good, and that's the
philosophy we use for in-tree-enabled warnings), but in practice these
fixes aren't actively bad either I think. So if folks want to best-effort
fix them, that's fine with me.

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, 2015-03-04 18:10:29]

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

[Nico, 2015-03-04 18:11:27]

On Wed, Mar 4, 2015 at 10:08 AM, <dongseong.hwang@intel.com> wrote:

> On 2015/03/04 18:05:16, Mounir Lamouri wrote:
>
>> On 2015/03/04 at 18:03:23, thakis wrote:
>> > I think the idea was that folks who uses gcc use -GYP_DEFINES=werror=,
>> and
>> then this shouldn't be needed. (This warning seems to have a high
>>
> false-positive
>
>> rate, so we wouldn't enable a warning like this.)
>> >
>> > But if this is the only instance of this warning, I suppose this change
>> lgtm
>>
>
> Thx for reviewing.
>
>  Note that it's the only instance because every time one is introduced,
>> someone
>> fixes it. If we need to have a policy to allow that kind of code, we
>> should do
>> it regardless of the fact that our code base is not full of those.
>>
>
> IMO, we should consider warning as error. If we allow some warnings,
> everybody
> will be confused which warning is allowed?
> It begin to break your windows.
>

We do use warnings as errors for all "in-tree compilers" (compilers used on
the main waterfall), with a curated list of enabled warnings. If you don't
use one of these compilers, your life is needlessly difficult with
warnings-as-errors imho.

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, 2015-03-04 18:34:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-04 18:35:10]

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