Use visible entry for website settings dialog.

Use visible entry for website settings dialog.

BUG=265221
TEST=Click page icon after clicking a link to a page with a 204 response.

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

[Charlie Reis, 2013-08-12 17:54:05]

This makes sure that the website settings dialog shows info for the same URL
that is showing in the omnibox.

Nasko: Can you review?

Ben: This bug appears to exist for all UI platforms.  Can you verify whether
I've found them all, or if I'm still missing some?  Thanks!

[nasko, 2013-08-12 18:19:51]

lgtm

Thanks for flagging occurrences to review in subsequent CLs.

[Charlie Reis, 2013-08-12 18:40:40]

Glen, maybe you can help confirm whether I've updated all affected platforms for
the website settings icon in the location bar?  Both ben@ and sky@ are OOO.

[Charlie Reis, 2013-08-12 23:02:57]

Darin, can you take a look for owners?  It's a small change to each UI platform,
so I thought that might be better than getting a different owner for each file.

[darin (slow to review), 2013-08-13 06:58:44]

LGTM

[darin (slow to review), 2013-08-13 06:58:44]

LGTM

[nasko, 2013-08-13 13:08:57]

Hey Charlie,
I've been digging into GetActiveEntry and I think this might not be a complete
fix. In chrome/browser/content_settings/tab_specific_content_settings.cc, the
TabSpecificContentSettings class tracks various types of information for the
active tab. It also uses GetActiveEntry and I've verified that in Canary it
shows the incorrect information.
It seems your fix will gate whether the dialog is shown, but it won't ensure we
collect the proper information. I haven't had a chance to test this yet, but
wanted to mention it here.

[Charlie Reis, 2013-08-13 17:40:04]

On 2013/08/13 13:08:57, nasko wrote:
> Hey Charlie,
> I've been digging into GetActiveEntry and I think this might not be a complete
> fix. In chrome/browser/content_settings/tab_specific_content_settings.cc, the
> TabSpecificContentSettings class tracks various types of information for the
> active tab. It also uses GetActiveEntry and I've verified that in Canary it
> shows the incorrect information.
> It seems your fix will gate whether the dialog is shown, but it won't ensure
we
> collect the proper information. I haven't had a chance to test this yet, but
> wanted to mention it here.

Thanks for catching that, Nasko.  Looks like it affects whether the renderer is
notified when a setting is changed.  Jochen, can you review
TabSpecificContentSettings?

[jochen (gone - plz use gerrit), 2013-08-13 18:22:19]

On 2013/08/13 17:40:04, creis wrote:
> On 2013/08/13 13:08:57, nasko wrote:
> > Hey Charlie,
> > I've been digging into GetActiveEntry and I think this might not be a
complete
> > fix. In chrome/browser/content_settings/tab_specific_content_settings.cc,
the
> > TabSpecificContentSettings class tracks various types of information for the
> > active tab. It also uses GetActiveEntry and I've verified that in Canary it
> > shows the incorrect information.
> > It seems your fix will gate whether the dialog is shown, but it won't ensure
> we
> > collect the proper information. I haven't had a chance to test this yet, but
> > wanted to mention it here.
> 
> Thanks for catching that, Nasko.  Looks like it affects whether the renderer
is
> notified when a setting is changed.  Jochen, can you review
> TabSpecificContentSettings?

Right, this is used to update the render view if the content settings affecting
it changed. What is the definition of GetVisibleEntry()? For this code, it
should be the last commited entry.

[jochen (gone - plz use gerrit), 2013-08-13 18:22:56]

On 2013/08/13 13:08:57, nasko wrote:
> Hey Charlie,
> I've been digging into GetActiveEntry and I think this might not be a complete
> fix. In chrome/browser/content_settings/tab_specific_content_settings.cc, the
> TabSpecificContentSettings class tracks various types of information for the
> active tab. It also uses GetActiveEntry and I've verified that in Canary it
> shows the incorrect information.
> It seems your fix will gate whether the dialog is shown, but it won't ensure
we
> collect the proper information. I haven't had a chance to test this yet, but
> wanted to mention it here.

What do you mean be incorrect information btw?

[Charlie Reis, 2013-08-13 18:30:28]

On 2013/08/13 18:22:19, jochen wrote:
> On 2013/08/13 17:40:04, creis wrote:
> > On 2013/08/13 13:08:57, nasko wrote:
> > > Hey Charlie,
> > > I've been digging into GetActiveEntry and I think this might not be a
> complete
> > > fix. In chrome/browser/content_settings/tab_specific_content_settings.cc,
> the
> > > TabSpecificContentSettings class tracks various types of information for
the
> > > active tab. It also uses GetActiveEntry and I've verified that in Canary
it
> > > shows the incorrect information.
> > > It seems your fix will gate whether the dialog is shown, but it won't
ensure
> > we
> > > collect the proper information. I haven't had a chance to test this yet,
but
> > > wanted to mention it here.
> > 
> > Thanks for catching that, Nasko.  Looks like it affects whether the renderer
> is
> > notified when a setting is changed.  Jochen, can you review
> > TabSpecificContentSettings?
> 
> Right, this is used to update the render view if the content settings
affecting
> it changed. What is the definition of GetVisibleEntry()? For this code, it
> should be the last commited entry.

GetVisibleEntry is what's showing in the address bar.  It avoids returning
things like renderer-initiated pending entries to prevent URL spoofs.

[Charlie Reis, 2013-08-13 18:31:50]

On 2013/08/13 18:22:56, jochen wrote:
> On 2013/08/13 13:08:57, nasko wrote:
> > Hey Charlie,
> > I've been digging into GetActiveEntry and I think this might not be a
complete
> > fix. In chrome/browser/content_settings/tab_specific_content_settings.cc,
the
> > TabSpecificContentSettings class tracks various types of information for the
> > active tab. It also uses GetActiveEntry and I've verified that in Canary it
> > shows the incorrect information.
> > It seems your fix will gate whether the dialog is shown, but it won't ensure
> we
> > collect the proper information. I haven't had a chance to test this yet, but
> > wanted to mention it here.
> 
> What do you mean be incorrect information btw?

I don't think Nasko was correct about "collecting the proper information."  The
change to TabSpecificContentSettings doesn't affect which URL is used to
collect/store the setting.  It just affects whether the renderer process for a
given tab is notified when a setting for a URL is changed.

[jochen (gone - plz use gerrit), 2013-08-13 19:03:52]

this code seems to be racy anyway. Also, it's not that big much of a problem if
the settings aren't updated, e.g. if you unblock javascript, you'll have to
reload the page anyway.

so lgtm

[commit-bot: I haz the power, 2013-08-13 22:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/22831005/17001

[commit-bot: I haz the power, 2013-08-14 05:12:36]

Change committed as 217485