|
|
Created:
7 years, 2 months ago by robliao Modified:
7 years, 2 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdjust Settings UI for Chrome Now's Geolocation Source
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228326
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add TODO #Messages
Total messages: 16 (0 generated)
lgtm
jhawkins: Please provide owner approval for this CL. Thanks!
lgtm
Ping!
The bug you linked is very nebulous, so I have no idea how this CL relates to some feature request.
On 2013/10/09 18:05:47, James Hawkins wrote: > The bug you linked is very nebulous, so I have no idea how this CL relates to > some feature request. This is a feature change request as part of Google Now for Chrome (Chrome Now). We added the checkbox as part of initial plans to use client geolocation and bound the checkbox to Chrome Now. Now, Chrome Now may ship without client geolocation and we're removing entry points to run a larger experiment without client geolocation. Removing the show code to the checkbox is part of this. Once we conclude that client geolocation is no longer needed, we'll be sending through another CL that removes the checkbox as well as logic to show and hide it.
https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:27: GeolocationOptions.showGeolocationOption = function() {}; This file looks like dead code now. What am I missing?
https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/geolocation_options.js (right): https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/geolocation_options.js:27: GeolocationOptions.showGeolocationOption = function() {}; This is a transitional step as we get the experiment going. For all intents and purposes, yes, this code does nothing. At this point in time, we aren't ready to remove the infrastructure around this code, but at the same time, we don't want this checkbox to show up either. We expect to remove this code and surrounding infrastructure once our team is comfortable with the results of a experiment we'll be running soon. On 2013/10/11 17:54:53, James Hawkins wrote: > This file looks like dead code now. What am I missing?
On 2013/10/11 17:58:12, Robert Liao wrote: > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/geolocation_options.js (right): > > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/geolocation_options.js:27: > GeolocationOptions.showGeolocationOption = function() {}; > This is a transitional step as we get the experiment going. For all intents and > purposes, yes, this code does nothing. > > At this point in time, we aren't ready to remove the infrastructure around this > code, but at the same time, we don't want this checkbox to show up either. > > We expect to remove this code and surrounding infrastructure once our team is > comfortable with the results of a experiment we'll be running soon. > > On 2013/10/11 17:54:53, James Hawkins wrote: > > This file looks like dead code now. What am I missing? What do you see as the cost to removing this? If you ever want it back, it's in git history.
On 2013/10/11 18:01:26, James Hawkins wrote: > On 2013/10/11 17:58:12, Robert Liao wrote: > > > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > > File chrome/browser/resources/options/geolocation_options.js (right): > > > > > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > > chrome/browser/resources/options/geolocation_options.js:27: > > GeolocationOptions.showGeolocationOption = function() {}; > > This is a transitional step as we get the experiment going. For all intents > and > > purposes, yes, this code does nothing. > > > > At this point in time, we aren't ready to remove the infrastructure around > this > > code, but at the same time, we don't want this checkbox to show up either. > > > > We expect to remove this code and surrounding infrastructure once our team is > > comfortable with the results of a experiment we'll be running soon. > > > > On 2013/10/11 17:54:53, James Hawkins wrote: > > > This file looks like dead code now. What am I missing? > > What do you see as the cost to removing this? If you ever want it back, it's in > git history. Quite a bit actually: The best unroll would include most 205841 (http://src.chromium.org/viewvc/chrome?view=revision&revision=205841) as well as 206190 (http://src.chromium.org/viewvc/chrome?view=revision&revision=206190). Once we are sure we don't want this feature, I will move to unrolling these CLs. Until then, there's quite a bit over overhead involved in removing the code, and then readding it back in if we decide that we want it.
On 2013/10/11 18:07:34, Robert Liao wrote: > On 2013/10/11 18:01:26, James Hawkins wrote: > > On 2013/10/11 17:58:12, Robert Liao wrote: > > > > > > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > > > File chrome/browser/resources/options/geolocation_options.js (right): > > > > > > > > > https://codereview.chromium.org/26315007/diff/1/chrome/browser/resources/opti... > > > chrome/browser/resources/options/geolocation_options.js:27: > > > GeolocationOptions.showGeolocationOption = function() {}; > > > This is a transitional step as we get the experiment going. For all intents > > and > > > purposes, yes, this code does nothing. > > > > > > At this point in time, we aren't ready to remove the infrastructure around > > this > > > code, but at the same time, we don't want this checkbox to show up either. > > > > > > We expect to remove this code and surrounding infrastructure once our team > is > > > comfortable with the results of a experiment we'll be running soon. > > > > > > On 2013/10/11 17:54:53, James Hawkins wrote: > > > > This file looks like dead code now. What am I missing? > > > > What do you see as the cost to removing this? If you ever want it back, it's > in > > git history. > > Quite a bit actually: > The best unroll would include most 205841 > (http://src.chromium.org/viewvc/chrome?view=revision&revision=205841) > as well as 206190 > (http://src.chromium.org/viewvc/chrome?view=revision&revision=206190). > > Once we are sure we don't want this feature, I will move to unrolling these CLs. > Until then, there's quite a bit over overhead involved in removing the code, and > then readding it back in if we decide that we want it. I disagree, but I don't think we'll find common ground here. LGTM but please document the current status (discussed here) in the source file in question.
On 2013/10/11 18:42:35, Robert Liao wrote: Done!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/26315007/22001
Message was sent while issue was closed.
Change committed as 228326 |