Adds geolocaiton support to the location bar content image model and content ...

Adds geolocaiton support to the location bar content image model and content bubble model.
Most of these edits were lifted out of http://codereview.chromium.org/650180

TODO: add geolocation support to the views in the three UI platforms for the bubble model.
(NOTE this change results in poorly formed bubble contents for the geolocaiton bubble, this will be fixed up in the following CLs)


BUG=11246
TEST=open a site that uses geolocaiton, select allow/deny & click the icon.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42665

[Peter Kasting, 2010-03-25 19:25:09]

LGTM

http://codereview.chromium.org/1344002/diff/6001/7004
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/1344002/diff/6001/7004#newcode166
chrome/browser/content_setting_bubble_model.cc:166: "SetDomains currently only
support geolocation content type";
Nit: support -> supports

http://codereview.chromium.org/1344002/diff/6001/7004#newcode198
chrome/browser/content_setting_bubble_model.cc:198: // duplicating them here as
well as in the infobars.
I don't agree; I think we ought to show these.  From this class' viewpoint
adding these is easy; you simply have a third section and check for
CONTENT_SETTING_ASK.  I don't know what impact that has on the TabContents side,
but note that even if this isn't supported there, you can check in here without
having to simultaneously change that side.

http://codereview.chromium.org/1344002/diff/6001/7004#newcode201
chrome/browser/content_setting_bubble_model.cc:201:
set_clear_link(l10n_util::GetStringUTF8(IDS_GEOLOCATION_BUBBLE_CLEAR_LINK));
Nit: Don't set the "clear" link in a function called SetDomains()... I'd do it
in the constructor directly but you could also do it in its own function to
match how the TitleAndLink model does this.

http://codereview.chromium.org/1344002/diff/6001/7002
File chrome/browser/content_setting_image_model.cc (right):

http://codereview.chromium.org/1344002/diff/6001/7002#newcode62
chrome/browser/content_setting_image_model.cc:62: set_visible(false);
Nit: You might want to reverse the conditional and then make this clause
early-return, to reduce the amount of indenting... doesn't matter much either
way

http://codereview.chromium.org/1344002/diff/6001/7002#newcode82
chrome/browser/content_setting_image_model.cc:82: bool geolocation_active =
false;
Nit: You can avoid the temporary and the break as follows:

set_visible(true);
for (...) {
  if (it->second == CONTENT_SETTING_ALLOW) {
    set_icon(...);
    set_tooltip(...);
    return;
  }
}
set_icon(...);
set_tooltip(...);

...This avoids the ?: ops and hopefully makes things simpler.

http://codereview.chromium.org/1344002/diff/6001/7002#newcode107
chrome/browser/content_setting_image_model.cc:107:
CreateContentSettingImageModel(ContentSettingsType content_settings_type) {
Nit: Don't break after "::"; normally we do this like:

LongReturnType
    Class::Func(
    ArgType arg);

Sometimes people indent the last line an additional 4, it's not clear to me what
the style guide wants.

http://codereview.chromium.org/1344002/diff/6001/7002#newcode108
chrome/browser/content_setting_image_model.cc:108: if (content_settings_type ==
CONTENT_SETTINGS_TYPE_GEOLOCATION)
Nit: If you want, you could use ?: here.

[Peter Kasting, 2010-03-25 19:30:41]

Oh, another issue: This change assumes CONTENT_SETTINGS_TYPE_GEOLOCATION has
already been added, which the trybots claim isn't true.  I haven't dug through
enough email to know if that's landed already, but if it hasn't, it needs to,
and that change will probably be nontrivial as you'll need to enlarge every
array in the codebase in order to not fail DCHECKs.

[Peter Kasting, 2010-03-25 19:32:37]

Ah, I see you've indeed landed this.  OK, might want to gcl try -r <xxx> with a
revision past the landing point then...

[joth, 2010-03-25 20:17:52]

(+thakis FYI)
Thanks for the useful comments, think I addressed them all.
I'll try and commit this as soon as it's green, and churn out the windows patch
before I head home.

http://codereview.chromium.org/1344002/diff/6001/7004
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/1344002/diff/6001/7004#newcode166
chrome/browser/content_setting_bubble_model.cc:166: "SetDomains currently only
support geolocation content type";
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: support -> supports

Done.

http://codereview.chromium.org/1344002/diff/6001/7004#newcode198
chrome/browser/content_setting_bubble_model.cc:198: // duplicating them here as
well as in the infobars.
On 2010/03/25 19:25:10, Peter Kasting wrote:
> I don't agree; I think we ought to show these.  From this class' viewpoint
> adding these is easy; you simply have a third section and check for
> CONTENT_SETTING_ASK.  I don't know what impact that has on the TabContents
side,
> but note that even if this isn't supported there, you can check in here
without
> having to simultaneously change that side.

The discussion on this one is in the Wave with Glen.
The problem is, this is either showing duplicated info (it's in an info bar and
again in a bubble) or it's showing info the user can't do anything about from
this bubble (clear will only increase the set requesting permission, not
decrease)

very good point that we can code this side and worry about if/when to put it
into the TabContents map in another change, so I'll do that.

http://codereview.chromium.org/1344002/diff/6001/7004#newcode201
chrome/browser/content_setting_bubble_model.cc:201:
set_clear_link(l10n_util::GetStringUTF8(IDS_GEOLOCATION_BUBBLE_CLEAR_LINK));
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: Don't set the "clear" link in a function called SetDomains()... I'd do it
> in the constructor directly but you could also do it in its own function to
> match how the TitleAndLink model does this.

Done.

http://codereview.chromium.org/1344002/diff/6001/7002
File chrome/browser/content_setting_image_model.cc (right):

http://codereview.chromium.org/1344002/diff/6001/7002#newcode62
chrome/browser/content_setting_image_model.cc:62: set_visible(false);
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: You might want to reverse the conditional and then make this clause
> early-return, to reduce the amount of indenting... doesn't matter much either
> way

Done.

http://codereview.chromium.org/1344002/diff/6001/7002#newcode82
chrome/browser/content_setting_image_model.cc:82: bool geolocation_active =
false;
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: You can avoid the temporary and the break as follows:
> 
> set_visible(true);
> for (...) {
>   if (it->second == CONTENT_SETTING_ALLOW) {
>     set_icon(...);
>     set_tooltip(...);
>     return;
>   }
> }
> set_icon(...);
> set_tooltip(...);
> 
> ...This avoids the ?: ops and hopefully makes things simpler.

Done.

http://codereview.chromium.org/1344002/diff/6001/7002#newcode107
chrome/browser/content_setting_image_model.cc:107:
CreateContentSettingImageModel(ContentSettingsType content_settings_type) {
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: Don't break after "::"; normally we do this like:
> 
> LongReturnType
>     Class::Func(
>     ArgType arg);
> 
> Sometimes people indent the last line an additional 4, it's not clear to me
what
> the style guide wants.

Done.

http://codereview.chromium.org/1344002/diff/6001/7002#newcode108
chrome/browser/content_setting_image_model.cc:108: if (content_settings_type ==
CONTENT_SETTINGS_TYPE_GEOLOCATION)
On 2010/03/25 19:25:10, Peter Kasting wrote:
> Nit: If you want, you could use ?: here.

Seems clearer in my mind this way (mostly for consistency with other factory
functions that have >2 cases, such as the bubble model)

[Peter Kasting, 2010-03-25 20:21:34]

http://codereview.chromium.org/1344002/diff/6001/7004
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/1344002/diff/6001/7004#newcode198
chrome/browser/content_setting_bubble_model.cc:198: // duplicating them here as
well as in the infobars.
On 2010/03/25 20:17:52, joth wrote:
> The problem is, this is either showing duplicated info (it's in an info bar
and
> again in a bubble) or it's showing info the user can't do anything about from
> this bubble (clear will only increase the set requesting permission, not
> decrease)

The duplicated info doesn't bother me, but your other point does, and I forgot
that I was concerned about putting the link above the third section.  Plus your
other argument on Wave was good.  I change my mind.  Let's rip this out and
remove the string.

[joth, 2010-03-25 20:37:27]

On 2010/03/25 20:21:34, Peter Kasting wrote:
> http://codereview.chromium.org/1344002/diff/6001/7004
> File chrome/browser/content_setting_bubble_model.cc (right):
> 
> http://codereview.chromium.org/1344002/diff/6001/7004#newcode198
> chrome/browser/content_setting_bubble_model.cc:198: // duplicating them here
as
> well as in the infobars.
> On 2010/03/25 20:17:52, joth wrote:
> > The problem is, this is either showing duplicated info (it's in an info bar
> and
> > again in a bubble) or it's showing info the user can't do anything about
from
> > this bubble (clear will only increase the set requesting permission, not
> > decrease)
> 
> The duplicated info doesn't bother me, but your other point does, and I forgot
> that I was concerned about putting the link above the third section.  Plus
your
> other argument on Wave was good.  I change my mind.  Let's rip this out and
> remove the string.

So whilst you were reading that, I just uploaded a more general and much neater
version of SetDomains() which I'm quite happy with the shape of.
Anyway it's just one line to remove this. I'll do that now  (and the string),
then build &  commit.....

[Peter Kasting, 2010-03-25 20:40:02]

LGTM