fixed the DCHECK and also corrected the website setting UI for media

It hits the DCHECK when changing the permission for some types like media.

Also, media does not support "allow" in website setting, when the site permission for media is "allow", the global default should be selected as default, instead of block.


BUG=166396
TEST= go to apprtc.appspot.com, and verify the website setting.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173453

[no longer working on chromium, 2012-12-13 16:35:07]

Hi,
dubroy, I am removing the DCHECK you added in Issue 11417082, could you please
review this patch? 
Shess, owner stamp.

Thanks,
SX

[no longer working on chromium, 2012-12-13 16:37:39]

Hi, I got a ping from rsesek on the issue that I am trying to fix here, so it
might be better to let  rsesek review this patch. But you guys are still
welcomed to give comment.

rsesek, please take a look at the fix.

Thanks,
SX

[Scott Hess - ex-Googler, 2012-12-13 20:56:39]

lgtm owners-wise.  didn't review for whether it addresses the bug or anything.

[Robert Sesek, 2012-12-13 21:33:24]

Markus wrote most of this file. Please check the SCM log to find the best
reviewers.

[Patrick Dubroy, 2012-12-13 21:38:48]

On 2012/12/13 21:33:24, rsesek wrote:
> Markus wrote most of this file. Please check the SCM log to find the best
> reviewers.

I wrote most of this file.

As the comment described, the DCHECK is actually verifying an important
assumption. It can't just be removed. Do you have a repro case for that?

I will discuss the other change with Markus tomorrow.

[Robert Sesek, 2012-12-13 21:47:23]

On 2012/12/13 21:38:48, dubroy wrote:
> On 2012/12/13 21:33:24, rsesek wrote:
> > Markus wrote most of this file. Please check the SCM log to find the best
> > reviewers.
> 
> I wrote most of this file.

:$ I should follow my own advice

[no longer working on chromium, 2012-12-13 22:09:51]

On 2012/12/13 21:38:48, dubroy wrote:
> On 2012/12/13 21:33:24, rsesek wrote:
> > Markus wrote most of this file. Please check the SCM log to find the best
> > reviewers.
> 
> I wrote most of this file.
> 
> As the comment described, the DCHECK is actually verifying an important
> assumption. It can't just be removed. Do you have a repro case for that?
> 

You can easily reproduce the problem in debug mode by editing the media
permission for page like apprtc.appspot.com.
And I believe it also hits the dcheck for other type of permission like full
screen, or mouse lock.

> I will discuss the other change with Markus tomorrow.

sgtm.

[markusheintz_, 2012-12-14 10:49:04]

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (left):

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:1126:
DCHECK_EQ(0U, [[cookiesView_ subviews] count]);
Patrick Dubroy will fix the DCHECK issue and upload a separate CL.

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:896: if
(permissionInfo.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM &&
Sorry I think I miss some detail. Why do we need this?

Line 877-881 already makes sure that we exclude the allow option.

If the default settings is used, then the |.setting| should be
CONTENT_SETTING_DEFAULT. The permission info has a separate field for the
effective default setting.
If the |.setting| is not default, then we don't use the default setting.

Creating proper permission info objects should be done in
chrome/browser/ui/website_settings/website_settings.cc.

[markusheintz_, 2012-12-14 15:10:03]

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11571010/diff/2001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:896: if
(permissionInfo.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM &&
On 2012/12/14 10:49:04, markusheintz_ wrote:
> Sorry I think I miss some detail. Why do we need this?
> 
> Line 877-881 already makes sure that we exclude the allow option.
> 
> If the default settings is used, then the |.setting| should be
> CONTENT_SETTING_DEFAULT. The permission info has a separate field for the
> effective default setting.
> If the |.setting| is not default, then we don't use the default setting.
> 
> Creating proper permission info objects should be done in
> chrome/browser/ui/website_settings/website_settings.cc.
> 

Alrigth after discussing this offline via chat I'm ok with this beeing a
temporary change. Please add a comment and a TODO. If we don't have a bug for
this pls create one and cc me. Thanks a lot! :)

[markusheintz_, 2012-12-14 15:46:43]

Thanks for going the extra miles/km's

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:875: bool
secure_scheme = webContents_->GetURL().SchemeIsSecure();
nit: I think we could inline this.

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:897: if
(permissionInfo.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM &&
Can  this condition ever be true while the condition in line 877 is false?

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/views/we...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/views/we...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:163: GURL
site_url_;
I think you don't use this attribute since you get the URL from the web_contents
in website_settings_popup_view.cc:494

[no longer working on chromium, 2012-12-14 15:56:19]

Markus, please take another look.

Thanks

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:875: bool
secure_scheme = webContents_->GetURL().SchemeIsSecure();
On 2012/12/14 15:46:43, markusheintz_ wrote:
> nit: I think we could inline this.

this secure_scheme is used in two places, and it is actually better to use a
temp variable to store the bool.

Let me know if you strongly want removing the temp variable, otherwise, I hope
to keep it this way.

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:897: if
(permissionInfo.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM &&
On 2012/12/14 15:46:43, markusheintz_ wrote:
> Can  this condition ever be true while the condition in line 877 is false?
> 

line 877 decides if it will add a "allow" button to the UI.
And here will decide CONTENT_SETTING_DEFAULT item will be checked if the
permission for the http site is "allow".

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/views/we...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.h
(right):

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/views/we...
chrome/browser/ui/views/website_settings/website_settings_popup_view.h:163: GURL
site_url_;
On 2012/12/14 15:46:43, markusheintz_ wrote:
> I think you don't use this attribute since you get the URL from the
web_contents
> in website_settings_popup_view.cc:494

Thanks, uncleaned code.

[markusheintz_, 2012-12-14 16:12:09]

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:875: bool
secure_scheme = webContents_->GetURL().SchemeIsSecure();
On 2012/12/14 15:56:19, xians1 wrote:
> On 2012/12/14 15:46:43, markusheintz_ wrote:
> > nit: I think we could inline this.
> 
> this secure_scheme is used in two places, and it is actually better to use a
> temp variable to store the bool.
> 
> Let me know if you strongly want removing the temp variable, otherwise, I hope
> to keep it this way.

If we keep using it in two places then ignore this comment. If not that pls
inline.

https://codereview.chromium.org/11571010/diff/7003/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:897: if
(permissionInfo.type == CONTENT_SETTINGS_TYPE_MEDIASTREAM &&
On 2012/12/14 15:56:19, xians1 wrote:
> On 2012/12/14 15:46:43, markusheintz_ wrote:
> > Can  this condition ever be true while the condition in line 877 is false?
> > 
> 
> line 877 decides if it will add a "allow" button to the UI.
> And here will decide CONTENT_SETTING_DEFAULT item will be checked if the
> permission for the http site is "allow".

But the permission should never be ALLOW for an http site. It should only be
possible that the permission is CONTENT_SETTING_ALLOW for secure URLs.

The setting is either CONTENT_SETTING_DEFAULT or CONTENT_SETTING_ASK or
CONTENT_SETTING_BLOCK.

Users won't be able to create an allow content setting exception (permission)
for a non https URL using the infobar. The settings page itself also does only
allow users to rm exceptions.

[no longer working on chromium, 2012-12-17 11:19:34]

Hi Markus, I know you are OOO, but if you have time, please take another look.

Thanks,
SX

[markusheintz_, 2012-12-17 11:25:02]

On 2012/12/17 11:19:34, xians1 wrote:
> Hi Markus, I know you are OOO, but if you have time, please take another look.
> 
> Thanks,
> SX

LGTM

[commit-bot: I haz the power, 2012-12-17 11:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/11571010/18001

[commit-bot: I haz the power, 2012-12-17 12:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/11571010/18001