Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
650180:
Initial Geolocation location bar icons. (Closed)
|
Created:
10 years, 10 months ago by bulach Modified:
9 years, 7 months ago Reviewers:
CC:
chromium-reviews_googlegroups.com, brettw+cc_chromium.org, darin+cc_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org, jam, jam+cc_chromium.org Visibility:
Public. |
DescriptionInitial Geolocation location bar icons.
(see design doc and UI mocks:
https://docs.google.com/View?id=dfbnm49n_0dpc7pxpx
)
Moves GeolocationPermissionContext up to profile.
Adds Geolocation icons and bubbles to location bar.
TEST=chrome/browser/geolocation/geolocation_browsertest.cc
BUG=37206
Patch Set 1 : Adds support for multiple frames. #
Total comments: 67
Patch Set 2 : Addresses joth's comments #
Total comments: 18
Patch Set 3 : More comments #
Total comments: 25
Patch Set 4 : More comments #Patch Set 5 : Jorlow's comments #Patch Set 6 : Uses ContentSettingImageView. #
Total comments: 26
Patch Set 7 : Previous comments #Patch Set 8 : Merge with other changes. #
Total comments: 74
Patch Set 9 : Addresses Peter and Brett's comments. #Messages
Total messages: 30 (0 generated)
Overall looks good to me, some suggestions below. Needs some feedback from someone more familiar with profiles & host content settings. http://codereview.chromium.org/650180/diff/2053/1008 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/650180/diff/2053/1008#newcode6525 chrome/app/generated_resources.grd:6525: <message name="IDS_GEOLOCATION_ALLOWED_TOOLTIP" desc="Tooltip text when a page is not allowed to use geolocation."> when it *is* allowed http://codereview.chromium.org/650180/diff/2053/1008#newcode6532 chrome/app/generated_resources.grd:6532: This site, <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph>, wants to track your current location using wifi and GPS. I'd drop "using wifi and GPS". - the site may not have requested use_high_accuracy (GPS) - we may not be using either these (cell ID, ethernet GW address....) http://codereview.chromium.org/650180/diff/2053/1012 File chrome/browser/geolocation/geolocation_dispatcher_host.cc (right): http://codereview.chromium.org/650180/diff/2053/1012#newcode54 chrome/browser/geolocation/geolocation_dispatcher_host.cc:54: if (!ChromeThread::CurrentlyOn(ChromeThread::UI)) { could you update this to IO whilst here? http://codereview.chromium.org/650180/diff/2053/1012#newcode62 chrome/browser/geolocation/geolocation_dispatcher_host.cc:62: DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); ditto http://codereview.chromium.org/650180/diff/2053/1012#newcode69 chrome/browser/geolocation/geolocation_dispatcher_host.cc:69: if (geoposition.error_code == Geoposition::ERROR_CODE_NONE) { I think it would be better as if (geoposition.IsValidFix()) { } else { DCHECK(geoposition.IsInitialized()); } http://codereview.chromium.org/650180/diff/2053/1013 File chrome/browser/geolocation/geolocation_dispatcher_host.h (right): http://codereview.chromium.org/650180/diff/2053/1013#newcode72 chrome/browser/geolocation/geolocation_dispatcher_host.h:72: render_view_id < rhs.render_view_id; this only works if process_id == rhs.process_id is never true. Otherwise: if (process_id == rhs.process_id) return render_view_id < rhs.render_view_id; return process_id < rhs.process_id http://codereview.chromium.org/650180/diff/2053/1014 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/2053/1014#newcode22 chrome/browser/geolocation/geolocation_permission_context.cc:22: #include "chrome/common/json_value_serializer.h" still needed? http://codereview.chromium.org/650180/diff/2053/1014#newcode89 chrome/browser/geolocation/geolocation_permission_context.cc:89: : host_content_settings_map_(host_content_settings_map) { DCHECK(host_con_set_map); http://codereview.chromium.org/650180/diff/2053/1014#newcode112 chrome/browser/geolocation/geolocation_permission_context.cc:112: DCHECK("unknown content setting") << content_setting; UNREACHED() << "unknown content setting " << content_setting; http://codereview.chromium.org/650180/diff/2053/1014#newcode128 chrome/browser/geolocation/geolocation_permission_context.cc:128: SavePermissionOnUIThread(origin, allowed); surely both these 2 methods will only be called from the UI thread anyway? maybe DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); and save on the extra check & redirect inside that method? http://codereview.chromium.org/650180/diff/2053/1014#newcode133 chrome/browser/geolocation/geolocation_permission_context.cc:133: // of geolocation. so they have already received their position updated or error callbacks. http://codereview.chromium.org/650180/diff/2053/1015 File chrome/browser/geolocation/geolocation_permission_context.h (right): http://codereview.chromium.org/650180/diff/2053/1015#newcode66 chrome/browser/geolocation/geolocation_permission_context.h:66: void SavePermissionOnUIThread(const GURL& origin, bool allowed); Ah interesting. I've been using FooOnBarThread to mean it must *only* be called from bar thread (as this is more interesting to the caller) I'd call this SavePermission[FromAnyThread] No idea if there's a wider convention though. http://codereview.chromium.org/650180/diff/2053/1018 File chrome/browser/profile.h (right): http://codereview.chromium.org/650180/diff/2053/1018#newcode298 chrome/browser/profile.h:298: virtual GeolocationPermissionContext* GetGeolocationPermissionContext() = 0; would be good to loop in someone else to give this thumbs up (but looks fine to me....) http://codereview.chromium.org/650180/diff/2053/1019 File chrome/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/650180/diff/2053/1019#newcode13 chrome/browser/renderer_host/render_view_host_delegate.h:13: #include "chrome/common/content_settings.h" is this needed? http://codereview.chromium.org/650180/diff/2053/1023 File chrome/browser/views/geolocation_bubble_contents.cc (right): http://codereview.chromium.org/650180/diff/2053/1023#newcode59 chrome/browser/views/geolocation_bubble_contents.cc:59: for (std::vector<RadioButtons>::const_iterator i = // loop through all the origins in the bubble, updating the permission setting for each (as a small optimization we might just set those that have changed) http://codereview.chromium.org/650180/diff/2053/1023#newcode77 chrome/browser/views/geolocation_bubble_contents.cc:77: if (tab_contents_) { maybe a comment indicating when these 2 different scenarios may occur http://codereview.chromium.org/650180/diff/2053/1023#newcode92 chrome/browser/views/geolocation_bubble_contents.cc:92: const NotificationDetails& details) { just to check, I guess we don't have warnings about unused arguments? http://codereview.chromium.org/650180/diff/2053/1023#newcode101 chrome/browser/views/geolocation_bubble_contents.cc:101: GridLayout* layout = new views::GridLayout(this); no need for vies:: http://codereview.chromium.org/650180/diff/2053/1023#newcode111 chrome/browser/views/geolocation_bubble_contents.cc:111: i != settings_.end(); ++i) { this means the origins will be listed in alpha order. Guess that's not a problem, maybe worth commenting it. http://codereview.chromium.org/650180/diff/2053/1023#newcode114 chrome/browser/views/geolocation_bubble_contents.cc:114: l10n_util::GetStringF( can this line join up onto the previous one? http://codereview.chromium.org/650180/diff/2053/1023#newcode116 chrome/browser/views/geolocation_bubble_contents.cc:116: UTF8ToWide(i->first.host()))); it's not clear if the GURL have already been stripped down to just contain the host portion? if not, one host may appear many times in the map (with different origin pages on the same host) ISTM it would be clearest that where-ever it is we stem page urls down down to origin host, we should switch from GURL to string. http://codereview.chromium.org/650180/diff/2053/1023#newcode124 chrome/browser/views/geolocation_bubble_contents.cc:124: radios.allow_radio = new views::RadioButton( maybe clarify here or on the struct comments who owns this button? http://codereview.chromium.org/650180/diff/2053/1024 File chrome/browser/views/geolocation_bubble_contents.h (right): http://codereview.chromium.org/650180/diff/2053/1024#newcode51 chrome/browser/views/geolocation_bubble_contents.h:51: typedef std::map<views::Link*, TabContents*> PopupLinks; looks like this typedef is only used once, maybe not needed? (as other places in this file inc public api you use std::map directly) http://codereview.chromium.org/650180/diff/2053/1025 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/2053/1025#newcode789 chrome/browser/views/location_bar_view.cc:789: geolocation_image_view_.SetContentSetting(geolocation_settings); blech: double deep copy of map<> not sure what else to suggest other than have TabContents::GetGeolocationSettings return a const std::map& http://codereview.chromium.org/650180/diff/2053/1025#newcode1456 chrome/browser/views/location_bar_view.cc:1456: SkBitmap* LocationBarView::GeolocationImageView::warning_icon_ = NULL; depending on your point of view, either icon could be a warning icon. perhaps allowed_icon_ and denied_icon_ to make them a clear pair? http://codereview.chromium.org/650180/diff/2053/1025#newcode1468 chrome/browser/views/location_bar_view.cc:1468: ResourceBundle& rb = ResourceBundle::GetSharedInstance(); DCHECK(!blocked_icon_); http://codereview.chromium.org/650180/diff/2053/1025#newcode1480 chrome/browser/views/location_bar_view.cc:1480: const std::map<GURL, ContentSetting>& settings) { ah, we don't actually take a copy of the map in here. really should take a pair of iterators but that would be a mess http://codereview.chromium.org/650180/diff/2053/1025#newcode1488 chrome/browser/views/location_bar_view.cc:1488: UTF8ToWide(i->first.host())); this will result in the tooltip just mentioning the (lexicographically) first host, rather than all. http://codereview.chromium.org/650180/diff/2053/1025#newcode1494 chrome/browser/views/location_bar_view.cc:1494: UTF8ToWide(i->first.host())); conversely, this will just list the last blocked site. maybe we need 4 strings, shown in preference order: 1/ Site foo.com is able track your location 2/ Multiple sites are tracking your location 3/ Site foo.com is blocked from tracking your location. 4/ Multiple sites are blocked from tracking your location. if (i->second == CONTENT_SETTING_ALLOW) { if (icon == warning_icon_) { // use #2 break; } else { // use #1 } } else if (i->second == CONTENT_SETTING_BLOCK) { if (icon == warning_icon_) { continue; } else if (icon == blocked_icon_) { // use #4 } else { // use #3 } } else { NOTREACHED() << "bad content setting " << i->second; } http://codereview.chromium.org/650180/diff/2053/1025#newcode1518 chrome/browser/views/location_bar_view.cc:1518: return true; do this first in the method, before screen coord conversions? http://codereview.chromium.org/650180/diff/2053/1025#newcode1523 chrome/browser/views/location_bar_view.cc:1523: NULL, NULL); we don't seem to do anything with display_host should we be doing this to each of the URLs inside the bubble? http://codereview.chromium.org/650180/diff/2053/1026 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/2053/1026#newcode371 chrome/browser/views/location_bar_view.h:371: CONTENT_BLOCKED_NUM_TYPES = CONTENT_SETTINGS_TYPE_POPUPS + 1, magic plus 1 ? http://codereview.chromium.org/650180/diff/2053/1026#newcode414 chrome/browser/views/location_bar_view.h:414: // GeolocationImageView is used to display the Geolocation icon when this guy kind of sticks out in here as being domain specific code rather than generic code. I guess this is again because geolocaiton is the only thing that needs both allow & deny icons? maybe we can capture this in the comment // (Currently geolocation is the only feature that needs both an enabled and a blocked icon) http://codereview.chromium.org/650180/diff/2053/1030 File chrome/renderer/geolocation_dispatcher.cc (right): http://codereview.chromium.org/650180/diff/2053/1030#newcode43 chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); ah ha! we stem URL to host origin here. In that case I think we should pass it as std::string from this point onward. Alternatively, if we want to keep the full URL over IPC, do the GetOrigin() call once it arrives in the browser.
thanks joth! all comments addressed, and I had to include "GeolocationFilterPageView", which is the tab for managing the settings: again, it's a bit different from ContentFilterPageView, as we need three states (ask, allow, block).. another look please? if you're fine with it overall, I'll ping jorlow and someone else to look at the hostcontentsettingsmap usage. http://codereview.chromium.org/650180/diff/2053/1008 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/650180/diff/2053/1008#newcode6525 chrome/app/generated_resources.grd:6525: <message name="IDS_GEOLOCATION_ALLOWED_TOOLTIP" desc="Tooltip text when a page is not allowed to use geolocation."> On 2010/02/24 13:09:14, jothchrome wrote: > when it *is* allowed Done. http://codereview.chromium.org/650180/diff/2053/1008#newcode6532 chrome/app/generated_resources.grd:6532: This site, <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph>, wants to track your current location using wifi and GPS. On 2010/02/24 13:09:14, jothchrome wrote: > I'd drop "using wifi and GPS". > - the site may not have requested use_high_accuracy (GPS) > - we may not be using either these (cell ID, ethernet GW address....) > > done (this was in the UI mocks in the design doc) http://codereview.chromium.org/650180/diff/2053/1012 File chrome/browser/geolocation/geolocation_dispatcher_host.cc (right): http://codereview.chromium.org/650180/diff/2053/1012#newcode54 chrome/browser/geolocation/geolocation_dispatcher_host.cc:54: if (!ChromeThread::CurrentlyOn(ChromeThread::UI)) { On 2010/02/24 13:09:14, jothchrome wrote: > could you update this to IO whilst here? Done. http://codereview.chromium.org/650180/diff/2053/1012#newcode62 chrome/browser/geolocation/geolocation_dispatcher_host.cc:62: DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); On 2010/02/24 13:09:14, jothchrome wrote: > ditto Done. http://codereview.chromium.org/650180/diff/2053/1012#newcode69 chrome/browser/geolocation/geolocation_dispatcher_host.cc:69: if (geoposition.error_code == Geoposition::ERROR_CODE_NONE) { On 2010/02/24 13:09:14, jothchrome wrote: > I think it would be better as > if (geoposition.IsValidFix()) { > } else { > DCHECK(geoposition.IsInitialized()); > } > > Done. http://codereview.chromium.org/650180/diff/2053/1013 File chrome/browser/geolocation/geolocation_dispatcher_host.h (right): http://codereview.chromium.org/650180/diff/2053/1013#newcode72 chrome/browser/geolocation/geolocation_dispatcher_host.h:72: render_view_id < rhs.render_view_id; On 2010/02/24 13:09:14, jothchrome wrote: > this only works if process_id == rhs.process_id is never true. > Otherwise: > > if (process_id == rhs.process_id) > return render_view_id < rhs.render_view_id; > return process_id < rhs.process_id > Done. http://codereview.chromium.org/650180/diff/2053/1014 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/2053/1014#newcode22 chrome/browser/geolocation/geolocation_permission_context.cc:22: #include "chrome/common/json_value_serializer.h" On 2010/02/24 13:09:14, jothchrome wrote: > still needed? nice catch! removed. http://codereview.chromium.org/650180/diff/2053/1014#newcode89 chrome/browser/geolocation/geolocation_permission_context.cc:89: : host_content_settings_map_(host_content_settings_map) { On 2010/02/24 13:09:14, jothchrome wrote: > DCHECK(host_con_set_map); > Done. http://codereview.chromium.org/650180/diff/2053/1014#newcode112 chrome/browser/geolocation/geolocation_permission_context.cc:112: DCHECK("unknown content setting") << content_setting; On 2010/02/24 13:09:14, jothchrome wrote: > UNREACHED() << "unknown content setting " << content_setting; Done. http://codereview.chromium.org/650180/diff/2053/1014#newcode128 chrome/browser/geolocation/geolocation_permission_context.cc:128: SavePermissionOnUIThread(origin, allowed); On 2010/02/24 13:09:14, jothchrome wrote: > surely both these 2 methods will only be called from the UI thread anyway? maybe > DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); and save on the extra check > & redirect inside that method? done (was a left-over from going over to FILE thread, no longer needed). http://codereview.chromium.org/650180/diff/2053/1014#newcode133 chrome/browser/geolocation/geolocation_permission_context.cc:133: // of geolocation. On 2010/02/24 13:09:14, jothchrome wrote: > so they have already received their position updated or error callbacks. Done. http://codereview.chromium.org/650180/diff/2053/1015 File chrome/browser/geolocation/geolocation_permission_context.h (right): http://codereview.chromium.org/650180/diff/2053/1015#newcode66 chrome/browser/geolocation/geolocation_permission_context.h:66: void SavePermissionOnUIThread(const GURL& origin, bool allowed); On 2010/02/24 13:09:14, jothchrome wrote: > Ah interesting. I've been using FooOnBarThread to mean it must *only* be called > from bar thread (as this is more interesting to the caller) > I'd call this SavePermission[FromAnyThread] > No idea if there's a wider convention though. good point, fixed the comment. http://codereview.chromium.org/650180/diff/2053/1018 File chrome/browser/profile.h (right): http://codereview.chromium.org/650180/diff/2053/1018#newcode298 chrome/browser/profile.h:298: virtual GeolocationPermissionContext* GetGeolocationPermissionContext() = 0; On 2010/02/24 13:09:14, jothchrome wrote: > would be good to loop in someone else to give this thumbs up (but looks fine to > me....) sure! once you're happy with this, I'll loop in jorlow and someone else to look at it as well.. http://codereview.chromium.org/650180/diff/2053/1019 File chrome/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/650180/diff/2053/1019#newcode13 chrome/browser/renderer_host/render_view_host_delegate.h:13: #include "chrome/common/content_settings.h" On 2010/02/24 13:09:14, jothchrome wrote: > is this needed? removed. http://codereview.chromium.org/650180/diff/2053/1023 File chrome/browser/views/geolocation_bubble_contents.cc (right): http://codereview.chromium.org/650180/diff/2053/1023#newcode59 chrome/browser/views/geolocation_bubble_contents.cc:59: for (std::vector<RadioButtons>::const_iterator i = On 2010/02/24 13:09:14, jothchrome wrote: > // loop through all the origins in the bubble, updating the permission setting > for each (as a small optimization we might just set those that have changed) done (with the optimization bit) http://codereview.chromium.org/650180/diff/2053/1023#newcode92 chrome/browser/views/geolocation_bubble_contents.cc:92: const NotificationDetails& details) { On 2010/02/24 13:09:14, jothchrome wrote: > just to check, I guess we don't have warnings about unused arguments? same as http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/chrome/browser/views/con... http://codereview.chromium.org/650180/diff/2053/1023#newcode101 chrome/browser/views/geolocation_bubble_contents.cc:101: GridLayout* layout = new views::GridLayout(this); On 2010/02/24 13:09:14, jothchrome wrote: > no need for vies:: done (but had to expand the using above..) http://codereview.chromium.org/650180/diff/2053/1023#newcode111 chrome/browser/views/geolocation_bubble_contents.cc:111: i != settings_.end(); ++i) { On 2010/02/24 13:09:14, jothchrome wrote: > this means the origins will be listed in alpha order. > Guess that's not a problem, maybe worth commenting it. Done. http://codereview.chromium.org/650180/diff/2053/1023#newcode114 chrome/browser/views/geolocation_bubble_contents.cc:114: l10n_util::GetStringF( On 2010/02/24 13:09:14, jothchrome wrote: > can this line join up onto the previous one? Done. http://codereview.chromium.org/650180/diff/2053/1023#newcode116 chrome/browser/views/geolocation_bubble_contents.cc:116: UTF8ToWide(i->first.host()))); On 2010/02/24 13:09:14, jothchrome wrote: > it's not clear if the GURL have already been stripped down to just contain the > host portion? if not, one host may appear many times in the map (with different > origin pages on the same host) > > ISTM it would be clearest that where-ever it is we stem page urls down down to > origin host, we should switch from GURL to string. > very good point. using string instead. http://codereview.chromium.org/650180/diff/2053/1023#newcode124 chrome/browser/views/geolocation_bubble_contents.cc:124: radios.allow_radio = new views::RadioButton( On 2010/02/24 13:09:14, jothchrome wrote: > maybe clarify here or on the struct comments who owns this button? done (although we have more than enough smart ptrs, i.e., a naked ptrs are never owned..) http://codereview.chromium.org/650180/diff/2053/1024 File chrome/browser/views/geolocation_bubble_contents.h (right): http://codereview.chromium.org/650180/diff/2053/1024#newcode51 chrome/browser/views/geolocation_bubble_contents.h:51: typedef std::map<views::Link*, TabContents*> PopupLinks; On 2010/02/24 13:09:14, jothchrome wrote: > looks like this typedef is only used once, maybe not needed? (as other places in > this file inc public api you use std::map directly) removed both above, unnecessary. http://codereview.chromium.org/650180/diff/2053/1025 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/2053/1025#newcode789 chrome/browser/views/location_bar_view.cc:789: geolocation_image_view_.SetContentSetting(geolocation_settings); On 2010/02/24 13:09:14, jothchrome wrote: > blech: double deep copy of map<> not sure what else to suggest > other than have TabContents::GetGeolocationSettings return a const std::map& > avoiding this double copy by passing tab_contents here so that geolocation_image_view_ can get the map directly.. naming suggestions for the method are welcome! http://codereview.chromium.org/650180/diff/2053/1025#newcode1456 chrome/browser/views/location_bar_view.cc:1456: SkBitmap* LocationBarView::GeolocationImageView::warning_icon_ = NULL; On 2010/02/24 13:09:14, jothchrome wrote: > depending on your point of view, either icon could be a warning icon. > perhaps allowed_icon_ and denied_icon_ to make them a clear pair? Done. http://codereview.chromium.org/650180/diff/2053/1025#newcode1468 chrome/browser/views/location_bar_view.cc:1468: ResourceBundle& rb = ResourceBundle::GetSharedInstance(); On 2010/02/24 13:09:14, jothchrome wrote: > DCHECK(!blocked_icon_); Done. http://codereview.chromium.org/650180/diff/2053/1025#newcode1480 chrome/browser/views/location_bar_view.cc:1480: const std::map<GURL, ContentSetting>& settings) { On 2010/02/24 13:09:14, jothchrome wrote: > ah, we don't actually take a copy of the map in here. really should take a pair > of iterators but that would be a mess > as above, passing tab_contents and fetching the map here. http://codereview.chromium.org/650180/diff/2053/1025#newcode1488 chrome/browser/views/location_bar_view.cc:1488: UTF8ToWide(i->first.host())); On 2010/02/24 13:09:14, jothchrome wrote: > this will result in the tooltip just mentioning the (lexicographically) first > host, rather than all. > Done. http://codereview.chromium.org/650180/diff/2053/1025#newcode1494 chrome/browser/views/location_bar_view.cc:1494: UTF8ToWide(i->first.host())); On 2010/02/24 13:09:14, jothchrome wrote: > conversely, this will just list the last blocked site. > > maybe we need 4 strings, shown in preference order: > > 1/ Site http://foo.com is able track your location > 2/ Multiple sites are tracking your location > 3/ Site http://foo.com is blocked from tracking your location. > 4/ Multiple sites are blocked from tracking your location. > > > if (i->second == CONTENT_SETTING_ALLOW) { > if (icon == warning_icon_) { > // use #2 > break; > } else { > // use #1 > } > } else if (i->second == CONTENT_SETTING_BLOCK) { > if (icon == warning_icon_) { > continue; > } else if (icon == blocked_icon_) { > // use #4 > } else { > // use #3 > } > } else { > NOTREACHED() << "bad content setting " << i->second; > } > quite liked this. added new strings and using this flow instead, thanks! http://codereview.chromium.org/650180/diff/2053/1025#newcode1518 chrome/browser/views/location_bar_view.cc:1518: return true; On 2010/02/24 13:09:14, jothchrome wrote: > do this first in the method, before screen coord conversions? Done. http://codereview.chromium.org/650180/diff/2053/1025#newcode1523 chrome/browser/views/location_bar_view.cc:1523: NULL, NULL); On 2010/02/24 13:09:14, jothchrome wrote: > we don't seem to do anything with display_host > should we be doing this to each of the URLs inside the bubble? leftover, removed. http://codereview.chromium.org/650180/diff/2053/1026 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/2053/1026#newcode371 chrome/browser/views/location_bar_view.h:371: CONTENT_BLOCKED_NUM_TYPES = CONTENT_SETTINGS_TYPE_POPUPS + 1, On 2010/02/24 13:09:14, jothchrome wrote: > magic plus 1 ? added a comment, let me know if it's any clearer.. http://codereview.chromium.org/650180/diff/2053/1026#newcode414 chrome/browser/views/location_bar_view.h:414: // GeolocationImageView is used to display the Geolocation icon when On 2010/02/24 13:09:14, jothchrome wrote: > this guy kind of sticks out in here as being domain specific code rather than > generic code. I guess this is again because geolocaiton is the only thing that > needs both allow & deny icons? maybe we can capture this in the comment > // (Currently geolocation is the only feature that needs both an enabled and a > blocked icon) > done. http://codereview.chromium.org/650180/diff/2053/1030 File chrome/renderer/geolocation_dispatcher.cc (right): http://codereview.chromium.org/650180/diff/2053/1030#newcode43 chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); On 2010/02/24 13:09:14, jothchrome wrote: > ah ha! we stem URL to host origin here. In that case I think we should pass it > as std::string from this point onward. > Alternatively, if we want to keep the full URL over IPC, do the GetOrigin() call > once it arrives in the browser. Done.
Nice! Thanks http://codereview.chromium.org/650180/diff/4001/4020 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/4001/4020#newcode1489 chrome/browser/views/location_bar_view.cc:1489: // 4. Multiple sites are blocked from tracking your location. Thinking about it, we need to swap 1 with 2, and 3 with 4 for this to make logical sense http://codereview.chromium.org/650180/diff/4001/4020#newcode1494 chrome/browser/views/location_bar_view.cc:1494: if (icon == allowed_icon_) { using new numbering scheme... if () { // case #1 http://codereview.chromium.org/650180/diff/4001/4020#newcode1497 chrome/browser/views/location_bar_view.cc:1497: } else { { // case #2 http://codereview.chromium.org/650180/diff/4001/4020#newcode1503 chrome/browser/views/location_bar_view.cc:1503: if (icon == allowed_icon_) { { // We're already in case 1 or 2 http://codereview.chromium.org/650180/diff/4001/4020#newcode1505 chrome/browser/views/location_bar_view.cc:1505: } else if (icon == blocked_icon_) { // case # 3 http://codereview.chromium.org/650180/diff/4001/4020#newcode1508 chrome/browser/views/location_bar_view.cc:1508: } else { {// case #4 http://codereview.chromium.org/650180/diff/4001/4020#newcode1542 chrome/browser/views/location_bar_view.cc:1542: geolocation_settings, profile_, tab_contents); again a double map copy. I should just quit fretting this. http://codereview.chromium.org/650180/diff/4001/4024 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/4001/4024#newcode42 chrome/browser/views/options/geolocation_filter_page_view.cc:42: layout->SetInsets(5, 5, 5, 5); i guess the magic numbers are standard practice http://codereview.chromium.org/650180/diff/4001/4025 File chrome/browser/views/options/geolocation_filter_page_view.h (right): http://codereview.chromium.org/650180/diff/4001/4025#newcode45 chrome/browser/views/options/geolocation_filter_page_view.h:45: DISALLOW_COPY_AND_ASSIGN(GeolocationFilterPageView); could be disallow implicit
+jorlow Thanks joth! all comments addressed. jorlow: would you mind taking a quick look at this change? I haven't fully finalized yet (still need to make sure it builds on linux / mac), but I'd appreciate if you could take a general look on it: basically, this change now uses HostContentSettingsMap to persist the geolocation permission, and adds the icon on the location bar. I tried to be less invasive as possible and haven't changed much of existing code (rather, added new classes). as a follow up, we could perhaps converge some of them, if it makes sense. thanks! marcus http://codereview.chromium.org/650180/diff/4001/4020 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/4001/4020#newcode1489 chrome/browser/views/location_bar_view.cc:1489: // 4. Multiple sites are blocked from tracking your location. On 2010/02/24 19:41:23, jothchrome wrote: > Thinking about it, we need to swap 1 with 2, and 3 with 4 for this to make > logical sense Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1494 chrome/browser/views/location_bar_view.cc:1494: if (icon == allowed_icon_) { On 2010/02/24 19:41:23, jothchrome wrote: > using new numbering scheme... > if () { // case #1 Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1497 chrome/browser/views/location_bar_view.cc:1497: } else { On 2010/02/24 19:41:23, jothchrome wrote: > { // case #2 Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1503 chrome/browser/views/location_bar_view.cc:1503: if (icon == allowed_icon_) { On 2010/02/24 19:41:23, jothchrome wrote: > { // We're already in case 1 or 2 Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1505 chrome/browser/views/location_bar_view.cc:1505: } else if (icon == blocked_icon_) { On 2010/02/24 19:41:23, jothchrome wrote: > // case # 3 Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1508 chrome/browser/views/location_bar_view.cc:1508: } else { On 2010/02/24 19:41:23, jothchrome wrote: > {// case #4 Done. http://codereview.chromium.org/650180/diff/4001/4020#newcode1542 chrome/browser/views/location_bar_view.cc:1542: geolocation_settings, profile_, tab_contents); On 2010/02/24 19:41:23, jothchrome wrote: > again a double map copy. I should just quit fretting this. good spot. removed, since GeolocationBubbleContents already knows about tab_contents. http://codereview.chromium.org/650180/diff/4001/4024 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/4001/4024#newcode42 chrome/browser/views/options/geolocation_filter_page_view.cc:42: layout->SetInsets(5, 5, 5, 5); On 2010/02/24 19:41:23, jothchrome wrote: > i guess the magic numbers are standard practice yep, came from ContentFilterPageView.. http://codereview.chromium.org/650180/diff/4001/4025 File chrome/browser/views/options/geolocation_filter_page_view.h (right): http://codereview.chromium.org/650180/diff/4001/4025#newcode45 chrome/browser/views/options/geolocation_filter_page_view.h:45: DISALLOW_COPY_AND_ASSIGN(GeolocationFilterPageView); On 2010/02/24 19:41:23, jothchrome wrote: > could be disallow implicit Done.
Some comments. Still need to look at the meat tho. This CL is really too big to be managable. Stuff like adding the strings + icons and changing names can easily be pulled into other reviews. I'd even be tempted to pull stuff like the content settings tab, the blocked content changes, and maybe even some other chunks into their own changes. But I guess it's fine for now. Peter, please take a look at the conent settings related changes to this (which is about half of it). http://codereview.chromium.org/650180/diff/6004/6009 File chrome/browser/geolocation/geolocation_browsertest.cc (right): http://codereview.chromium.org/650180/diff/6004/6009#newcode14 chrome/browser/geolocation/geolocation_browsertest.cc:14: #include "chrome/common/content_settings_types.h" Alpha order http://codereview.chromium.org/650180/diff/6004/6010 File chrome/browser/geolocation/geolocation_dispatcher_host.cc (right): http://codereview.chromium.org/650180/diff/6004/6010#newcode71 chrome/browser/geolocation/geolocation_dispatcher_host.cc:71: geo->render_view_id, geoposition); When possible, please do these types of changes in their own CL. http://codereview.chromium.org/650180/diff/6004/6012 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/6004/6012#newcode34 chrome/browser/geolocation/geolocation_permission_context.cc:34: const std::string& host) I would have just left it a GURL...I guess it's ok tho. http://codereview.chromium.org/650180/diff/6004/6019 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/650180/diff/6004/6019#newcode2043 chrome/browser/tab_contents/tab_contents.cc:2043: // TODO(bulach): split OnBlockedContentChange from OnGeolocationContentChange. I don't get this todo. http://codereview.chromium.org/650180/diff/6004/6020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/6004/6020#newcode245 chrome/browser/tab_contents/tab_contents.h:245: void GetGeolocationSettings( Why not return a const &? http://codereview.chromium.org/650180/diff/6004/6020#newcode1218 chrome/browser/tab_contents/tab_contents.h:1218: std::map<std::string, ContentSetting> geolocation_settings_; geolocation_content_settings_ probably would be more clear. http://codereview.chromium.org/650180/diff/6004/6023 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/6004/6023#newcode786 chrome/browser/views/location_bar_view.cc:786: if (tab_contents) { no {}'s http://codereview.chromium.org/650180/diff/6004/6024 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/6004/6024#newcode373 chrome/browser/views/location_bar_view.h:373: CONTENT_BLOCKED_NUM_TYPES = CONTENT_SETTINGS_TYPE_POPUPS + 1, Hmmmmmm....not sure I like this. At very least, this should be near the CONTENT_SETTINGS_TYPE enum itself, not here in case others are added. It seems like we could do something even cleaner though. http://codereview.chromium.org/650180/diff/6004/6024#newcode419 chrome/browser/views/location_bar_view.h:419: // the only feature that needs both an enabled and blocked icon). It's more than just enabled....it means "in use" http://codereview.chromium.org/650180/diff/6004/6025 File chrome/browser/views/options/content_settings_window_view.cc (right): http://codereview.chromium.org/650180/diff/6004/6025#newcode15 chrome/browser/views/options/content_settings_window_view.cc:15: #include "chrome/browser/views/options/geolocation_filter_page_view.h" alpha http://codereview.chromium.org/650180/diff/6004/6034 File net/socket/tcp_client_socket_win.cc (right): http://codereview.chromium.org/650180/diff/6004/6034#newcode5 net/socket/tcp_client_socket_win.cc:5: #include <ws2tcpip.h> Does this absolutely have to be at the top of the file? If not, net/socket/tcp_client_socket_win.h" should come first. And other wise, I'm not sure what to do...but we probably can't leave it like this.
thanks Jeremy! apologies for the biggie, I hope next CLs will be more manageable.. another look please? I think specially with regards to the contentsettings versus contentblockedsettings issues that arise with introducing Geolocation.. please, see my comments and questions inline: http://codereview.chromium.org/650180/diff/6004/6009 File chrome/browser/geolocation/geolocation_browsertest.cc (right): http://codereview.chromium.org/650180/diff/6004/6009#newcode14 chrome/browser/geolocation/geolocation_browsertest.cc:14: #include "chrome/common/content_settings_types.h" On 2010/02/24 22:26:31, Jeremy Orlow wrote: > Alpha order Done. http://codereview.chromium.org/650180/diff/6004/6010 File chrome/browser/geolocation/geolocation_dispatcher_host.cc (right): http://codereview.chromium.org/650180/diff/6004/6010#newcode71 chrome/browser/geolocation/geolocation_dispatcher_host.cc:71: geo->render_view_id, geoposition); On 2010/02/24 22:26:31, Jeremy Orlow wrote: > When possible, please do these types of changes in their own CL. yep, sorry.. there was a bug with the id's that I only noticed in this CL, so I ended up fixing here.. next time I'll fix it separately.. http://codereview.chromium.org/650180/diff/6004/6012 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/6004/6012#newcode34 chrome/browser/geolocation/geolocation_permission_context.cc:34: const std::string& host) On 2010/02/24 22:26:31, Jeremy Orlow wrote: > I would have just left it a GURL...I guess it's ok tho. joth's point was that since we're only using host, seems to make more sense to have it as string to avoid wrong / inconsistent conversions.. http://codereview.chromium.org/650180/diff/6004/6019 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/650180/diff/6004/6019#newcode2043 chrome/browser/tab_contents/tab_contents.cc:2043: // TODO(bulach): split OnBlockedContentChange from OnGeolocationContentChange. On 2010/02/24 22:26:31, Jeremy Orlow wrote: > I don't get this todo. sorry, this was more of a question: should we have TabContentsDelegate::OnGeolocationContentChange instead of TabContentsDelegate::OnBlockedContentChange? http://codereview.chromium.org/650180/diff/6004/6020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/6004/6020#newcode245 chrome/browser/tab_contents/tab_contents.h:245: void GetGeolocationSettings( On 2010/02/24 22:26:31, Jeremy Orlow wrote: > Why not return a const &? done (question: is it common to return const&? in my previous project we avoided that to make the ownership a bit clearer..) http://codereview.chromium.org/650180/diff/6004/6020#newcode1218 chrome/browser/tab_contents/tab_contents.h:1218: std::map<std::string, ContentSetting> geolocation_settings_; On 2010/02/24 22:26:31, Jeremy Orlow wrote: > geolocation_content_settings_ probably would be more clear. Done. http://codereview.chromium.org/650180/diff/6004/6023 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/6004/6023#newcode786 chrome/browser/views/location_bar_view.cc:786: if (tab_contents) { On 2010/02/24 22:26:31, Jeremy Orlow wrote: > no {}'s Done. http://codereview.chromium.org/650180/diff/6004/6024 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/6004/6024#newcode373 chrome/browser/views/location_bar_view.h:373: CONTENT_BLOCKED_NUM_TYPES = CONTENT_SETTINGS_TYPE_POPUPS + 1, On 2010/02/24 22:26:31, Jeremy Orlow wrote: > Hmmmmmm....not sure I like this. At very least, this should be near the > CONTENT_SETTINGS_TYPE enum itself, not here in case others are added. It seems > like we could do something even cleaner though. agree, this is ugly... not sure how best to solve it: by introducing geolocation as CONTENT_SETTINGS, there's now a missing abstraction / mapping from CONTENT_SETTINGS to CONTENT_BLOCKED.. I moved this enum here to content_settings_types.h, but perhaps I could bite the bullet and actually introduce a new enum CONTENT_BLOCKED? http://codereview.chromium.org/650180/diff/6004/6024#newcode419 chrome/browser/views/location_bar_view.h:419: // the only feature that needs both an enabled and blocked icon). On 2010/02/24 22:26:31, Jeremy Orlow wrote: > It's more than just enabled....it means "in use" Done. http://codereview.chromium.org/650180/diff/6004/6025 File chrome/browser/views/options/content_settings_window_view.cc (right): http://codereview.chromium.org/650180/diff/6004/6025#newcode15 chrome/browser/views/options/content_settings_window_view.cc:15: #include "chrome/browser/views/options/geolocation_filter_page_view.h" On 2010/02/24 22:26:31, Jeremy Orlow wrote: > alpha Done. http://codereview.chromium.org/650180/diff/6004/6034 File net/socket/tcp_client_socket_win.cc (right): http://codereview.chromium.org/650180/diff/6004/6034#newcode5 net/socket/tcp_client_socket_win.cc:5: #include <ws2tcpip.h> On 2010/02/24 22:26:31, Jeremy Orlow wrote: > Does this absolutely have to be at the top of the file? If not, > net/socket/tcp_client_socket_win.h" should come first. > > And other wise, I'm not sure what to do...but we probably can't leave it like > this. no idea why this was here.. reverted this file.
http://codereview.chromium.org/650180/diff/6004/6019 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/650180/diff/6004/6019#newcode2043 chrome/browser/tab_contents/tab_contents.cc:2043: // TODO(bulach): split OnBlockedContentChange from OnGeolocationContentChange. On 2010/02/24 23:49:10, bulach wrote: > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > I don't get this todo. > > sorry, this was more of a question: > should we have TabContentsDelegate::OnGeolocationContentChange instead of > TabContentsDelegate::OnBlockedContentChange? > For what purpose? Can you outline the full proposal of what you're thinking? http://codereview.chromium.org/650180/diff/6004/6020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/6004/6020#newcode245 chrome/browser/tab_contents/tab_contents.h:245: void GetGeolocationSettings( On 2010/02/24 23:49:10, bulach wrote: > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > Why not return a const &? > > done (question: is it common to return const&? in my previous project we avoided > that to make the ownership a bit clearer..) We almost always have getters return const& or pointers. A comment on the method will tell you the lifetime of the pointer and/or tell you it's ref counted (and thus you can take a ref to control the lifetime). const& is used in the rest of the cases. Lifetime is generally short...so if you want to keep it around immediately assign it to a variable. It's nice though because you can access const memebers without needing to do the copy. And it's easier to use. But yeah...if you look around, we return const &'s a lot...and then let the caller do the copy. http://codereview.chromium.org/650180/diff/6004/6024 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/6004/6024#newcode373 chrome/browser/views/location_bar_view.h:373: CONTENT_BLOCKED_NUM_TYPES = CONTENT_SETTINGS_TYPE_POPUPS + 1, On 2010/02/24 23:49:10, bulach wrote: > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > Hmmmmmm....not sure I like this. At very least, this should be near the > > CONTENT_SETTINGS_TYPE enum itself, not here in case others are added. It > seems > > like we could do something even cleaner though. > > agree, this is ugly... not sure how best to solve it: by introducing geolocation > as CONTENT_SETTINGS, there's now a missing abstraction / mapping from > CONTENT_SETTINGS to CONTENT_BLOCKED.. I moved this enum here to > content_settings_types.h, but perhaps I could bite the bullet and actually > introduce a new enum CONTENT_BLOCKED? I think you should change this all to ContentSettingSet rather than ContentBlocked. And have 2 arrays of icons: the blocked ones and the allowed ones. And have the allowed ones contain some sort of "no icon" setting (0? -1?) that says don't display anything when this happens. Add a comment somewhere saying that this function doesn't necessarily get "allowed" signals for all of the APIs, so if you want to add an icon, you'll probably need to do that as well. And then get rid of all this special casing between geolocation and the normal blocked stuff.
thanks Jeremy! converged things as you suggested, once you're happy with it I'll replicate in GTK and Mac. On 2010/02/25 09:14:21, Jeremy Orlow wrote: > http://codereview.chromium.org/650180/diff/6004/6019 > File chrome/browser/tab_contents/tab_contents.cc (right): > > http://codereview.chromium.org/650180/diff/6004/6019#newcode2043 > chrome/browser/tab_contents/tab_contents.cc:2043: // TODO(bulach): split > OnBlockedContentChange from OnGeolocationContentChange. > On 2010/02/24 23:49:10, bulach wrote: > > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > > I don't get this todo. > > > > sorry, this was more of a question: > > should we have TabContentsDelegate::OnGeolocationContentChange instead of > > TabContentsDelegate::OnBlockedContentChange? > > > > For what purpose? Can you outline the full proposal of what you're thinking? now that I replaced BlockedContent with ContentSettings, this has become a simpler rename (which I'll do as a follow up). > > http://codereview.chromium.org/650180/diff/6004/6020 > File chrome/browser/tab_contents/tab_contents.h (right): > > http://codereview.chromium.org/650180/diff/6004/6020#newcode245 > chrome/browser/tab_contents/tab_contents.h:245: void GetGeolocationSettings( > On 2010/02/24 23:49:10, bulach wrote: > > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > > Why not return a const &? > > > > done (question: is it common to return const&? in my previous project we > avoided > > that to make the ownership a bit clearer..) > > We almost always have getters return const& or pointers. A comment on the > method will tell you the lifetime of the pointer and/or tell you it's ref > counted (and thus you can take a ref to control the lifetime). > > const& is used in the rest of the cases. Lifetime is generally short...so if > you want to keep it around immediately assign it to a variable. It's nice > though because you can access const memebers without needing to do the copy. > And it's easier to use. > > But yeah...if you look around, we return const &'s a lot...and then let the > caller do the copy. got it. thanks for the clarification! > > http://codereview.chromium.org/650180/diff/6004/6024 > File chrome/browser/views/location_bar_view.h (right): > > http://codereview.chromium.org/650180/diff/6004/6024#newcode373 > chrome/browser/views/location_bar_view.h:373: CONTENT_BLOCKED_NUM_TYPES = > CONTENT_SETTINGS_TYPE_POPUPS + 1, > On 2010/02/24 23:49:10, bulach wrote: > > On 2010/02/24 22:26:31, Jeremy Orlow wrote: > > > Hmmmmmm....not sure I like this. At very least, this should be near the > > > CONTENT_SETTINGS_TYPE enum itself, not here in case others are added. It > > seems > > > like we could do something even cleaner though. > > > > agree, this is ugly... not sure how best to solve it: by introducing > geolocation > > as CONTENT_SETTINGS, there's now a missing abstraction / mapping from > > CONTENT_SETTINGS to CONTENT_BLOCKED.. I moved this enum here to > > content_settings_types.h, but perhaps I could bite the bullet and actually > > introduce a new enum CONTENT_BLOCKED? > > I think you should change this all to ContentSettingSet rather than > ContentBlocked. And have 2 arrays of icons: the blocked ones and the allowed > ones. And have the allowed ones contain some sort of "no icon" setting (0? > -1?) that says don't display anything when this happens. Add a comment > somewhere saying that this function doesn't necessarily get "allowed" signals > for all of the APIs, so if you want to add an icon, you'll probably need to do > that as well. And then get rid of all this special casing between geolocation > and the normal blocked stuff. yeah, this is a more invasive change but in the end it seems clearer.. let me know your thoughts on how I implemented it. note: I kept GeolocationBubbleContents as a sub class of ContentBlockedBubbleContents (which already have a bunch of special-cases depending on the content type). it seems that in the future we can further split these contents into sub-classes, and rename the base class to ContentSettingsBubbleContents.
I don't have the patience to do as thorough of a review of this as I'd like....I think it's close to good enough tho. Peter, can you take a look? http://codereview.chromium.org/650180/diff/6071/7075 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/6071/7075#newcode118 chrome/browser/geolocation/geolocation_permission_context.cc:118: SavePermissionOnUIThread(host, allowed); OnUIThread isn't needed http://codereview.chromium.org/650180/diff/6071/7075#newcode187 chrome/browser/geolocation/geolocation_permission_context.cc:187: DCHECK(tab_contents); not needed...you'll segfault on the next op anyway not sure having this in its own function is worth it. http://codereview.chromium.org/650180/diff/6071/7075#newcode193 chrome/browser/geolocation/geolocation_permission_context.cc:193: DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); You can do this from any thread. I'm actually not even sure if its worth having a special method for it since it's just one setting. http://codereview.chromium.org/650180/diff/6071/7089 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 chrome/browser/views/location_bar_view.cc:1402: 0, Add a comment http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { Hmm...add a comment maybe http://codereview.chromium.org/650180/diff/6071/7090 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/6071/7090#newcode662 chrome/browser/views/location_bar_view.h:662: //GeolocationImageView geolocation_image_view_; delete http://codereview.chromium.org/650180/diff/6071/7091 File chrome/browser/views/options/content_settings_window_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7091#newcode177 chrome/browser/views/options/content_settings_window_view.cc:177: geolocation_page, false); line up on ( http://codereview.chromium.org/650180/diff/6071/7093 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7093#newcode102 chrome/browser/views/options/geolocation_filter_page_view.cc:102: const views::Event& event) { line up or nothing after ( http://codereview.chromium.org/650180/diff/6071/7093#newcode117 chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting = CONTENT_SETTING_ASK; Just don't set in this case. http://codereview.chromium.org/650180/diff/6071/7098 File chrome/renderer/geolocation_dispatcher.cc (right): http://codereview.chromium.org/650180/diff/6071/7098#newcode43 chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); don't you mean host()? all the content settings are per host, not origin
One quick comment.... :) http://codereview.chromium.org/650180/diff/6071/7089 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 chrome/browser/views/location_bar_view.cc:1361: if (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { maybe clearer to have an explicit static icons_initialized = false; if (!icons_initialized) { icons_initialized = true; ... http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != CONTENT_SETTINGS_TYPE_GEOLOCATION) { this special case defeats the purpose of the generalization of blocked_icons_ and allowed_icons_ ignoring the tooltip for a moment, the logic here should be const SkBitmap* icon = NULL; const bool content_blocked = tab_contents->IsContentBlocked(content_type_); if (content_blocked) { icon = blocked_icons_[content_type_]; } else { icon = allowed_icons_[content_type_]; } SetImage(icon); SetVisible(!!icon); // Now special case for the geolocation tooltip if (type == GEOLOCATION) { const std::map& settings = tab_contents->geolocaiton_setings(); if (settings.size() == 1) { // Special case for a single site, as this is the common case. tooltip = l10n_util::GetStringF(content_blocked ? IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, UTF8ToWide(settings.begin()->first)); } else { tooltip = l10n_util::GetString(content_blocked ? IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); } SetTooltip(tooltip); http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 chrome/browser/views/location_bar_view.cc:1424: const std::map<std::string, ContentSetting> settings = make this a const& too
Hi, First of all, many thanks for putting up with such a huge change: unfortunately introducing a new content setting proved more complex than we were expecting.. I'll try on Monday to split this in at least three changes: 1. adding strings / resources 2. the few files with those straightforward renaming 3. then the chunky rest with UI bits for all platforms although it won't make (3) significantly smaller, it should help your review. meanwhile, all comments addressed so far, and replicated the change in GTK. http://codereview.chromium.org/650180/diff/6071/7075 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/6071/7075#newcode118 chrome/browser/geolocation/geolocation_permission_context.cc:118: SavePermissionOnUIThread(host, allowed); On 2010/02/25 16:54:41, Jeremy Orlow wrote: > OnUIThread isn't needed Done. http://codereview.chromium.org/650180/diff/6071/7075#newcode187 chrome/browser/geolocation/geolocation_permission_context.cc:187: DCHECK(tab_contents); On 2010/02/25 16:54:41, Jeremy Orlow wrote: > not needed...you'll segfault on the next op anyway > > not sure having this in its own function is worth it. inlined in the two calling sites. http://codereview.chromium.org/650180/diff/6071/7075#newcode193 chrome/browser/geolocation/geolocation_permission_context.cc:193: DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); On 2010/02/25 16:54:41, Jeremy Orlow wrote: > You can do this from any thread. I'm actually not even sure if its worth having > a special method for it since it's just one setting. inlined http://codereview.chromium.org/650180/diff/6071/7089 File chrome/browser/views/location_bar_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 chrome/browser/views/location_bar_view.cc:1361: if (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { On 2010/02/26 15:29:55, jothchrome wrote: > maybe clearer to have an explicit > static icons_initialized = false; > if (!icons_initialized) { > icons_initialized = true; > ... > Done. http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 chrome/browser/views/location_bar_view.cc:1402: 0, On 2010/02/25 16:54:41, Jeremy Orlow wrote: > Add a comment Done. http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != CONTENT_SETTINGS_TYPE_GEOLOCATION) { On 2010/02/26 15:29:55, jothchrome wrote: > this special case defeats the purpose of the generalization of blocked_icons_ > and allowed_icons_ > ignoring the tooltip for a moment, the logic here should be > > const SkBitmap* icon = NULL; > const bool content_blocked = tab_contents->IsContentBlocked(content_type_); > if (content_blocked) { > icon = blocked_icons_[content_type_]; > } else { > icon = allowed_icons_[content_type_]; > } > > SetImage(icon); > SetVisible(!!icon); > > > // Now special case for the geolocation tooltip > if (type == GEOLOCATION) { > const std::map& settings = tab_contents->geolocaiton_setings(); > > if (settings.size() == 1) { > // Special case for a single site, as this is the common case. > tooltip = l10n_util::GetStringF(content_blocked ? > IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, > UTF8ToWide(settings.begin()->first)); > } else { > tooltip = l10n_util::GetString(content_blocked ? > IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : > IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); > } > > SetTooltip(tooltip); > > as we chatted, using "IsContentBlocked" but still with a special case for Geolocation. let me know if it's any clearer. http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 chrome/browser/views/location_bar_view.cc:1424: const std::map<std::string, ContentSetting> settings = On 2010/02/26 15:29:55, jothchrome wrote: > make this a const& too Done. http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { On 2010/02/25 16:54:41, Jeremy Orlow wrote: > Hmm...add a comment maybe done (let me know if it's any clear) http://codereview.chromium.org/650180/diff/6071/7090 File chrome/browser/views/location_bar_view.h (right): http://codereview.chromium.org/650180/diff/6071/7090#newcode662 chrome/browser/views/location_bar_view.h:662: //GeolocationImageView geolocation_image_view_; On 2010/02/25 16:54:41, Jeremy Orlow wrote: > delete good spot, deleted. http://codereview.chromium.org/650180/diff/6071/7091 File chrome/browser/views/options/content_settings_window_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7091#newcode177 chrome/browser/views/options/content_settings_window_view.cc:177: geolocation_page, false); On 2010/02/25 16:54:41, Jeremy Orlow wrote: > line up on ( hmm, not sure I understood: this is similar to the one above (169-171)? http://codereview.chromium.org/650180/diff/6071/7093 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/6071/7093#newcode102 chrome/browser/views/options/geolocation_filter_page_view.cc:102: const views::Event& event) { On 2010/02/25 16:54:41, Jeremy Orlow wrote: > line up or nothing after ( Done. http://codereview.chromium.org/650180/diff/6071/7093#newcode117 chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting = CONTENT_SETTING_ASK; On 2010/02/25 16:54:41, Jeremy Orlow wrote: > Just don't set in this case. Done. http://codereview.chromium.org/650180/diff/6071/7098 File chrome/renderer/geolocation_dispatcher.cc (right): http://codereview.chromium.org/650180/diff/6071/7098#newcode43 chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); On 2010/02/25 16:54:41, Jeremy Orlow wrote: > don't you mean host()? all the content settings are per host, not origin oh, good point. done (and changed the IPC so that it passes a std::string for host rather than a GURL).
FYI, split this into two changes: http://codereview.chromium.org/661272/show http://codereview.chromium.org/661273/show once they're in, this change will hopefully become a bit smaller.. On 2010/02/26 21:06:56, bulach wrote: > Hi, > > First of all, many thanks for putting up with such a huge change: unfortunately > introducing a new content setting proved more complex than we were expecting.. > > I'll try on Monday to split this in at least three changes: > 1. adding strings / resources > 2. the few files with those straightforward renaming > 3. then the chunky rest with UI bits for all platforms > > although it won't make (3) significantly smaller, it should help your review. > > meanwhile, all comments addressed so far, and replicated the change in GTK. > > http://codereview.chromium.org/650180/diff/6071/7075 > File chrome/browser/geolocation/geolocation_permission_context.cc (right): > > http://codereview.chromium.org/650180/diff/6071/7075#newcode118 > chrome/browser/geolocation/geolocation_permission_context.cc:118: > SavePermissionOnUIThread(host, allowed); > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > OnUIThread isn't needed > > Done. > > http://codereview.chromium.org/650180/diff/6071/7075#newcode187 > chrome/browser/geolocation/geolocation_permission_context.cc:187: > DCHECK(tab_contents); > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > not needed...you'll segfault on the next op anyway > > > > not sure having this in its own function is worth it. > > inlined in the two calling sites. > > http://codereview.chromium.org/650180/diff/6071/7075#newcode193 > chrome/browser/geolocation/geolocation_permission_context.cc:193: > DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > You can do this from any thread. I'm actually not even sure if its worth > having > > a special method for it since it's just one setting. > > inlined > > http://codereview.chromium.org/650180/diff/6071/7089 > File chrome/browser/views/location_bar_view.cc (right): > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 > chrome/browser/views/location_bar_view.cc:1361: if > (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { > On 2010/02/26 15:29:55, jothchrome wrote: > > maybe clearer to have an explicit > > static icons_initialized = false; > > if (!icons_initialized) { > > icons_initialized = true; > > ... > > > > Done. > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 > chrome/browser/views/location_bar_view.cc:1402: 0, > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > Add a comment > > Done. > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 > chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != > CONTENT_SETTINGS_TYPE_GEOLOCATION) { > On 2010/02/26 15:29:55, jothchrome wrote: > > this special case defeats the purpose of the generalization of blocked_icons_ > > and allowed_icons_ > > ignoring the tooltip for a moment, the logic here should be > > > > const SkBitmap* icon = NULL; > > const bool content_blocked = tab_contents->IsContentBlocked(content_type_); > > if (content_blocked) { > > icon = blocked_icons_[content_type_]; > > } else { > > icon = allowed_icons_[content_type_]; > > } > > > > SetImage(icon); > > SetVisible(!!icon); > > > > > > // Now special case for the geolocation tooltip > > if (type == GEOLOCATION) { > > const std::map& settings = tab_contents->geolocaiton_setings(); > > > > if (settings.size() == 1) { > > // Special case for a single site, as this is the common case. > > tooltip = l10n_util::GetStringF(content_blocked ? > > IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, > > UTF8ToWide(settings.begin()->first)); > > } else { > > tooltip = l10n_util::GetString(content_blocked ? > > IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : > > IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); > > } > > > > SetTooltip(tooltip); > > > > > > as we chatted, using "IsContentBlocked" but still with a special case for > Geolocation. let me know if it's any clearer. > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 > chrome/browser/views/location_bar_view.cc:1424: const std::map<std::string, > ContentSetting> settings = > On 2010/02/26 15:29:55, jothchrome wrote: > > make this a const& too > > Done. > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 > chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type == > CONTENT_SETTINGS_TYPE_GEOLOCATION) { > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > Hmm...add a comment maybe > > done (let me know if it's any clear) > > http://codereview.chromium.org/650180/diff/6071/7090 > File chrome/browser/views/location_bar_view.h (right): > > http://codereview.chromium.org/650180/diff/6071/7090#newcode662 > chrome/browser/views/location_bar_view.h:662: //GeolocationImageView > geolocation_image_view_; > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > delete > > good spot, deleted. > > http://codereview.chromium.org/650180/diff/6071/7091 > File chrome/browser/views/options/content_settings_window_view.cc (right): > > http://codereview.chromium.org/650180/diff/6071/7091#newcode177 > chrome/browser/views/options/content_settings_window_view.cc:177: > geolocation_page, false); > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > line up on ( > > hmm, not sure I understood: this is similar to the one above (169-171)? > > http://codereview.chromium.org/650180/diff/6071/7093 > File chrome/browser/views/options/geolocation_filter_page_view.cc (right): > > http://codereview.chromium.org/650180/diff/6071/7093#newcode102 > chrome/browser/views/options/geolocation_filter_page_view.cc:102: const > views::Event& event) { > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > line up or nothing after ( > > Done. > > http://codereview.chromium.org/650180/diff/6071/7093#newcode117 > chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting = > CONTENT_SETTING_ASK; > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > Just don't set in this case. > > Done. > > http://codereview.chromium.org/650180/diff/6071/7098 > File chrome/renderer/geolocation_dispatcher.cc (right): > > http://codereview.chromium.org/650180/diff/6071/7098#newcode43 > chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); > On 2010/02/25 16:54:41, Jeremy Orlow wrote: > > don't you mean host()? all the content settings are per host, not origin > > oh, good point. done (and changed the IPC so that it passes a std::string for > host rather than a GURL).
Thanks for doing that. Took a quick look and didn't see any issues, though I still think it's worth Joth taking a look. I bet you could pull even more out of this if you tried. For example, you could add some of the code for the dialogs and then hook them up in a subsequent CL. In webkit, they frown upon any patches bigger than about 70k...and I think it's actually a pretty decent goal for even Chromium. On Mon, Mar 1, 2010 at 1:43 PM, <bulach@chromium.org> wrote: > FYI, > > split this into two changes: > http://codereview.chromium.org/661272/show > http://codereview.chromium.org/661273/show > > once they're in, this change will hopefully become a bit smaller.. > > > On 2010/02/26 21:06:56, bulach wrote: > >> Hi, >> > > First of all, many thanks for putting up with such a huge change: >> > unfortunately > >> introducing a new content setting proved more complex than we were >> expecting.. >> > > I'll try on Monday to split this in at least three changes: >> 1. adding strings / resources >> 2. the few files with those straightforward renaming >> 3. then the chunky rest with UI bits for all platforms >> > > although it won't make (3) significantly smaller, it should help your >> review. >> > > meanwhile, all comments addressed so far, and replicated the change in >> GTK. >> > > http://codereview.chromium.org/650180/diff/6071/7075 >> File chrome/browser/geolocation/geolocation_permission_context.cc (right): >> > > http://codereview.chromium.org/650180/diff/6071/7075#newcode118 >> chrome/browser/geolocation/geolocation_permission_context.cc:118: >> SavePermissionOnUIThread(host, allowed); >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > OnUIThread isn't needed >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7075#newcode187 >> chrome/browser/geolocation/geolocation_permission_context.cc:187: >> DCHECK(tab_contents); >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > not needed...you'll segfault on the next op anyway >> > >> > not sure having this in its own function is worth it. >> > > inlined in the two calling sites. >> > > http://codereview.chromium.org/650180/diff/6071/7075#newcode193 >> chrome/browser/geolocation/geolocation_permission_context.cc:193: >> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > You can do this from any thread. I'm actually not even sure if its >> worth >> having >> > a special method for it since it's just one setting. >> > > inlined >> > > http://codereview.chromium.org/650180/diff/6071/7089 >> File chrome/browser/views/location_bar_view.cc (right): >> > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 >> chrome/browser/views/location_bar_view.cc:1361: if >> (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { >> On 2010/02/26 15:29:55, jothchrome wrote: >> > maybe clearer to have an explicit >> > static icons_initialized = false; >> > if (!icons_initialized) { >> > icons_initialized = true; >> > ... >> > >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 >> chrome/browser/views/location_bar_view.cc:1402: 0, >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > Add a comment >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 >> chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != >> CONTENT_SETTINGS_TYPE_GEOLOCATION) { >> On 2010/02/26 15:29:55, jothchrome wrote: >> > this special case defeats the purpose of the generalization of >> > blocked_icons_ > >> > and allowed_icons_ >> > ignoring the tooltip for a moment, the logic here should be >> > >> > const SkBitmap* icon = NULL; >> > const bool content_blocked = >> tab_contents->IsContentBlocked(content_type_); >> > if (content_blocked) { >> > icon = blocked_icons_[content_type_]; >> > } else { >> > icon = allowed_icons_[content_type_]; >> > } >> > >> > SetImage(icon); >> > SetVisible(!!icon); >> > >> > >> > // Now special case for the geolocation tooltip >> > if (type == GEOLOCATION) { >> > const std::map& settings = tab_contents->geolocaiton_setings(); >> > >> > if (settings.size() == 1) { >> > // Special case for a single site, as this is the common case. >> > tooltip = l10n_util::GetStringF(content_blocked ? >> > IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, >> > UTF8ToWide(settings.begin()->first)); >> > } else { >> > tooltip = l10n_util::GetString(content_blocked ? >> > IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : >> > IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); >> > } >> > >> > SetTooltip(tooltip); >> > >> > >> > > as we chatted, using "IsContentBlocked" but still with a special case for >> Geolocation. let me know if it's any clearer. >> > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 >> chrome/browser/views/location_bar_view.cc:1424: const >> std::map<std::string, >> ContentSetting> settings = >> On 2010/02/26 15:29:55, jothchrome wrote: >> > make this a const& too >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 >> chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type >> == >> CONTENT_SETTINGS_TYPE_GEOLOCATION) { >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > Hmm...add a comment maybe >> > > done (let me know if it's any clear) >> > > http://codereview.chromium.org/650180/diff/6071/7090 >> File chrome/browser/views/location_bar_view.h (right): >> > > http://codereview.chromium.org/650180/diff/6071/7090#newcode662 >> chrome/browser/views/location_bar_view.h:662: //GeolocationImageView >> geolocation_image_view_; >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > delete >> > > good spot, deleted. >> > > http://codereview.chromium.org/650180/diff/6071/7091 >> File chrome/browser/views/options/content_settings_window_view.cc (right): >> > > http://codereview.chromium.org/650180/diff/6071/7091#newcode177 >> chrome/browser/views/options/content_settings_window_view.cc:177: >> geolocation_page, false); >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > line up on ( >> > > hmm, not sure I understood: this is similar to the one above (169-171)? >> > > http://codereview.chromium.org/650180/diff/6071/7093 >> File chrome/browser/views/options/geolocation_filter_page_view.cc (right): >> > > http://codereview.chromium.org/650180/diff/6071/7093#newcode102 >> chrome/browser/views/options/geolocation_filter_page_view.cc:102: const >> views::Event& event) { >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > line up or nothing after ( >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7093#newcode117 >> chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting >> = >> CONTENT_SETTING_ASK; >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > Just don't set in this case. >> > > Done. >> > > http://codereview.chromium.org/650180/diff/6071/7098 >> File chrome/renderer/geolocation_dispatcher.cc (right): >> > > http://codereview.chromium.org/650180/diff/6071/7098#newcode43 >> chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); >> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >> > don't you mean host()? all the content settings are per host, not >> origin >> > > oh, good point. done (and changed the IPC so that it passes a std::string >> for >> host rather than a GURL). >> > > > > http://codereview.chromium.org/650180 >
Thanks for the advise! I'll split this further and add the dialog bits required for this change... On Mon, Mar 1, 2010 at 1:52 PM, Jeremy Orlow <jorlow@chromium.org> wrote: > Thanks for doing that. Â Took a quick look and didn't see any issues, though > I still think it's worth Joth taking a look. > I bet you could pull even more out of this if you tried. Â For example, you > could add some of the code for the dialogs and then hook them up in a > subsequent CL. > In webkit, they frown upon any patches bigger than about 70k...and I think > it's actually a pretty decent goal for even Chromium. > > On Mon, Mar 1, 2010 at 1:43 PM, <bulach@chromium.org> wrote: >> >> FYI, >> >> split this into two changes: >> http://codereview.chromium.org/661272/show >> http://codereview.chromium.org/661273/show >> >> once they're in, this change will hopefully become a bit smaller.. >> >> On 2010/02/26 21:06:56, bulach wrote: >>> >>> Hi, >> >>> First of all, many thanks for putting up with such a huge change: >> >> unfortunately >>> >>> introducing a new content setting proved more complex than we were >>> expecting.. >> >>> I'll try on Monday to split this in at least three changes: >>> 1. adding strings / resources >>> 2. the few files with those straightforward renaming >>> 3. then the chunky rest with UI bits for all platforms >> >>> although it won't make (3) significantly smaller, it should help your >>> review. >> >>> meanwhile, all comments addressed so far, and replicated the change in >>> GTK. >> >>> http://codereview.chromium.org/650180/diff/6071/7075 >>> File chrome/browser/geolocation/geolocation_permission_context.cc >>> (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode118 >>> chrome/browser/geolocation/geolocation_permission_context.cc:118: >>> SavePermissionOnUIThread(host, allowed); >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > OnUIThread isn't needed >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode187 >>> chrome/browser/geolocation/geolocation_permission_context.cc:187: >>> DCHECK(tab_contents); >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > not needed...you'll segfault on the next op anyway >>> > >>> > not sure having this in its own function is worth it. >> >>> inlined in the two calling sites. >> >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode193 >>> chrome/browser/geolocation/geolocation_permission_context.cc:193: >>> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > You can do this from any thread. Â I'm actually not even sure if its >>> > worth >>> having >>> > a special method for it since it's just one setting. >> >>> inlined >> >>> http://codereview.chromium.org/650180/diff/6071/7089 >>> File chrome/browser/views/location_bar_view.cc (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 >>> chrome/browser/views/location_bar_view.cc:1361: if >>> (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { >>> On 2010/02/26 15:29:55, jothchrome wrote: >>> > maybe clearer to have an explicit >>> > static icons_initialized = false; >>> > if (!icons_initialized) { >>> > Â icons_initialized = true; >>> > Â ... >>> > >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 >>> chrome/browser/views/location_bar_view.cc:1402: 0, >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > Add a comment >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 >>> chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != >>> CONTENT_SETTINGS_TYPE_GEOLOCATION) { >>> On 2010/02/26 15:29:55, jothchrome wrote: >>> > this special case defeats the purpose of the generalization of >> >> blocked_icons_ >>> >>> > and allowed_icons_ >>> > ignoring the tooltip for a moment, the logic here should be >>> > >>> > const SkBitmap* icon = NULL; >>> > const bool content_blocked = >>> > tab_contents->IsContentBlocked(content_type_); >>> > if (content_blocked) { >>> > Â icon = blocked_icons_[content_type_]; >>> > } else { >>> > Â icon = allowed_icons_[content_type_]; >>> > } >>> > >>> > SetImage(icon); >>> > SetVisible(!!icon); >>> > >>> > >>> > // Now special case for the geolocation tooltip >>> > if (type == GEOLOCATION) { >>> > const std::map& settings = tab_contents->geolocaiton_setings(); >>> > >>> > if (settings.size() == 1) { >>> > // Special case for a single site, as this is the common case. >>> > tooltip = l10n_util::GetStringF(content_blocked ? >>> > IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, >>> > Â Â Â Â Â Â Â Â Â Â Â UTF8ToWide(settings.begin()->first)); >>> > } else { >>> > tooltip = l10n_util::GetString(content_blocked ? >>> > IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : >>> > IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); >>> > } >>> > >>> > SetTooltip(tooltip); >>> > >>> > >> >>> as we chatted, using "IsContentBlocked" but still with a special case for >>> Geolocation. let me know if it's any clearer. >> >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 >>> chrome/browser/views/location_bar_view.cc:1424: const >>> std::map<std::string, >>> ContentSetting> settings = >>> On 2010/02/26 15:29:55, jothchrome wrote: >>> > make this a const& too >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 >>> chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type >>> == >>> CONTENT_SETTINGS_TYPE_GEOLOCATION) { >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > Hmm...add a comment maybe >> >>> done (let me know if it's any clear) >> >>> http://codereview.chromium.org/650180/diff/6071/7090 >>> File chrome/browser/views/location_bar_view.h (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7090#newcode662 >>> chrome/browser/views/location_bar_view.h:662: //GeolocationImageView >>> geolocation_image_view_; >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > delete >> >>> good spot, deleted. >> >>> http://codereview.chromium.org/650180/diff/6071/7091 >>> File chrome/browser/views/options/content_settings_window_view.cc >>> (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7091#newcode177 >>> chrome/browser/views/options/content_settings_window_view.cc:177: >>> geolocation_page, false); >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > line up on ( >> >>> hmm, not sure I understood: this is similar to the one above (169-171)? >> >>> http://codereview.chromium.org/650180/diff/6071/7093 >>> File chrome/browser/views/options/geolocation_filter_page_view.cc >>> (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7093#newcode102 >>> chrome/browser/views/options/geolocation_filter_page_view.cc:102: const >>> views::Event& event) { >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > line up or nothing after ( >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7093#newcode117 >>> chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting >>> = >>> CONTENT_SETTING_ASK; >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > Just don't set in this case. >> >>> Done. >> >>> http://codereview.chromium.org/650180/diff/6071/7098 >>> File chrome/renderer/geolocation_dispatcher.cc (right): >> >>> http://codereview.chromium.org/650180/diff/6071/7098#newcode43 >>> chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: >>> > don't you mean host()? Â all the content settings are per host, not >>> > origin >> >>> oh, good point. done (and changed the IPC so that it passes a std::string >>> for >>> host rather than a GURL). >> >> >> >> http://codereview.chromium.org/650180 > >
FYI, another change with a smaller scope, in preparation for this: http://codereview.chromium.org/660279/show I'm happy with this workflow: a big CL showing the "big picture" and giving some context, and then split into smaller ones preparing for it. Let me know if you have more ideas on how to streamline this process. Hopefully this big one will get much slimmer once we get back to it. Thanks, Marcus On 2010/03/01 15:04:28, bulach wrote: > Thanks for the advise! I'll split this further and add the dialog bits > required for this change... > > > On Mon, Mar 1, 2010 at 1:52 PM, Jeremy Orlow <mailto:jorlow@chromium.org> wrote: > > Thanks for doing that. Took a quick look and didn't see any issues, though > > I still think it's worth Joth taking a look. > > I bet you could pull even more out of this if you tried. For example, you > > could add some of the code for the dialogs and then hook them up in a > > subsequent CL. > > In webkit, they frown upon any patches bigger than about 70k...and I think > > it's actually a pretty decent goal for even Chromium. > > > > On Mon, Mar 1, 2010 at 1:43 PM, <mailto:bulach@chromium.org> wrote: > >> > >> FYI, > >> > >> split this into two changes: > >> http://codereview.chromium.org/661272/show > >> http://codereview.chromium.org/661273/show > >> > >> once they're in, this change will hopefully become a bit smaller.. > >> > >> On 2010/02/26 21:06:56, bulach wrote: > >>> > >>> Hi, > >> > >>> First of all, many thanks for putting up with such a huge change: > >> > >> unfortunately > >>> > >>> introducing a new content setting proved more complex than we were > >>> expecting.. > >> > >>> I'll try on Monday to split this in at least three changes: > >>> 1. adding strings / resources > >>> 2. the few files with those straightforward renaming > >>> 3. then the chunky rest with UI bits for all platforms > >> > >>> although it won't make (3) significantly smaller, it should help your > >>> review. > >> > >>> meanwhile, all comments addressed so far, and replicated the change in > >>> GTK. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7075 > >>> File chrome/browser/geolocation/geolocation_permission_context.cc > >>> (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode118 > >>> chrome/browser/geolocation/geolocation_permission_context.cc:118: > >>> SavePermissionOnUIThread(host, allowed); > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > OnUIThread isn't needed > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode187 > >>> chrome/browser/geolocation/geolocation_permission_context.cc:187: > >>> DCHECK(tab_contents); > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > not needed...you'll segfault on the next op anyway > >>> > > >>> > not sure having this in its own function is worth it. > >> > >>> inlined in the two calling sites. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7075#newcode193 > >>> chrome/browser/geolocation/geolocation_permission_context.cc:193: > >>> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > You can do this from any thread. I'm actually not even sure if its > >>> > worth > >>> having > >>> > a special method for it since it's just one setting. > >> > >>> inlined > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089 > >>> File chrome/browser/views/location_bar_view.cc (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1361 > >>> chrome/browser/views/location_bar_view.cc:1361: if > >>> (!blocked_icons_[CONTENT_SETTINGS_TYPE_COOKIES]) { > >>> On 2010/02/26 15:29:55, jothchrome wrote: > >>> > maybe clearer to have an explicit > >>> > static icons_initialized = false; > >>> > if (!icons_initialized) { > >>> > icons_initialized = true; > >>> > ... > >>> > > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1402 > >>> chrome/browser/views/location_bar_view.cc:1402: 0, > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > Add a comment > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1419 > >>> chrome/browser/views/location_bar_view.cc:1419: if (content_type_ != > >>> CONTENT_SETTINGS_TYPE_GEOLOCATION) { > >>> On 2010/02/26 15:29:55, jothchrome wrote: > >>> > this special case defeats the purpose of the generalization of > >> > >> blocked_icons_ > >>> > >>> > and allowed_icons_ > >>> > ignoring the tooltip for a moment, the logic here should be > >>> > > >>> > const SkBitmap* icon = NULL; > >>> > const bool content_blocked = > >>> > tab_contents->IsContentBlocked(content_type_); > >>> > if (content_blocked) { > >>> > icon = blocked_icons_[content_type_]; > >>> > } else { > >>> > icon = allowed_icons_[content_type_]; > >>> > } > >>> > > >>> > SetImage(icon); > >>> > SetVisible(!!icon); > >>> > > >>> > > >>> > // Now special case for the geolocation tooltip > >>> > if (type == GEOLOCATION) { > >>> > const std::map& settings = tab_contents->geolocaiton_setings(); > >>> > > >>> > if (settings.size() == 1) { > >>> > // Special case for a single site, as this is the common case. > >>> > tooltip = l10n_util::GetStringF(content_blocked ? > >>> > IDS_GEOLOCATION_BLOCKED_TOOLTIP : IDS_GEOLOCATION_ALLOWED_TOOLTIP, > >>> > UTF8ToWide(settings.begin()->first)); > >>> > } else { > >>> > tooltip = l10n_util::GetString(content_blocked ? > >>> > IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP : > >>> > IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP); > >>> > } > >>> > > >>> > SetTooltip(tooltip); > >>> > > >>> > > >> > >>> as we chatted, using "IsContentBlocked" but still with a special case for > >>> Geolocation. let me know if it's any clearer. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1424 > >>> chrome/browser/views/location_bar_view.cc:1424: const > >>> std::map<std::string, > >>> ContentSetting> settings = > >>> On 2010/02/26 15:29:55, jothchrome wrote: > >>> > make this a const& too > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7089#newcode1477 > >>> chrome/browser/views/location_bar_view.cc:1477: if (content_settings_type > >>> == > >>> CONTENT_SETTINGS_TYPE_GEOLOCATION) { > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > Hmm...add a comment maybe > >> > >>> done (let me know if it's any clear) > >> > >>> http://codereview.chromium.org/650180/diff/6071/7090 > >>> File chrome/browser/views/location_bar_view.h (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7090#newcode662 > >>> chrome/browser/views/location_bar_view.h:662: //GeolocationImageView > >>> geolocation_image_view_; > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > delete > >> > >>> good spot, deleted. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7091 > >>> File chrome/browser/views/options/content_settings_window_view.cc > >>> (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7091#newcode177 > >>> chrome/browser/views/options/content_settings_window_view.cc:177: > >>> geolocation_page, false); > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > line up on ( > >> > >>> hmm, not sure I understood: this is similar to the one above (169-171)? > >> > >>> http://codereview.chromium.org/650180/diff/6071/7093 > >>> File chrome/browser/views/options/geolocation_filter_page_view.cc > >>> (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7093#newcode102 > >>> chrome/browser/views/options/geolocation_filter_page_view.cc:102: const > >>> views::Event& event) { > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > line up or nothing after ( > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7093#newcode117 > >>> chrome/browser/views/options/geolocation_filter_page_view.cc:117: setting > >>> = > >>> CONTENT_SETTING_ASK; > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > Just don't set in this case. > >> > >>> Done. > >> > >>> http://codereview.chromium.org/650180/diff/6071/7098 > >>> File chrome/renderer/geolocation_dispatcher.cc (right): > >> > >>> http://codereview.chromium.org/650180/diff/6071/7098#newcode43 > >>> chrome/renderer/geolocation_dispatcher.cc:43: GURL(url).GetOrigin())); > >>> On 2010/02/25 16:54:41, Jeremy Orlow wrote: > >>> > don't you mean host()? all the content settings are per host, not > >>> > origin > >> > >>> oh, good point. done (and changed the IPC so that it passes a std::string > >>> for > >>> host rather than a GURL). > >> > >> > >> > >> http://codereview.chromium.org/650180 > > > > >
Please do not hijack the content blocked icons array to shoehorn geolocation
icons into it.
Feel free to subclass the current base class to avoid copy and pasting too much
code, but r- on your existing solution.
Make sure you also follow common practice for formatting. Just in the one file
I looked at I saw {} used on single-line conditional bodies, and weird indents
like
Foo::
bar::baz =
{ 0 };
Belay my last comment. I'd like to see the mocks for this and screenshots of it in action, that will help me decide what sort of code organization makes the most sense. Please don't land your "precursor" changes before this. The reason I'm concerned is that this is a large complex addition to code that already needed refactoring and it's coming at me with no warning or context, so I need to get up to speed. Also please add BUG= and TEST= lines to your description.
Peter, you don't think we're going to have other privacy related things in the future that have both blocked and allowed icons? I'm not crazy about the current design, but after talking to Marcus, it was the best we could come up with. This _needs_ to be in mstone5 and thus needs to be in the dev channel in the next couple weeks. And this change doesn't even get us 100% of the way there. Since we're in different time zones, I'm gong to schedule a VC meeting in the morning your time so we can come up with a plan. Additionally, anything you can do to review london stuff first thing when you get in will be a big help, since there's only a short period of time where we're both in the office and can have any sort of dialog in these bugs/reviews. Lastly, a bunch of the patches Marcus is breaking out have nothing to do with what you're objecting to, so we'll continue moving those forward, but will be sure not to commit this stuff just quite yet. (If you do have issues with any of the other stuff, we can always clean it up afterwards.)
Thanks for your comments, Peter! As Jeremy said, I'm splitting this up in smaller pieces, will keep this change as a "big picture" so we can see where everything falls into place. I'll hold on submitting any controversial bits. As for screenshots, please find them down the bottom on "UI Mocks" section at: https://docs.google.com/View?id=dfbnm49n_0dpc7pxpx The basic idea is that geolocation introduces a "contentsettings" that is not mapped 1:1 as "content*blocked*settings" as the current implementation: the icon will be shown when either in-use by any frame in the page, or when it's blocked. I'm not sure if you followed the whole review thread, but you may want to step backwards a bit: http://codereview.chromium.org/650180/diff/7004/6055 on that patchset, there was a new class rather than changing / renaming the existing behavior, which I think it's your suggestion? I'm happy to go back to that design if you think it makes more sense than extending / renaming the current classes? Thanks, Marcus On Tue, Mar 2, 2010 at 10:24 AM, <jorlow@chromium.org> wrote: > Peter, you don't think we're going to have other privacy related things in > the > future that have both blocked and allowed icons? Â I'm not crazy about the > current design, but after talking to Marcus, it was the best we could come > up > with. > > This _needs_ to be in mstone5 and thus needs to be in the dev channel in the > next couple weeks. Â And this change doesn't even get us 100% of the way > there. > Since we're in different time zones, I'm gong to schedule a VC meeting in > the > morning your time so we can come up with a plan. Â Additionally, anything you > can > do to review london stuff first thing when you get in will be a big help, > since > there's only a short period of time where we're both in the office and can > have > any sort of dialog in these bugs/reviews. > > Lastly, a bunch of the patches Marcus is breaking out have nothing to do > with > what you're objecting to, so we'll continue moving those forward, but will > be > sure not to commit this stuff just quite yet. Â (If you do have issues with > any > of the other stuff, we can always clean it up afterwards.) > > http://codereview.chromium.org/650180 >
Hi Peter, Thanks for the meeting today, I appreciate your help and the clarifications you made. To help your review, here's the two different approaches: 1. The original one, sub-classing / changing ContentBlockedImageView and introducing GeolocationImageView: http://codereview.chromium.org/650180/diff/7004/6055 2. The latest one on a self-contained change renaming ContentBlockedImageview to ContentSettingImageView: http://codereview.chromium.org/660279/show If you're happy with (2), please take a thorough look at that change instead of this one. Once that goes in (and a few other less controversial changes), I'll merge them here and things will get much simpler. If you're _not_ happy with (2) :) let me know and I can work from there. Thanks, Marcus On 2010/03/02 10:37:38, bulach wrote: > Thanks for your comments, Peter! > > As Jeremy said, I'm splitting this up in smaller pieces, will keep > this change as a "big picture" so we can see where everything falls > into place. I'll hold on submitting any controversial bits. > As for screenshots, please find them down the bottom on "UI Mocks" section at: > https://docs.google.com/View?id=dfbnm49n_0dpc7pxpx > > The basic idea is that geolocation introduces a "contentsettings" that > is not mapped 1:1 as "content*blocked*settings" as the current > implementation: the icon will be shown when either in-use by any frame > in the page, or when it's blocked. > > I'm not sure if you followed the whole review thread, but you may want > to step backwards a bit: > http://codereview.chromium.org/650180/diff/7004/6055 > > on that patchset, there was a new class rather than changing / > renaming the existing behavior, which I think it's your suggestion? > I'm happy to go back to that design if you think it makes more sense > than extending / renaming the current classes? > > Thanks, > Marcus > > > > On Tue, Mar 2, 2010 at 10:24 AM, <mailto:jorlow@chromium.org> wrote: > > Peter, you don't think we're going to have other privacy related things in > > the > > future that have both blocked and allowed icons? I'm not crazy about the > > current design, but after talking to Marcus, it was the best we could come > > up > > with. > > > > This _needs_ to be in mstone5 and thus needs to be in the dev channel in the > > next couple weeks. And this change doesn't even get us 100% of the way > > there. > > Since we're in different time zones, I'm gong to schedule a VC meeting in > > the > > morning your time so we can come up with a plan. Additionally, anything you > > can > > do to review london stuff first thing when you get in will be a big help, > > since > > there's only a short period of time where we're both in the office and can > > have > > any sort of dialog in these bugs/reviews. > > > > Lastly, a bunch of the patches Marcus is breaking out have nothing to do > > with > > what you're objecting to, so we'll continue moving those forward, but will > > be > > sure not to commit this stuff just quite yet. (If you do have issues with > > any > > of the other stuff, we can always clean it up afterwards.) > > > > http://codereview.chromium.org/650180 > > >
On Tue, Mar 2, 2010 at 11:24 AM, <bulach@chromium.org> wrote: > To help your review, here's the two different approaches: > 1. The original one, sub-classing / changing ContentBlockedImageView and > introducing GeolocationImageView: > > http://codereview.chromium.org/650180/diff/7004/6055 > I glanced at the idea here. It looks like this creates two parallel classes, one of which is used for the existing icons and one of which is used for geolocation. This seems like a non-optimal solution because there's a lot of duplicated code, and anyway the "existing" class still has to be modified to know about things like "I shouldn't handle content type X". Thoughts on a different design below. 2. The latest one on a self-contained change renaming > ContentBlockedImageview to > ContentSettingImageView: > > http://codereview.chromium.org/660279/show > Comments sent. Here's the rough design I'm currently considering: * Check in the rename of ContentBlockedImageView to ContentSettingImageView, and make its declaration purposefully vague about details like what images it's keeping track of. * Add a subclass, named e.g. CSOnlyBlockedImageView, that is used when we only want to show an image for blocked content. * Add a second subclass, named e.g. CSAlwaysShownImageView or CSAnySettingImageView, that is used for geolocation and other cases that want to show both "blocked" and "allowed" icons. * The only contents of these two classes will be different implementations of the "update my state" function, and a constructor that initializes any necessary images (which can be kept as private members on the subclasses, while the parent knows nothing). * In the future, when I add Zoom, I may do it by adding a third subclass, or I may decide at that point that even that makes no sense, if none of the code in the base class applies. Seems like it will, though. This design minimizes copy-and-pasting and allows the location bar to have a single array of pointers-to-the-base-class, and keeps the differences in logic between the various types confined to separate classes. As I mentioned in the review comments I sent, it might make sense to move these classes to a separate file for clarity, and it's also possible that we could reuse some code across platforms, although I want to be careful that that doesn't blow up into some crazy multiple virtual inheritance design :D WDYT? http://codereview.chromium.org/650180 >
I'll be doing the full-merge in the following hours, but I'd appreciate your comments on the "BubbleContents" bits. Here's some ideas: 1. Leave as it is: the current impl hides the fact that only geolocation might have multiple radios, however it's spread across platforms and surely, not generic. 2. Make it more generic (again, with a cross-platform model) that would supply a list of titles and labels for allow/deny radios. Instead of "ContentBlockedBubblePLATFORM", there would be "ContentSettingBubblePLATFORM" which would traverse a list of titles/labels from some kind of "ContentSettingBubbleModel". I'm happy either way, and if you choose (2), I'd again split in a separate change doing only s/ContentBlockedBubble/ContentSettingBubble/ and introducing the ability to have multiple radios before actually adding geolocation. ...and of course, if we go with (2), no need to look any further on this change yet.. :) thoughts? On 2010/03/02 22:18:14, Peter Kasting wrote: > On Tue, Mar 2, 2010 at 11:24 AM, <mailto:bulach@chromium.org> wrote: > > > To help your review, here's the two different approaches: > > 1. The original one, sub-classing / changing ContentBlockedImageView and > > introducing GeolocationImageView: > > > > http://codereview.chromium.org/650180/diff/7004/6055 > > > > I glanced at the idea here. It looks like this creates two parallel > classes, one of which is used for the existing icons and one of which is > used for geolocation. This seems like a non-optimal solution because > there's a lot of duplicated code, and anyway the "existing" class still has > to be modified to know about things like "I shouldn't handle content type > X". > > Thoughts on a different design below. > > 2. The latest one on a self-contained change renaming > > ContentBlockedImageview to > > ContentSettingImageView: > > > > http://codereview.chromium.org/660279/show > > > > Comments sent. > > Here's the rough design I'm currently considering: > > * Check in the rename of ContentBlockedImageView to ContentSettingImageView, > and make its declaration purposefully vague about details like what images > it's keeping track of. > * Add a subclass, named e.g. CSOnlyBlockedImageView, that is used when we > only want to show an image for blocked content. > * Add a second subclass, named e.g. CSAlwaysShownImageView or > CSAnySettingImageView, that is used for geolocation and other cases that > want to show both "blocked" and "allowed" icons. > * The only contents of these two classes will be different implementations > of the "update my state" function, and a constructor that initializes any > necessary images (which can be kept as private members on the subclasses, > while the parent knows nothing). > * In the future, when I add Zoom, I may do it by adding a third subclass, or > I may decide at that point that even that makes no sense, if none of the > code in the base class applies. Seems like it will, though. > > This design minimizes copy-and-pasting and allows the location bar to have a > single array of pointers-to-the-base-class, and keeps the differences in > logic between the various types confined to separate classes. As I > mentioned in the review comments I sent, it might make sense to move these > classes to a separate file for clarity, and it's also possible that we could > reuse some code across platforms, although I want to be careful that that > doesn't blow up into some crazy multiple virtual inheritance design :D > > WDYT? > > http://codereview.chromium.org/650180 > > >
On 2010/03/04 12:01:45, bulach wrote: > I'll be doing the full-merge in the following hours, but I'd appreciate your > comments on the "BubbleContents" bits. > > Here's some ideas: > 1. Leave as it is: the current impl hides the fact that only geolocation might > have multiple radios, however it's spread across platforms and surely, not > generic. > > 2. Make it more generic (again, with a cross-platform model) that would supply a > list of titles and labels for allow/deny radios. Instead of > "ContentBlockedBubblePLATFORM", there would be "ContentSettingBubblePLATFORM" > which would traverse a list of titles/labels from some kind of > "ContentSettingBubbleModel". > > I'm happy either way, and if you choose (2), I'd again split in a separate > change doing only s/ContentBlockedBubble/ContentSettingBubble/ and introducing > the ability to have multiple radios before actually adding geolocation. > > ...and of course, if we go with (2), no need to look any further on this change > yet.. :) > > thoughts? The frustrating bit about how it is right now is how we have to duplicate private members like |manage_link_| and chunks of code from the parent class. It seems worth doing _something_ to avoid that. After thinking about a couple different ways to do this, the model/view split seems like it's probably the best, because in addition to dealing with geolocation (and maybe the existing POPUP special case too), it can hopefully get rid of a lot of platform-specific duplication that there's no other good way to deal with.
Hi all, I merged with the other changes and cleaned up this change. I think it's in a good shape for a first review, the only duplication left is about the platform-specific "geolocation_filter_page" (the tab in the content settings dialog). do you think it's worth to extract a model and make it more generic, similar to what I did for the icon and bubble? I'm happy to either do it in parallel (so we get the core geolocation bits in), do it a pre-requisite for this change (so we trim this change a bit), or leave as it is. On 2010/03/04 19:41:59, Peter Kasting wrote: > On 2010/03/04 12:01:45, bulach wrote: > > I'll be doing the full-merge in the following hours, but I'd appreciate your > > comments on the "BubbleContents" bits. > > > > Here's some ideas: > > 1. Leave as it is: the current impl hides the fact that only geolocation might > > have multiple radios, however it's spread across platforms and surely, not > > generic. > > > > 2. Make it more generic (again, with a cross-platform model) that would supply > a > > list of titles and labels for allow/deny radios. Instead of > > "ContentBlockedBubblePLATFORM", there would be "ContentSettingBubblePLATFORM" > > which would traverse a list of titles/labels from some kind of > > "ContentSettingBubbleModel". > > > > I'm happy either way, and if you choose (2), I'd again split in a separate > > change doing only s/ContentBlockedBubble/ContentSettingBubble/ and introducing > > the ability to have multiple radios before actually adding geolocation. > > > > ...and of course, if we go with (2), no need to look any further on this > change > > yet.. :) > > > > thoughts? > > The frustrating bit about how it is right now is how we have to duplicate > private members like |manage_link_| and chunks of code from the parent class. > It seems worth doing _something_ to avoid that. > > After thinking about a couple different ways to do this, the model/view split > seems like it's probably the best, because in addition to dealing with > geolocation (and maybe the existing POPUP special case too), it can hopefully > get rid of a lot of platform-specific duplication that there's no other good way > to deal with.
I started to review this but stalled with the UI discussion. Let me make a few broad comments so you have something to do in the meantime: * Make sure your new strings are grammatically sound, and that resource IDs are grouped with other similar IDs (e.g. various things about content settings are already in groups) * Especially for the model classes you previously added, there's a lot of definition-in-declaration stuff -- i.e. defining a member function body right at the initial declaration point, usually for subclasses that only appear in the .cc file. Personally I'd prefer to split these apart both for clarity (because this leaves bare declarations atop the file, which are easy to quickly glance at) and because this tells the compiler you want to inline these functions, which isn't really what you're trying to say.
Random style comments for some files... http://codereview.chromium.org/650180/diff/12027/15020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/12027/15020#newcode246 chrome/browser/tab_contents/tab_contents.h:246: get_geolocation_content_settings() const; getter syntax doesn't include "get_", and must be inlined if you're using unix_hacker formatting (being named like a variable implies it's as efficient as a variable, which isn't the case unless you're inlined). If you can't inline, use CamelCase with "Get" http://codereview.chromium.org/650180/diff/12027/15020#newcode673 chrome/browser/tab_contents/tab_contents.h:673: virtual void OnGeolocationPermissionSet( This should go in the RenderViewHostDelegate implementation section. http://codereview.chromium.org/650180/diff/12027/15020#newcode674 chrome/browser/tab_contents/tab_contents.h:674: const std::string& host, bool allowed); Only indent to 4 spaces when you can't align to the ( on the previous line, so this should have "host" on the previous line, and "allowed" right below it. Be sure to check the other places you declare this function as well.
Oh heck, I might as well send my full set of comments. Some of these duplicate what I already wrote, sorry. Overall reaction: Holy CRAP there's a large cost to pay for this "multiple frame" UI design. Now I REALLY am convinced this is the wrong way to go, the implementation makes us leave footprints all over the place! http://codereview.chromium.org/650180/diff/12027/15001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/650180/diff/12027/15001#newcode6724 chrome/app/generated_resources.grd:6724: </ph> wants to track your physical location Nit: It's a little weird that the phrasing here is subtlely different than below http://codereview.chromium.org/650180/diff/12027/15001#newcode6733 chrome/app/generated_resources.grd:6733: This site <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph> is tracking your current location. This isn't grammatical. Either add commas, say "This site is tracking your current location." (my preference), or "www.google.com is tracking your location." Even better might be "This site can track" or "This site can access". http://codereview.chromium.org/650180/diff/12027/15001#newcode6735 chrome/app/generated_resources.grd:6735: <message name="IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP" desc="Tooltip text when a page is allowed to use geolocation."> Nit: Give a description which distinguishes this from the above case. http://codereview.chromium.org/650180/diff/12027/15001#newcode6738 chrome/app/generated_resources.grd:6738: <message name="IDS_GEOLOCATION_BLOCKED_TOOLTIP" desc="Tooltip text when a page is blocked to use geolocation."> Nit: Description not grammatical; try "blocked from using" or "not allowed to use". http://codereview.chromium.org/650180/diff/12027/15001#newcode6739 chrome/app/generated_resources.grd:6739: This site <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph> is blocked from tracking your current location. Same comment as in the allowed case; here I suggest "This site is blocked from tracking your current location." http://codereview.chromium.org/650180/diff/12027/15001#newcode6741 chrome/app/generated_resources.grd:6741: <message name="IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP" desc="Tooltip text when a page is blocked to use geolocation."> Nit: Same comments as above http://codereview.chromium.org/650180/diff/12027/15001#newcode6744 chrome/app/generated_resources.grd:6744: <message name="IDS_GEOLOCATION_RADIO_TITLE" desc="Label shown above the radio buttons asking the user to set geolocation permission for this site."> Nit: These subsequent strings should perhaps go in the "Content blocking strings" section? http://codereview.chromium.org/650180/diff/12027/15001#newcode6745 chrome/app/generated_resources.grd:6745: This site, <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph>, wants to track your current location. Nit: I think it would be clearer to just say "This site wants" (my preference) or "www.google.com wants". http://codereview.chromium.org/650180/diff/12027/15001#newcode6751 chrome/app/generated_resources.grd:6751: Don't let this site track me Nit: For consistency with our other blocking UI you might want "Prevent this site from tracking me". http://codereview.chromium.org/650180/diff/12027/15001#newcode6756 chrome/app/generated_resources.grd:6756: <message name="IDS_GEOLOCATION_TAB_LABEL" desc="Label for Geolocation tab on Content Settings dialog"> Nit: These subsequent strings should perhaps go in the "Content Settings dialog data" section? http://codereview.chromium.org/650180/diff/12027/15001#newcode6766 chrome/app/generated_resources.grd:6766: Do not allow any site to track my current location Nit: Might want to remove "current" http://codereview.chromium.org/650180/diff/12027/15002 File chrome/app/theme/theme_resources.grd (right): http://codereview.chromium.org/650180/diff/12027/15002#newcode325 chrome/app/theme/theme_resources.grd:325: <include name="IDR_GEOLOCATION_ALLOWED_LOCATIONBAR_ICON" file="geolocation_allowed_locationbar_icon.png" type="BINDATA" /> Nit: Maybe these should be located with, and named (on disk and as identifiers) similar to, IDR_BLOCKED_COOKIES and friends? http://codereview.chromium.org/650180/diff/12027/15003 File chrome/browser/content_setting_bubble_model.cc (right): http://codereview.chromium.org/650180/diff/12027/15003#newcode21 chrome/browser/content_setting_bubble_model.cc:21: ContentSettingTitleAndLinkModel(TabContents* tab_contents, Profile* profile, Nit: Wrong indent http://codereview.chromium.org/650180/diff/12027/15003#newcode24 chrome/browser/content_setting_bubble_model.cc:24: SetTitle(); Nit: Wrong indent (2 lines) http://codereview.chromium.org/650180/diff/12027/15003#newcode29 chrome/browser/content_setting_bubble_model.cc:29: void SetTitle() { Two nits: * Personally, I'm not sure it really buys you much to declare these separate functions; I'd put the code in the constructor. I don't mind them, though, if you think it's more readable this way. * When you write the function definition in the class declaration, the compiler assumes you want it inlined everywhere. That's not really the case in this file, so I suggest splitting your declarations and definitions apart. (I think this is a little more readable too, in the sense that you can look at the class declarations and see quickly what functional areas they cover.) http://codereview.chromium.org/650180/diff/12027/15003#newcode165 chrome/browser/content_setting_bubble_model.cc:165: const std::map<std::string, ContentSetting>& settings = Nit: It would be nice to have typedefs for these, presumably declared in TabContents.h or somewhere? http://codereview.chromium.org/650180/diff/12027/15003#newcode201 chrome/browser/content_setting_bubble_model.cc:201: return new ContentSettingPopupBubbleModel(tab_contents, profile, Nit: You can probably omit the content_type here like you do for geolocation. http://codereview.chromium.org/650180/diff/12027/15003#newcode204 chrome/browser/content_setting_bubble_model.cc:204: if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { Nit: No need for {} http://codereview.chromium.org/650180/diff/12027/15004 File chrome/browser/content_setting_image_model.cc (right): http://codereview.chromium.org/650180/diff/12027/15004#newcode14 chrome/browser/content_setting_image_model.cc:14: explicit ContentSettingBlockedImageModel( Nit: See comment about definitions-in-declarations in content_setting_bubble_model.cc. http://codereview.chromium.org/650180/diff/12027/15004#newcode63 chrome/browser/content_setting_image_model.cc:63: const std::map<std::string, ContentSetting>& settings = Nit: See comment about typedef in content_setting_bubble_model.cc. http://codereview.chromium.org/650180/diff/12027/15004#newcode65 chrome/browser/content_setting_image_model.cc:65: if (settings.size() == 0) { Nit: Use settings.empty() http://codereview.chromium.org/650180/diff/12027/15004#newcode72 chrome/browser/content_setting_image_model.cc:72: IDR_GEOLOCATION_DENIED_LOCATIONBAR_ICON : Nit: I would indent 4 from line beginning rather than the paren. http://codereview.chromium.org/650180/diff/12027/15004#newcode103 chrome/browser/content_setting_image_model.cc:103: if (content_settings_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { Nit: No need for {}. http://codereview.chromium.org/650180/diff/12027/15005 File chrome/browser/geolocation/geolocation_browsertest.cc (right): http://codereview.chromium.org/650180/diff/12027/15005#newcode293 chrome/browser/geolocation/geolocation_browsertest.cc:293: // callback is triggered. What? This isn't good behavior. We shouldn't just silently do nothing when the user is OTR. We should behave like we normally do. This is something we discussed for the other content settings already. http://codereview.chromium.org/650180/diff/12027/15006 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/12027/15006#newcode101 chrome/browser/geolocation/geolocation_permission_context.cc:101: RequestPermissionFromUI( Nit: In cases like this, you should fit as many args on the first line as possible, then align subsequent lines with the first arg if possible. http://codereview.chromium.org/650180/diff/12027/15007 File chrome/browser/geolocation/geolocation_permission_context.h (right): http://codereview.chromium.org/650180/diff/12027/15007#newcode1 chrome/browser/geolocation/geolocation_permission_context.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. I assume that if we go with my simplified UI, this entire class can disappear? Because this is crazy complex. http://codereview.chromium.org/650180/diff/12027/15012 File chrome/browser/gtk/options/geolocation_filter_page_gtk.cc (right): http://codereview.chromium.org/650180/diff/12027/15012#newcode33 chrome/browser/gtk/options/geolocation_filter_page_gtk.cc:33: ask_radio_ = gtk_radio_button_new_with_label(NULL, Please add an "allow" radio, as I requested in our video chat. http://codereview.chromium.org/650180/diff/12027/15017 File chrome/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/650180/diff/12027/15017#newcode298 chrome/browser/renderer_host/render_view_host_delegate.h:298: // Called when geolocation in a frame on the current page was blocked due to Nit: The comment and the function name look like they're out of sync. http://codereview.chromium.org/650180/diff/12027/15020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/12027/15020#newcode245 chrome/browser/tab_contents/tab_contents.h:245: const std::map<std::string, ContentSetting>& Nit: Use a typedef http://codereview.chromium.org/650180/diff/12027/15020#newcode1211 chrome/browser/tab_contents/tab_contents.h:1211: // Maps all frames on this page to its geolocation content settings. Nit: "each frame"? http://codereview.chromium.org/650180/diff/12027/15024 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/12027/15024#newcode61 chrome/browser/views/options/geolocation_filter_page_view.cc:61: ask_radio_ = new views::RadioButton( Add an "allow" button here http://codereview.chromium.org/650180/diff/12027/15024#newcode111 chrome/browser/views/options/geolocation_filter_page_view.cc:111: if (sender == ask_radio_) { Nit: no {}
Hi Peter, Brett, I addressed all your comments so far. One thing that I'd like to note is that the size of this change shouldn't influence the UI decision, but rather the other way around: what is the best UX we can provide? suppose we *are* going with this design, is there anything else this change could do to simplify the current infrastructure? I already did some changes in preparation for this one, and I'm fine to do more if you have any suggestions. Anyways, I'm happy either way: should you have further comments here, I'll address them; if we decide to go on a completely different way, I'll do in a new change.
http://codereview.chromium.org/650180/diff/12027/15001 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/650180/diff/12027/15001#newcode6724 chrome/app/generated_resources.grd:6724: </ph> wants to track your physical location On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: It's a little weird that the phrasing here is subtlely different than below oh, good point! changed the other places s/current location/physical location/. http://codereview.chromium.org/650180/diff/12027/15001#newcode6733 chrome/app/generated_resources.grd:6733: This site <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph> is tracking your current location. On 2010/03/09 21:56:24, Peter Kasting wrote: > This isn't grammatical. Either add commas, say "This site is tracking your > current location." (my preference), or "www.google.com is tracking your > location." Even better might be "This site can track" or "This site can > access". changed to: www.google.com is tracking your physical location. http://codereview.chromium.org/650180/diff/12027/15001#newcode6735 chrome/app/generated_resources.grd:6735: <message name="IDS_GEOLOCATION_MULTIPLE_ALLOWED_TOOLTIP" desc="Tooltip text when a page is allowed to use geolocation."> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Give a description which distinguishes this from the above case. Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6738 chrome/app/generated_resources.grd:6738: <message name="IDS_GEOLOCATION_BLOCKED_TOOLTIP" desc="Tooltip text when a page is blocked to use geolocation."> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Description not grammatical; try "blocked from using" or "not allowed to > use". Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6739 chrome/app/generated_resources.grd:6739: This site <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph> is blocked from tracking your current location. On 2010/03/09 21:56:24, Peter Kasting wrote: > Same comment as in the allowed case; here I suggest "This site is blocked from > tracking your current location." as above, $1 is blocked ... http://codereview.chromium.org/650180/diff/12027/15001#newcode6741 chrome/app/generated_resources.grd:6741: <message name="IDS_GEOLOCATION_MULTIPLE_BLOCKED_TOOLTIP" desc="Tooltip text when a page is blocked to use geolocation."> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Same comments as above Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6744 chrome/app/generated_resources.grd:6744: <message name="IDS_GEOLOCATION_RADIO_TITLE" desc="Label shown above the radio buttons asking the user to set geolocation permission for this site."> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: These subsequent strings should perhaps go in the "Content blocking > strings" section? Done, however I kept the "IDS_GEOLOCATION_*" prefix rather than "IDS_BLOCKED_*", let me know if it's ok. http://codereview.chromium.org/650180/diff/12027/15001#newcode6745 chrome/app/generated_resources.grd:6745: This site, <ph name="PAGE_URL">$1<ex>www.google.com</ex></ph>, wants to track your current location. On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: I think it would be clearer to just say "This site wants" (my preference) > or "www.google.com wants". Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6751 chrome/app/generated_resources.grd:6751: Don't let this site track me On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: For consistency with our other blocking UI you might want "Prevent this > site from tracking me". Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6756 chrome/app/generated_resources.grd:6756: <message name="IDS_GEOLOCATION_TAB_LABEL" desc="Label for Geolocation tab on Content Settings dialog"> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: These subsequent strings should perhaps go in the "Content Settings dialog > data" section? Done. http://codereview.chromium.org/650180/diff/12027/15001#newcode6766 chrome/app/generated_resources.grd:6766: Do not allow any site to track my current location On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Might want to remove "current" changed to "physical" http://codereview.chromium.org/650180/diff/12027/15002 File chrome/app/theme/theme_resources.grd (right): http://codereview.chromium.org/650180/diff/12027/15002#newcode325 chrome/app/theme/theme_resources.grd:325: <include name="IDR_GEOLOCATION_ALLOWED_LOCATIONBAR_ICON" file="geolocation_allowed_locationbar_icon.png" type="BINDATA" /> On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Maybe these should be located with, and named (on disk and as identifiers) > similar to, IDR_BLOCKED_COOKIES and friends? cookies don't have "allowed" equivalent. if you don't feel strongly about it I'd prefer to keep as it is, or do as a follow-up just with this renaming? http://codereview.chromium.org/650180/diff/12027/15003 File chrome/browser/content_setting_bubble_model.cc (right): http://codereview.chromium.org/650180/diff/12027/15003#newcode21 chrome/browser/content_setting_bubble_model.cc:21: ContentSettingTitleAndLinkModel(TabContents* tab_contents, Profile* profile, On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Wrong indent done (and split decl and impl for all classes here). http://codereview.chromium.org/650180/diff/12027/15003#newcode24 chrome/browser/content_setting_bubble_model.cc:24: SetTitle(); On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Wrong indent (2 lines) Done. http://codereview.chromium.org/650180/diff/12027/15003#newcode29 chrome/browser/content_setting_bubble_model.cc:29: void SetTitle() { On 2010/03/09 21:56:24, Peter Kasting wrote: > Two nits: > * Personally, I'm not sure it really buys you much to declare these separate > functions; I'd put the code in the constructor. I don't mind them, though, if > you think it's more readable this way. I have no strong feelings, it just names the steps slightly better than one long ctor.. happy to go either way though. > * When you write the function definition in the class declaration, the compiler > assumes you want it inlined everywhere. That's not really the case in this > file, so I suggest splitting your declarations and definitions apart. (I think > this is a little more readable too, in the sense that you can look at the class > declarations and see quickly what functional areas they cover.) well, the compiler wouldn't be able to inline these methods since they're declared in the cc and not visible outside. I did this way for conciseness rather than optimization, but since you think it's more readable I split the decl and impl everywhere. http://codereview.chromium.org/650180/diff/12027/15003#newcode165 chrome/browser/content_setting_bubble_model.cc:165: const std::map<std::string, ContentSetting>& settings = On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: It would be nice to have typedefs for these, presumably declared in > TabContents.h or somewhere? Done. http://codereview.chromium.org/650180/diff/12027/15003#newcode201 chrome/browser/content_setting_bubble_model.cc:201: return new ContentSettingPopupBubbleModel(tab_contents, profile, On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: You can probably omit the content_type here like you do for geolocation. Done. http://codereview.chromium.org/650180/diff/12027/15003#newcode204 chrome/browser/content_setting_bubble_model.cc:204: if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: No need for {} Done. http://codereview.chromium.org/650180/diff/12027/15004 File chrome/browser/content_setting_image_model.cc (right): http://codereview.chromium.org/650180/diff/12027/15004#newcode14 chrome/browser/content_setting_image_model.cc:14: explicit ContentSettingBlockedImageModel( On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: See comment about definitions-in-declarations in > content_setting_bubble_model.cc. Done. http://codereview.chromium.org/650180/diff/12027/15004#newcode63 chrome/browser/content_setting_image_model.cc:63: const std::map<std::string, ContentSetting>& settings = On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: See comment about typedef in content_setting_bubble_model.cc. Done. http://codereview.chromium.org/650180/diff/12027/15004#newcode65 chrome/browser/content_setting_image_model.cc:65: if (settings.size() == 0) { On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Use settings.empty() Done. http://codereview.chromium.org/650180/diff/12027/15004#newcode72 chrome/browser/content_setting_image_model.cc:72: IDR_GEOLOCATION_DENIED_LOCATIONBAR_ICON : On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: I would indent 4 from line beginning rather than the paren. Done. http://codereview.chromium.org/650180/diff/12027/15004#newcode103 chrome/browser/content_setting_image_model.cc:103: if (content_settings_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) { On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: No need for {}. Done. http://codereview.chromium.org/650180/diff/12027/15005 File chrome/browser/geolocation/geolocation_browsertest.cc (right): http://codereview.chromium.org/650180/diff/12027/15005#newcode293 chrome/browser/geolocation/geolocation_browsertest.cc:293: // callback is triggered. On 2010/03/09 21:56:24, Peter Kasting wrote: > What? This isn't good behavior. We shouldn't just silently do nothing when the > user is OTR. We should behave like we normally do. This is something we > discussed for the other content settings already. not sure I understood: I changed this test to reflect the HostContentSettingsMap persistence, that is: 1. go to example.com 2. allow geolocation. 3. go OTR, and go to example.com: geolocation should be allowed. afaict, this is the same behavior for images and other content type? http://codereview.chromium.org/650180/diff/12027/15006 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/12027/15006#newcode101 chrome/browser/geolocation/geolocation_permission_context.cc:101: RequestPermissionFromUI( On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: In cases like this, you should fit as many args on the first line as > possible, then align subsequent lines with the first arg if possible. got it, I think I finally see my confusion here, please correct me if I'm wrong: + declaration: either all in one line, or break right after parens + call site: as many args as it fits on the first line is this correct? fixed the other places with this in mind. http://codereview.chromium.org/650180/diff/12027/15007 File chrome/browser/geolocation/geolocation_permission_context.h (right): http://codereview.chromium.org/650180/diff/12027/15007#newcode1 chrome/browser/geolocation/geolocation_permission_context.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/03/09 21:56:24, Peter Kasting wrote: > I assume that if we go with my simplified UI, this entire class can disappear? > Because this is crazy complex. I don't think so: something would still need to check the persisted hostcontentsettingsmap, decide whether or not to put up a confirmation infobar, notify back the geolocation service, etc.. (unless of course we go for an entire re-design and get rid of even the confirmation info bar, but I don't think this is possible..) http://codereview.chromium.org/650180/diff/12027/15012 File chrome/browser/gtk/options/geolocation_filter_page_gtk.cc (right): http://codereview.chromium.org/650180/diff/12027/15012#newcode33 chrome/browser/gtk/options/geolocation_filter_page_gtk.cc:33: ask_radio_ = gtk_radio_button_new_with_label(NULL, On 2010/03/09 21:56:24, Peter Kasting wrote: > Please add an "allow" radio, as I requested in our video chat. Done. http://codereview.chromium.org/650180/diff/12027/15017 File chrome/browser/renderer_host/render_view_host_delegate.h (right): http://codereview.chromium.org/650180/diff/12027/15017#newcode298 chrome/browser/renderer_host/render_view_host_delegate.h:298: // Called when geolocation in a frame on the current page was blocked due to On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: The comment and the function name look like they're out of sync. Done. http://codereview.chromium.org/650180/diff/12027/15020 File chrome/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/650180/diff/12027/15020#newcode245 chrome/browser/tab_contents/tab_contents.h:245: const std::map<std::string, ContentSetting>& On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: Use a typedef Done. http://codereview.chromium.org/650180/diff/12027/15020#newcode246 chrome/browser/tab_contents/tab_contents.h:246: get_geolocation_content_settings() const; On 2010/03/09 20:42:18, brettw wrote: > getter syntax doesn't include "get_", and must be inlined if you're using > unix_hacker formatting (being named like a variable implies it's as efficient as > a variable, which isn't the case unless you're inlined). If you can't inline, > use CamelCase with "Get" removed get, inlined. http://codereview.chromium.org/650180/diff/12027/15020#newcode673 chrome/browser/tab_contents/tab_contents.h:673: virtual void OnGeolocationPermissionSet( On 2010/03/09 20:42:18, brettw wrote: > This should go in the RenderViewHostDelegate implementation section. Done. http://codereview.chromium.org/650180/diff/12027/15020#newcode674 chrome/browser/tab_contents/tab_contents.h:674: const std::string& host, bool allowed); On 2010/03/09 20:42:18, brettw wrote: > Only indent to 4 spaces when you can't align to the ( on the previous line, so > this should have "host" on the previous line, and "allowed" right below it. Be > sure to check the other places you declare this function as well. Done. http://codereview.chromium.org/650180/diff/12027/15020#newcode1211 chrome/browser/tab_contents/tab_contents.h:1211: // Maps all frames on this page to its geolocation content settings. On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: "each frame"? Done. http://codereview.chromium.org/650180/diff/12027/15024 File chrome/browser/views/options/geolocation_filter_page_view.cc (right): http://codereview.chromium.org/650180/diff/12027/15024#newcode61 chrome/browser/views/options/geolocation_filter_page_view.cc:61: ask_radio_ = new views::RadioButton( On 2010/03/09 21:56:24, Peter Kasting wrote: > Add an "allow" button here Done. http://codereview.chromium.org/650180/diff/12027/15024#newcode111 chrome/browser/views/options/geolocation_filter_page_view.cc:111: if (sender == ask_radio_) { On 2010/03/09 21:56:24, Peter Kasting wrote: > Nit: no {} Done.
Since UI is still up in the air it's hard to move forward, but here's some replies. Note that if my most recent UI proposal is accepted, we won't be creating a geolocation bubble at all, just the icon. OTOH maybe everyone will still hate it... http://codereview.chromium.org/650180/diff/12027/15002 File chrome/app/theme/theme_resources.grd (right): http://codereview.chromium.org/650180/diff/12027/15002#newcode325 chrome/app/theme/theme_resources.grd:325: <include name="IDR_GEOLOCATION_ALLOWED_LOCATIONBAR_ICON" file="geolocation_allowed_locationbar_icon.png" type="BINDATA" /> On 2010/03/10 15:33:25, bulach wrote: > On 2010/03/09 21:56:24, Peter Kasting wrote: > > Nit: Maybe these should be located with, and named (on disk and as > identifiers) > > similar to, IDR_BLOCKED_COOKIES and friends? > > cookies don't have "allowed" equivalent. if you don't feel strongly about it I'd > prefer to keep as it is, or do as a follow-up just with this renaming? I don't feel terribly strongly, it just seems weirder to me to have 5 types of content settings icons with one name scheme and one with another than it does to have six types of icons with the same names, only one of which has an "allowed" partner... http://codereview.chromium.org/650180/diff/12027/15005 File chrome/browser/geolocation/geolocation_browsertest.cc (right): http://codereview.chromium.org/650180/diff/12027/15005#newcode293 chrome/browser/geolocation/geolocation_browsertest.cc:293: // callback is triggered. On 2010/03/10 15:33:25, bulach wrote: > On 2010/03/09 21:56:24, Peter Kasting wrote: > > What? This isn't good behavior. We shouldn't just silently do nothing when > the > > user is OTR. We should behave like we normally do. This is something we > > discussed for the other content settings already. > > not sure I understood: I changed this test to reflect the HostContentSettingsMap > persistence, that is: > 1. go to http://example.com > 2. allow geolocation. > 3. go OTR, and go to example.com: geolocation should be allowed. > > afaict, this is the same behavior for images and other content type? Maybe I'm confused. All I want to make sure of is that, when you visit a site wanting geolocation data (that you have not yet Allowed) while OTR, we prompt you like we would when not OTR, and persist the setting if you choose that. As long as that happens I'm happy. http://codereview.chromium.org/650180/diff/12027/15006 File chrome/browser/geolocation/geolocation_permission_context.cc (right): http://codereview.chromium.org/650180/diff/12027/15006#newcode101 chrome/browser/geolocation/geolocation_permission_context.cc:101: RequestPermissionFromUI( On 2010/03/10 15:33:25, bulach wrote: > On 2010/03/09 21:56:24, Peter Kasting wrote: > > Nit: In cases like this, you should fit as many args on the first line as > > possible, then align subsequent lines with the first arg if possible. > > got it, I think I finally see my confusion here, please correct me if I'm wrong: > + declaration: either all in one line, or break right after parens > + call site: as many args as it fits on the first line > > is this correct? fixed the other places with this in mind. I do: "Declaration: All in one line if possible; else first arg on same line, other args aligned one per line if possible; else break after paren, all args aligned, one per line, 4-space indented." Other people on the team have declarations where they have multiple args on each line on multiple lines, which I don't think is preferred style, but we've apparently decided is also OK to do. Call sites I try to fit multiple args on each line, every line aligned with the first arg if possible, but if this isn't possible or takes too many lines I use various other kinds of indents. We're not too picky about these normally, even with nits, I think I was just spurred by one of Brett's comments. http://codereview.chromium.org/650180/diff/12027/15007 File chrome/browser/geolocation/geolocation_permission_context.h (right): http://codereview.chromium.org/650180/diff/12027/15007#newcode1 chrome/browser/geolocation/geolocation_permission_context.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/03/10 15:33:25, bulach wrote: > On 2010/03/09 21:56:24, Peter Kasting wrote: > > I assume that if we go with my simplified UI, this entire class can disappear? > > > Because this is crazy complex. > > I don't think so: something would still need to check the persisted > hostcontentsettingsmap, decide whether or not to put up a confirmation infobar, > notify back the geolocation service, etc.. > (unless of course we go for an entire re-design and get rid of even the > confirmation info bar, but I don't think this is possible..) I changed my UI proposal on the thread. Let's assume we're keeping this. I would make sure you run this particular class and the semantics of what it touches past darin and eroman; they'll find any subtle problems with it, which I'm not qualified to detect. |
