Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10917120:
Activate the VariationsService for ChromeOS and ensure that it does not ping the server until the E… (Closed)
|
Created:
8 years, 3 months ago by SteveT Modified:
8 years, 2 months ago CC:
chromium-reviews, MAD, nkostylev+watch_chromium.org, kkania, robertshield, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, jar (doing other things), jwd Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionActivate the VariationsService for ChromeOS and ensure that it does not ping the server until the EULA is accepted.
We accomplish this by adding an OnEulaAccepted method to the WizardControllerObserver and have the VariationsService be notified on that method when the EULA is accepted.
BUG=146865
TEST=Start CrOS on a new machine and ensure that, with a network connection, no requests are made to the variations server before the EULA is accepted. As soon as the EULA is accepted, ensure that a request is made. Also ensure that a request is made each time at startup after the first time the EULA is accepted.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158648
Patch Set 1 : init #
Total comments: 5
Patch Set 2 : nikita nits #
Total comments: 16
Patch Set 3 : asvit nits #Patch Set 4 : missed asvit nit #
Total comments: 1
Patch Set 5 : Refactored to use ResourceRequestAllowedNotifier #
Total comments: 2
Patch Set 6 : removed automation provider changes #Patch Set 7 : RRAN now hides all notion of criteria #Patch Set 8 : unit tests refactored #
Total comments: 68
Patch Set 9 : asvit nit 2 #
Total comments: 10
Patch Set 10 : notification situation solution #
Total comments: 39
Patch Set 11 : Comments addressed. #
Total comments: 30
Patch Set 12 : Test fixups from asvit #
Total comments: 28
Patch Set 13 : Addressed latest asvit comments #Patch Set 14 : rebase #Patch Set 15 : nonchromeos fixes #
Total comments: 23
Patch Set 16 : more asvit nits #Patch Set 17 : Flip EULA and added test. #
Total comments: 10
Patch Set 18 : Additiona fixes #
Total comments: 2
Patch Set 19 : one more asvit nit #
Total comments: 4
Messages
Total messages: 70 (0 generated)
Nikita - mind taking an initial look at this? I'll add the other OWNERS approvers when we're through this first review. https://chromiumcodereview.appspot.com/10917120/diff/2001/chrome/browser/chro... File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/2001/chrome/browser/chro... chrome/browser/chromeos/login/base_login_display_host.cc:32: #include "chrome/browser/metrics/variations/variations_service.h" Note that we add this header here because otherwise the AddObserver call below does not know that g_browser_process->variations_service() returns something that inherits from WizardController::Observer due to the way everything is forward declared in chrome_browser_process.h.
24 hr friendly ping :)
lgtm sorry for delay, was ooo on Friday. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1430: // call FetchLanguageListFromTranslateServer. nit: Comment should be updated so that it doesn't mention only translate service. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:32: #include "chrome/browser/chromeos/login/wizard_controller.h" nit: Should be removed as already included in the header.
Thanks for the look, Nikita. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1430: // call FetchLanguageListFromTranslateServer. Ah you're right. Should have left it the way it was before. Reverted. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:32: #include "chrome/browser/chromeos/login/wizard_controller.h" On 2012/09/10 14:48:55, Nikita Kostylev wrote: > nit: Should be removed as already included in the header. Done.
Thanks for the look, Nikita. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1430: // call FetchLanguageListFromTranslateServer. Ah you're right. Should have left it the way it was before. Reverted. http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/2001/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:32: #include "chrome/browser/chromeos/login/wizard_controller.h" On 2012/09/10 14:48:55, Nikita Kostylev wrote: > nit: Should be removed as already included in the header. Done.
R+Alexei, cc+Jesse PTAL at the VariationsService-related changes.
http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { I feel like it would better to have a helper class that abstracts both network availability and ChromeOS eula acceptance. Then, this can just call AreNetworkRequestAllowed() on an instance of that class and should be notified when that setting changes to the correct state. That helper class would itself observe the CrOS eula and the network adapter state and provide its own simple observer interface. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:172: // accepted. What about the case of a second start up when there's no wizard? How does that work? Can you expand the comment to explain this. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:199: void VariationsService::OnScreenChanged(chromeos::WizardScreen* next_screen) { Don't indent these. Please add a comment to the no-op methods to explain we only implement them to follow the Observer interface. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.h:66: virtual void OnSessionStart() OVERRIDE; Remove empty lines between these functions.
Responses inline. PTAL. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { So you're suggesting that I extract the Observer bits in VariationsService to better decouple it from that logic? It still needs to be notified by this new VSEventNotifier class, so we don't clean up all those relationships. Is there an example of another class somewhere that used to directly observe such notifications, but was refactored to delegate that to a helper class? I am happy to make this change, but I want to better understand the benefits of creating this new class. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:172: // accepted. Comment added below to explain how we NO-OP if the Eula was accepted already. http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:199: void VariationsService::OnScreenChanged(chromeos::WizardScreen* next_screen) { Done and done. Let me know if the comments are fine.
Some drive by comments, since something similar will need to be done for the translate manager... I find it a little sad to add ChromeOS specific code to the service and to add explicit knowledge of the service to the chrome os code... I wish we could find a more abstract way to deal with this... https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1441: // Request new variations seed information from server. This is not within "if (parameters().ui_task == NULL)" It is on purpose? https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { On 2012/09/10 17:25:28, SteveT wrote: > So you're suggesting that I extract the Observer bits in VariationsService to > better decouple it from that logic? It still needs to be notified by this new > VSEventNotifier class, so we don't clean up all those relationships. > > Is there an example of another class somewhere that used to directly observe > such notifications, but was refactored to delegate that to a helper class? > > I am happy to make this change, but I want to better understand the benefits of > creating this new class. Maybe we could share code with the translate manager?
http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/8003/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { On 2012/09/10 17:25:28, SteveT wrote: > So you're suggesting that I extract the Observer bits in VariationsService to > better decouple it from that logic? It still needs to be notified by this new > VSEventNotifier class, so we don't clean up all those relationships. > > Is there an example of another class somewhere that used to directly observe > such notifications, but was refactored to delegate that to a helper class? > > I am happy to make this change, but I want to better understand the benefits of > creating this new class. The benefits is that variations_service will not have ChromeOS specific code and will only need to act on one type of state and one type of notification instead of two. Additionally, this helper class could be re-used by the translate manager.
Okay cool. Responses inline for MAD. Thanks for the discussion, Alexei. I'll ping this when my new patchset is ready. https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1441: // Request new variations seed information from server. It still is. Perhaps it'd be more clear if I moved it up before the CrOS IFDEF? https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { I am convinced by these arguments and will do the work to extract that logic into this new ResourceRequestChecks helper class (or some better, shorter name). https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.h (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.h:66: virtual void OnSessionStart() OVERRIDE; On 2012/09/10 16:50:11, Alexei Svitkine wrote: > Remove empty lines between these functions. Done.
https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1441: // Request new variations seed information from server. On 2012/09/10 19:33:52, SteveT wrote: > It still is. Perhaps it'd be more clear if I moved it up before the CrOS IFDEF? D'Ho! :-) Yeah, maybe it would be better... Sorry about that...
https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { > ResourceRequestChecks helper class (or some better, shorter name). Maybe NetworkAllowedNotifier or something variation thereof? Since the class basically tells us whether or not we're allowed to use the network / make requests and would do so through a notification interface.
https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/8003/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.cc:164: if (was_offline_during_last_request_attempt_) { On 2012/09/10 21:47:35, Alexei Svitkine wrote: > > ResourceRequestChecks helper class (or some better, shorter name). > > Maybe NetworkAllowedNotifier or something variation thereof? Since the class > basically tells us whether or not we're allowed to use the network / make > requests and would do so through a notification interface. It's a bit more like a ResourceRequestAllowedNotifier, since it's not just the network, but also the EULA permission that needs to be set before we are allowed to make requests for our resources.
Alexei - PTAL at the line pointed out below, as part of our offline discussions. http://codereview.chromium.org/10917120/diff/7003/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/7003/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:202: wizard_controller_->AddObserver(g_browser_process->variations_service()); At this point in the code, we know the wizard_controller_ is accessible, so we have to add an Observers here. This means that our new ResourceRequestAllowedNotifier needs to hang off g_browser_process in order for us to add it here (unless there is some sort of global instance). Thoughts?
Refactoring done. Alexei, Nikita, and MAD, PTAL :) BTW it looks like this didn't clean out the OS_CHROMEOS stuff out of variations_service like we were hoping. Let me know if you see a way to do that that I might have missed. Thanks, all! Steve
http://codereview.chromium.org/10917120/diff/5004/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/5004/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.h:59: virtual void OnEulaAccepted() OVERRIDE; I was suggesting that ResourceRequestAllowedNotifier would provide only a single type of notification, i.e. OnResourceRequestAllowedChanged(bool allowed). Internally, it would manage the state of both the eula and the network connection, but users of it (i.e. varations service) would only need to know about whether it's okay or not to make the connection, not the implementation details of why it's okay or not (which should be entirely in ResourceRequestAllowedNotifier). In that case, "bool waiting_for_user_to_accept_eula_" would be an implementation detail of ResourceRequestAllowedNotifier.
Okay, more changes coming. http://codereview.chromium.org/10917120/diff/5004/chrome/browser/metrics/vari... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/5004/chrome/browser/metrics/vari... chrome/browser/metrics/variations/variations_service.h:59: virtual void OnEulaAccepted() OVERRIDE; Hm, okay. That could work, but will take some additional finessing to ensure that the network and eula state plays well together. Plus I'll have to move the unit tests around network state to resource_request_allowed_notifier_unittest.cc somehow.
Alexei: Okay, I think this is a bit closer to what you want. If this is looking OK, I will complete the rest of the work to migrate the unit tests over to resource_request_allowed_notifier_unittests.cc and also test the Eula state.
Yes, this is what I was looking for. :) Do you want me to review it now or wait till you update the tests?
On 2012/09/11 21:16:03, Alexei Svitkine wrote: > Yes, this is what I was looking for. :) > > Do you want me to review it now or wait till you update the tests? I'll take care of the tests first before we get carried away. Will ping this when ready.
Okay - unittests are refactored, with a few new ones. Nikita, Alexei, and MAD, PTAL!
Drive-by with nits, no need to block on my review. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... chrome/browser/chromeos/login/wizard_controller.cc:448: content::NotificationService::AllSources(), nit: Please specify a source rather than using AllSources() https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/met... File chrome/browser/metrics/variations/variations_service.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/met... chrome/browser/metrics/variations/variations_service.cc:181: VLOG(1) << "Retrying fetch."; nit: DVLOG? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/met... File chrome/browser/metrics/variations/variations_service.h (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/met... chrome/browser/metrics/variations/variations_service.h:57: virtual void OnResourceRequestsAllowed() OVERRIDE; nit: Can this have private visibility? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... File chrome/browser/resource_request_allowed_notifier.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.cc:23: content::NotificationService::AllSources()); nit: Listening to notifications from AllSources() is discouraged. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.cc:32: content::NotificationService::AllSources()); nit: Listening to notifications from AllSources() is discouraged. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.cc:37: DCHECK(observer_list_.size() == 0) << "One service per notifier."; nit: Please drop the """<< "One service per notifier."""" -- you can add it as a comment instead, but printing from DCHECKs is strongly discouraged. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.cc:43: DCHECK(observer_list_.size() == 0) << "One service per notifier."; nit: Please drop the logged line. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.cc:74: DVLOG(1) << "EULA was accepted."; Is this DVLOG just to help you while developing, or do you expect it to be useful longer term? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... File chrome/browser/resource_request_allowed_notifier.h (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.h:6: #define CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ nit: Is the variations service currently the only client of this header's contents? If so, it should probably be moved to chrome/browser/metrics/variations. It can always be extracted back up to a higher level if it proves useful for other clients. https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.h:64: const content::NotificationDetails& details) OVERRIDE; nit: Can this have private visibility? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.h:69: net::NetworkChangeNotifier::ConnectionType type) OVERRIDE; nit: Can this have private visibility? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.h:83: bool was_waiting_for_user_to_accept_eula_; nit: Should this also be in the #if defined(OS_CHROMEOS) conditional? https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/res... chrome/browser/resource_request_allowed_notifier.h:88: content::NotificationRegistrar registrar_; nit: Ditto
http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:174: void VariationsService::SetWasOfflineDuringLastRequestAttemptForTesting( I don't see your tests calling this anymore. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:180: void VariationsService::OnResourceRequestsAllowed() { Does this guarantee that we won't fetch more than once every 5 hours if e.g. the network is flapping? We've had this discussion before and I think the answer may be "yes" due to how the code is structured. If that's the case, please document it both here as well as making sure that it's also clear in the comments for the new class (e.g. that it only notifies when the last "is allowed" returned false, but not otherwise.) http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:21: #include "net/base/network_change_notifier.h" Remove this #include now that you're not using it directly. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:459: Can you add a test that mocks the ResourceRequestAllowedNotifier to ensure that we do fetch in the scenario where initially the request was not allowed but then we were notified that it is allowed? http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:18: was_waiting_for_user_to_accept_eula_(false) { I think you want the OS_CHROMEOS ifdef over this line, not over the previous one. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:37: DCHECK(observer_list_.size() == 0) << "One service per notifier."; If you're enforcing this, why not just make it enforced by the API? e.g. SetObserver() instead of AddObserver()? And why store them in a list if there's only going to be one observer? Just use a regular single variable. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:49: !chromeos::WizardController::IsEulaAccepted(); Do we need to query this each time or can we just query this once in the constructor and update its value when we get the EULA notification? http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:55: UMA_HISTOGRAM_BOOLEAN("Variations.NetworkAvailability", Maybe keep this histogram in variations service and have it track "resource request allowability" now - i.e. network availability for both technical and eula reasons. Also, especially if this class gets used by multiple consumers, the histogram results may not be meaningful anymore. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:70: DCHECK(type == chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED); Can you use DCHECK_EQ() here? http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:72: // do not notify it. Wouldn't the condition you describe in this comment be unexpected? If so, perhaps we should DCHECK on it. Or is it possible for the state go from EULA accepted -> EULA unaccepted? http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:84: type != net::NetworkChangeNotifier::CONNECTION_NONE) { Can you add a comment explaining the if statement conditions? http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:96: // Need to ensure that all criteria is met before notifying observers. Nit: "is" -> "are". http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:97: if (ResourceRequestsAllowed()) { I think this is problematic per what mmenke said in a past review. He said that it may be possible for the network notification to be inconsistent with the net::NetworkChangeNotifier::IsOffline() state under certain circumstances on Windows. So there could be a scenario where we get a notification that net is up, but IsOffline() returns true. In such a case, this notification will never get sent. Perhaps the code could be restructured such that when we get notified of X changing, we don't re-check X since we should know it's value from the notification? This would solve the above problem. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:6: #define CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: Is the variations service currently the only client of this header's > contents? If so, it should probably be moved to > chrome/browser/metrics/variations. It can always be extracted back up to a > higher level if it proves useful for other clients. I agree with Ilya here. Let's add it under chrome/browser/metrics/variations for now. It can be moved later when MAD makes the translate manager use it. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:16: // Services that need to request resources over the network should use this Can you rephrase the first sentence and/or paragraph to clearly state what problem this class is solving (i.e. "should use this class" doesn't explain "why"). Mention up-front (instead of further below) that on ChromeOS resource requests are not allowed until the EULA is accepted, etc. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:77: #if defined(OS_CHROMEOS) I think this ifdef was meant to go around the next variable, not this one. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:79: bool was_offline_during_last_request_attempt_; Can you name this consistently with |was_waiting_for_user_to_accept_eula_|. e.g. both refer to the state during the last ResourceRequestsAllowed(), but only this variable has suffix "during_last_request_attempt_". Adjust one or both names so that they're consistent. (The prefix may not be needed if you keep the "was_" suffix and document the specifics in the comments above the variables.) http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:5: #include "chrome/browser/resource_request_allowed_notifier.h" This is an anti-pattern. The style guide requires that the header file for an implementation file be included as the first file, so it doesn't apply here unless you had a resource_request_allowed_notifier_unittest.h file. (It's an anti-pattern because a lot of tests still do this unfortunately, but its incorrect.) http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:31: bool request_attempted() const { return request_attempted_; } I would call this "was_notified". http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:32: void SetRequestAttempted(bool attempted) { request_attempted_ = attempted; } This should use unix_hacker style since it's not doing more work than setting the variable.
lgtm chromeos code http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:72: // do not notify it. On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Wouldn't the condition you describe in this comment be unexpected? If so, > perhaps we should DCHECK on it. Or is it possible for the state go from EULA > accepted -> EULA unaccepted? No, it is not possible.
Sending this comment off to Ilya as I will be busy on another CL for a day before I get to this... Steve https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... chrome/browser/chromeos/login/wizard_controller.cc:448: content::NotificationService::AllSources(), I knew I would get in trouble for this :) - Unfortunately it is currently not easy on the receiving end to specify the specific WizardController that will be the Source of this Notification. The WizardController is created deep in the bowels of BaseLoginDisplayHost, where it is not easy for our ResourceRequestAllowedNotifier to access it. All existing listeners of this notification also listen on AllSources. What's the usual course of action for these situations? I can see if we can plumb the WizardController around, but it doesn't look clean.
https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... chrome/browser/chromeos/login/wizard_controller.cc:448: content::NotificationService::AllSources(), On 2012/09/12 17:56:10, SteveT wrote: > I knew I would get in trouble for this :) - Unfortunately it is currently not > easy on the receiving end to specify the specific WizardController that will be > the Source of this Notification. The WizardController is created deep in the > bowels of BaseLoginDisplayHost, where it is not easy for our > ResourceRequestAllowedNotifier to access it. > > All existing listeners of this notification also listen on AllSources. It looks like this is a new notification that you're adding, so I'm not sure what you mean about the existing listeners ;) > What's the usual course of action for these situations? I can see if we can > plumb the WizardController around, but it doesn't look clean. I think we should do the work to plumb this through, or else specify a different source (user account/profile?) that maps one-to-one to the controller. The second option is probably cleaner, if there's a suitable alternate source. The danger with using AllSources() here is that if ChromeOS ever supports multiple users being simultaneously logged in (and each having their own EULA?), this will start listening to notifications from *all* users, which seems wrong.
On 2012/09/12 20:30:01, Ilya Sherman wrote: > https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > https://chromiumcodereview.appspot.com/10917120/diff/11004/chrome/browser/chr... > chrome/browser/chromeos/login/wizard_controller.cc:448: > content::NotificationService::AllSources(), > On 2012/09/12 17:56:10, SteveT wrote: > > I knew I would get in trouble for this :) - Unfortunately it is currently not > > easy on the receiving end to specify the specific WizardController that will > be > > the Source of this Notification. The WizardController is created deep in the > > bowels of BaseLoginDisplayHost, where it is not easy for our > > ResourceRequestAllowedNotifier to access it. > > > > All existing listeners of this notification also listen on AllSources. > > It looks like this is a new notification that you're adding, so I'm not sure > what you mean about the existing listeners ;) My bad - One of the approaches we were considering earlier involved listening to an existing Notification, which used AllSources() and would involve the same plumbing to avoid that. > > > What's the usual course of action for these situations? I can see if we can > > plumb the WizardController around, but it doesn't look clean. > > I think we should do the work to plumb this through, or else specify a different > source (user account/profile?) that maps one-to-one to the controller. The > second option is probably cleaner, if there's a suitable alternate source. > > The danger with using AllSources() here is that if ChromeOS ever supports > multiple users being simultaneously logged in (and each having their own EULA?), > this will start listening to notifications from *all* users, which seems wrong. K, I will look into this.
Okay, I've addressed most of the comments. Please take a look at the tests again as they've received some significant changes after I've moved the EULA check to the ctor of the Notifier class. Thanks again. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/wizard_controller.cc:448: content::NotificationService::AllSources(), On 2012/09/12 20:30:01, Ilya Sherman wrote: > On 2012/09/12 17:56:10, SteveT wrote: > > I knew I would get in trouble for this :) - Unfortunately it is currently not > > easy on the receiving end to specify the specific WizardController that will > be > > the Source of this Notification. The WizardController is created deep in the > > bowels of BaseLoginDisplayHost, where it is not easy for our > > ResourceRequestAllowedNotifier to access it. > > > > All existing listeners of this notification also listen on AllSources. > > It looks like this is a new notification that you're adding, so I'm not sure > what you mean about the existing listeners ;) > > > What's the usual course of action for these situations? I can see if we can > > plumb the WizardController around, but it doesn't look clean. > > I think we should do the work to plumb this through, or else specify a different > source (user account/profile?) that maps one-to-one to the controller. The > second option is probably cleaner, if there's a suitable alternate source. > > The danger with using AllSources() here is that if ChromeOS ever supports > multiple users being simultaneously logged in (and each having their own EULA?), > this will start listening to notifications from *all* users, which seems wrong. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:174: void VariationsService::SetWasOfflineDuringLastRequestAttemptForTesting( Removed. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:180: void VariationsService::OnResourceRequestsAllowed() { Done here, and already done in the header of the RRAN class (see the 3rd paragraph). http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:181: VLOG(1) << "Retrying fetch."; On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: DVLOG? Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:21: #include "net/base/network_change_notifier.h" On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Remove this #include now that you're not using it directly. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:57: virtual void OnResourceRequestsAllowed() OVERRIDE; Yup! Done! http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:459: Not done yet... http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:18: was_waiting_for_user_to_accept_eula_(false) { D'oh. You can tell I haven't ran my Non-CrOS tests. Fixed. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:23: content::NotificationService::AllSources()); Did the work to resolve all this. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:32: content::NotificationService::AllSources()); On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: Listening to notifications from AllSources() is discouraged. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:37: DCHECK(observer_list_.size() == 0) << "One service per notifier."; Didn't realize that was the case. I'll drop the logs. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:37: DCHECK(observer_list_.size() == 0) << "One service per notifier."; No, you're totally right. I've changed this class to just have the one observer and changed the methods to set or clear it. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:43: DCHECK(observer_list_.size() == 0) << "One service per notifier."; On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: Please drop the logged line. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:49: !chromeos::WizardController::IsEulaAccepted(); I think your suggestion is reasonable, because once it gets set by the notification, that's it. It'll generally never go false again. Made those changes. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:55: UMA_HISTOGRAM_BOOLEAN("Variations.NetworkAvailability", I am considering just sunsetting, since we already got the data we wanted from it (approximately 2% network downtime). As for not pinging because the Eula has not been accepted... We sort of have an understanding of that event already... it'll get hit roughly once per new CrOS device activation (assuming the user accepts the Eula the first time and doesn't just reboot a lot). I've removed it for now. Let me know if you're opposed. I've noted that I should clean it from histograms.xml as well. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:70: DCHECK(type == chrome::NOTIFICATION_WIZARD_EULA_ACCEPTED); On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Can you use DCHECK_EQ() here? Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:74: DVLOG(1) << "EULA was accepted."; Well I expect it to be useful in the future as developers can quickly check to see when events could cause their service to start resource requests. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:84: type != net::NetworkChangeNotifier::CONNECTION_NONE) { On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Can you add a comment explaining the if statement conditions? Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:96: // Need to ensure that all criteria is met before notifying observers. On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Nit: "is" -> "are". Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.cc:97: if (ResourceRequestsAllowed()) { X being one of Eula-accepted or network-online? So you're suggesting that after the first time the user calls ResourceRequestAllowed(), the criteria flags can only flip from false to true? And here, instead of calling ResourceRequestsAllowed, which could reset the flags, I just check all flags directly? (note that I haven't had a fix for this yet) http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:6: #define CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ On 2012/09/12 15:15:46, Alexei Svitkine wrote: > On 2012/09/11 22:51:27, Ilya Sherman wrote: > > nit: Is the variations service currently the only client of this header's > > contents? If so, it should probably be moved to > > chrome/browser/metrics/variations. It can always be extracted back up to a > > higher level if it proves useful for other clients. > > I agree with Ilya here. Let's add it under chrome/browser/metrics/variations for > now. It can be moved later when MAD makes the translate manager use it. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:6: #define CHROME_BROWSER_RESOURCE_REQUEST_ALLOWED_NOTIFIER_H_ On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: Is the variations service currently the only client of this header's > contents? If so, it should probably be moved to > chrome/browser/metrics/variations. It can always be extracted back up to a > higher level if it proves useful for other clients. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:16: // Services that need to request resources over the network should use this On 2012/09/12 15:15:46, Alexei Svitkine wrote: > Can you rephrase the first sentence and/or paragraph to clearly state what > problem this class is solving (i.e. "should use this class" doesn't explain > "why"). > > Mention up-front (instead of further below) that on ChromeOS resource requests > are not allowed until the EULA is accepted, etc. Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:64: const content::NotificationDetails& details) OVERRIDE; Yes! Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:69: net::NetworkChangeNotifier::ConnectionType type) OVERRIDE; Yeah, done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:77: #if defined(OS_CHROMEOS) Yes, done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:79: bool was_offline_during_last_request_attempt_; Sure. was_waiting_for_network_ and was_waiting_for_user_to_accept_eula_ Let me know if you prefer otherwise. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:83: bool was_waiting_for_user_to_accept_eula_; Yes - thanks. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier.h:88: content::NotificationRegistrar registrar_; On 2012/09/11 22:51:27, Ilya Sherman wrote: > nit: Ditto Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... File chrome/browser/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:5: #include "chrome/browser/resource_request_allowed_notifier.h" Good to know! Fixed. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:31: bool request_attempted() const { return request_attempted_; } On 2012/09/12 15:15:46, Alexei Svitkine wrote: > I would call this "was_notified". Done. http://codereview.chromium.org/10917120/diff/11004/chrome/browser/resource_re... chrome/browser/resource_request_allowed_notifier_unittest.cc:32: void SetRequestAttempted(bool attempted) { request_attempted_ = attempted; } Didn't realize the style was called that. Also, turns out this isn't used, so I've gotten rid of it.
http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); I don't like this at all. I understand you're trying to solve the problem of using "all sources" for notifications, but this now introduces a lot of extra dependencies between things that shouldn't have dependencies. Wizard controller now has to know about variations service and variations service has to know about wizard controller. Additionally, any class using ResourceRequestAllowedNotifier would now have to do this sort of thing too. I understand that the problem is that it's not clear how to get at an instance of WizardController - and perhaps it may not even exist. However, I have another proposal of how to solve this problem. For the notification source, you just need a pointer to something. Let's add a static member variable to WizardController, let's say notification_source_token_. Then, we can have a static WizardController::GetNotificationSource() function that returns ¬ification_source_token_. ResourceRequestAllowedNotifier could then register for the notification using WizardController::GetNotificationSource() while VariationsService could be completely oblivious of the wizard controller and vice versa.
Over to Ilya with a question. http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); Hmm, this seems less hacky in the sense that the deps are cleaned up, but still hacky in the sense that we're abusing NotificationSource. Ilya, what do you think?
https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); Ick. It looks like the WizardController class provides a default_controller() method. Can we use that instead? Or, perhaps someone more knowledgeable with the ChromeOS structure can suggest a better option? https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/metr... File chrome/browser/metrics/variations/variations_service.h (right): https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/metr... chrome/browser/metrics/variations/variations_service.h:74: bool create_trials_from_seed_called_; nit: Member variables should always have private visibility. Please expose a setter instead.
https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); On 2012/09/14 20:55:43, Ilya Sherman wrote: > Ick. It looks like the WizardController class provides a default_controller() > method. Can we use that instead? It looks like that is set to NULL until the wizard actually gets created.
Response to our soon to be epic thread! https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... File chrome/browser/chromeos/login/base_login_display_host.cc (right): https://chromiumcodereview.appspot.com/10917120/diff/1016/chrome/browser/chro... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); That was my original approach WAAAAYY back. The issue is that default_controller() is NULL (the Wizard has not been created) at the time VariationsService is created. So either way, we'd have to make a call within the Wizard code. Nikita and I discussed possibly listening to this other Notification that fires when WizardController has been created, but that involves using AllSources ;)
http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); On 2012/09/14 20:52:45, SteveT wrote: > Hmm, this seems less hacky in the sense that the deps are cleaned up, but still > hacky in the sense that we're abusing NotificationSource. > > Ilya, what do you think? I didn't see Alexei's suggestion until after I had mailed my previous comment. This proposal sounds like it is entirely equivalent to using AllSources(), but just adds extra code to do so. (Admittedly, that's more or less true of my suggestion to use default_controller() as well :() It does have the advantage that anyone working on the WizardController class would be more likely to notice this and update the dependencies if we ever need to listen to more fine-grained notifications... I think it would be helpful for someone who is more familiar with the WizardController code to chime in. Right now, the class is structured to essentially be a singleton. Since we don't use singletons much in Chrome code, it's harder to get a feel for what the best way to integrate with notifications is. Here's another way to think about this issue: Who owns the WizardController, and the WizardController's owner, and the owner of that? At any point in this ownership chain, do we reach a concrete object that the VariationsService can also grasp? If so, perhaps that object should be treated as the source of the notification?
Potato over to Nikita. http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); That's a possibility, assuming that common object exists. The WizardController lives in the scary pre-first-Chrome-window land of CrOS. Nikita - any thoughts on this?
http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); On 2012/09/14 21:09:24, SteveT wrote: > That's a possibility, assuming that common object exists. The WizardController > lives in the scary pre-first-Chrome-window land of CrOS. > > Nikita - any thoughts on this? Instance of LoginDisplayHost/BaseLoginDisplayHost owns controller: wizard_controller_.reset(CreateWizardController()); There's a BaseLoginDisplayHost::default_host() but keep in mind that LoginDisplayHost instance that it returns only exists during login. I would actually step back a bit and list all possible entry points that VariationService should handle. 1. Normal boot on Chrome OS - boot into login screen. BaseLoginDisplayHost::default_host() does exist on boot but we're not going through OOBE so WizardController is not created too. Solution: Rely on WizardController::IsEulaAccepted static method that returns value of Local State pref. 2. OOBE boot. BaseLoginDisplayHost::default_host() does exist on boot, then instance of WizardController is created - these lines. That's the only case when EULA is not accepted on boot so you have to subscribe to notification / delegate call etc. 3. Restart after crash (inside user session). This goes through special code path because SessionManager passes --login-user=<...> switch meaning that session has already started. LoginDisplayHost instance is not created, same for WizardController. See code http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/chr... VariationService should use IsEulaAccepted() as in #1 case. Btw, this case is also used for guest mode which essentially is done as Chrome restart on login screen. As was discussed here in the comments I don't think that we ever plan to introduce multiple EULAs on ChromeOS meaning that each user would have to accept it. You should not target this case and assume there's only single global EULA accepted during out ouf box flow (OOBE).
To summarize, it seems like what we are doing the right thing by checking IsEulaAccepted first (long before any OOBE code runs) and only waiting on the Eula notification if that is FALSE. In fact, with this current patchset where we don't listen to AllSources, we don't even listen for the EULA_ACCEPTED notification unless the WizardController was created. One possible danger: Is it ever possible for IsEulaAccepted to initially return FALSE, and the WizardController is never created? If that's possible, then we may never tell our service to start. I also wonder about how we can test for all these cases. I believe I've manually tested the OOBE and "Normal" case, but not the crash case. Though I assume the crash case is the same as the Normal case (just check IsEulaAccepted at startup and never worry about the WizCtrl - which is implicit since IsEulaAccepted should return true).
BTW - Once the IsEulaAccepted starts returning true, there is nothing that will set it to false short of changing the value in the Local State file?
On 2012/09/17 13:12:03, SteveT wrote: > One possible danger: Is it ever possible for IsEulaAccepted to initially return > FALSE, and the WizardController is never created? If that's possible, then we > may never tell our service to start. IsEulaAccepted() will always return false on Chromium OS / non-official builds of Chrome OS. On official Chrome OS build EulaAccepted Local State pref is initialized with false and WizardController will be created on first boot. > I also wonder about how we can test for all these cases. I believe I've manually > tested the OOBE and "Normal" case, but not the crash case. Though I assume the > crash case is the same as the Normal case (just check IsEulaAccepted at startup > and never worry about the WizCtrl - which is implicit since IsEulaAccepted > should return true). You may create browser_tests for some test cases but I believe we don't have a browser_test that tests whole OOBE flow at once. If you want to test full flow (i.e. start with fresh EULA, accept EULA and check that Variation service is initialized), you may add autotest. Let's cover that separately, you may create a bug and we'll provide additional info.
On 2012/09/17 13:17:00, SteveT wrote: > BTW - Once the IsEulaAccepted starts returning true, there is nothing that will > set it to false short of changing the value in the Local State file? That's correct.
http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/base_login_display_host.cc (right): http://codereview.chromium.org/10917120/diff/1016/chrome/browser/chromeos/log... chrome/browser/chromeos/login/base_login_display_host.cc:201: RegisterForEulaAcceptedNotification(wizard_controller_.get()); From offline discussion with Nikita: It looks like the first call to (possibly) show the wizard is in PostProfileInit (http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/ch...), which happens before VariationsService attempts its initial ping. This means that when the VS attempts its first ping, it (or rather, the RRANotifier) can check to see if WizardController::default_controller() is not NULL, and register itself for the notification* at that point. This removes the coupling of WizCtrl with VS. We should also be able to DCHECK that default_controller is not NULL if IsEulaAccepted had returned false at that time, give the discussion of how IsEulaAccepted should always return true after the first accept... otherwise if it was not accepted it will initiate the OOBE login wizard. Note that all this stuff (including VS' first ping attempt) happens before the main msg loop, so the Wizard UI won't be shown to users yet, so we avoid any races where the user accepts the Eula around the time that the VS checks it/registers for notifications.
Okay, so after some debugging and discussions with Nikita, it looks like we can't easily assert when the WizCtrl::default_controller() instance is available, because the WizardController actually has two load paths, one of which is asynchronously. As a result, Nikita and I believe that the best way to inform interested parties about the availability of the WizardController is through a notification... which brings us back to the original problem. Chatting offline with Alexei about this, we feel like, since we're guaranteed that there is only one Eula (and WizardController) for the foreseeable future, we go with listening to AllSources. - It's a lot less error prone (I ran into tons of crashy situations trying to find the right place to hook into the WizardController directly). - It resolves the dependency issue Alexei originally brought up. - Technically there is precedence for it, a there are a number of other NOTIFICATIONS from the same submodule of code that transmits on AllSources. (though you can definitely argue that it's not a preferrable pattern...) Ilya - what do you think? The other option is that somewhat hacky listen-on-an-int-handle suggestion that Alexei suggested.
On 2012/09/19 01:59:10, SteveT wrote: > Okay, so after some debugging and discussions with Nikita, it looks like we > can't easily assert when the WizCtrl::default_controller() instance is > available, because the WizardController actually has two load paths, one of > which is asynchronously. As a result, Nikita and I believe that the best way to > inform interested parties about the availability of the WizardController is > through a notification... which brings us back to the original problem. > > Chatting offline with Alexei about this, we feel like, since we're guaranteed > that there is only one Eula (and WizardController) for the foreseeable future, > we go with listening to AllSources. > - It's a lot less error prone (I ran into tons of crashy situations trying to > find the right place to hook into the WizardController directly). > - It resolves the dependency issue Alexei originally brought up. > - Technically there is precedence for it, a there are a number of other > NOTIFICATIONS from the same submodule of code that transmits on AllSources. > (though you can definitely argue that it's not a preferrable pattern...) > > Ilya - what do you think? Ok. Please document this reasoning by the code that listens for a notification from AllSources(). And, please still *send* the notification with a specific source, even if you only listen for AllSources().
On 2012/09/19 02:47:11, Ilya Sherman wrote: > On 2012/09/19 01:59:10, SteveT wrote: > > Okay, so after some debugging and discussions with Nikita, it looks like we > > can't easily assert when the WizCtrl::default_controller() instance is > > available, because the WizardController actually has two load paths, one of > > which is asynchronously. As a result, Nikita and I believe that the best way > to > > inform interested parties about the availability of the WizardController is > > through a notification... which brings us back to the original problem. > > > > Chatting offline with Alexei about this, we feel like, since we're guaranteed > > that there is only one Eula (and WizardController) for the foreseeable future, > > we go with listening to AllSources. > > - It's a lot less error prone (I ran into tons of crashy situations trying to > > find the right place to hook into the WizardController directly). > > - It resolves the dependency issue Alexei originally brought up. > > - Technically there is precedence for it, a there are a number of other > > NOTIFICATIONS from the same submodule of code that transmits on AllSources. > > (though you can definitely argue that it's not a preferrable pattern...) > > > > Ilya - what do you think? > > Ok. Please document this reasoning by the code that listens for a notification > from AllSources(). And, please still *send* the notification with a specific > source, even if you only listen for AllSources(). Okay, sounds good. Will ping this when that's all done and cleaned up.
Okay, back to using AllSources, with a comment to explain why. Fixed up a few things in the RRAN class to only listen for the notification when we need it (rather than add/remove in the ctor/dtor). Also, a question for reviewers in variations_service_unittests.cc, which had a bit of an overhaul thanks to these new tests. PTAL! Steve http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:109: // TODO(stevet): This is not great since it creates useless overhead for non- REVIEWERS: Any thoughts here? I might be able to just create a CreateVariationsTestLocalState() helper in an anon-namespace which returns a TestingPerfService with the EulaAccepted pref already set to true (on CrOS)... and then use that the way Jesse's original tests used it (rather than have this fixture class). Does that seem neater?
I haven't finished looking over everything yet, but here's the first set of comments. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:106: local_state_->SetUserPref("EulaAccepted", Can you instead wrap chromeos::WizardController::IsEulaAccepted() in a virtual method on ResourceRequestAllowedNotifier? Then, the test instance of that class could override it and the test wouldn't have to know about implementation-specific details of the Wizard (i.e. where it stores the pref). http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:189: resource_request_allowed_notifier_->ClearObserver(); The destructor of the notifier should do the sane thing and clear its own observer. Then the call to ClearObserver() here won't be necessary since it would be handled by the .reset() below and similarly the ClearObserver() call in ~VariationsService() could also be removed. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:62: virtual void DoActualFetch(); Why the name change? http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:71: bool create_trials_from_seed_called_; I suggest instead making *ForTesting() getters and setters and keeping this private. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:128: local_state_->SetUserPref("EulaAccepted", Value::CreateBooleanValue(true)); I made a comment about this in the test for new notifier class. Ideally, you should be able to use a test notifier here too to simulate the state you want, rather than murking around in the prefs. http://codereview.chromium.org/10917120/diff/24001/chrome/common/chrome_notif... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/common/chrome_notif... chrome/common/chrome_notification_types.h:1025: // Sent when the EULA has been accepted in the first-run wizard. Mention that it doesn't sent if the wizard never runs (e.g. if the eula had previously been accepted)?
(lg) http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:31: // observer. nit: Please mention that the WizardController is a singleton, which is why listening for AllSources() is ok. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:109: // TODO(stevet): This is not great since it creates useless overhead for non- On 2012/09/19 19:27:09, SteveT wrote: > REVIEWERS: Any thoughts here? I might be able to just create a > CreateVariationsTestLocalState() helper in an anon-namespace which returns a > TestingPerfService with the EulaAccepted pref already set to true (on CrOS)... > and then use that the way Jesse's original tests used it (rather than have this > fixture class). > > Does that seem neater? Either way seems fine to me, or Alexei's suggestion also seems fine. If you keep this approach, a couple of nits below... http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:123: void SetUp() { nit: OVERRIDE http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:133: TestingPrefService* local_state_; nit: This doesn't look like it needs to be a separate member variable -- you could just use scoped_testing_local_state_ throughout (and rename that one to local_state_ if you get rid of this one).
And another set of comments. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:43: DCHECK(!observer_ && observer); Either remove this check or document it in the header. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:44: observer_ = observer; Hmm, I think perhaps the checks that are currently in the constructor should be done here instead. (i.e. calling !chromeos::WizardController::IsEulaAccepted()). Here's my reasoning: If there's some interval between construction and setting the observer, then notifications during that time may be lost (since the code that checks to notify the observer will just be a no-op). Alternatively, perhaps it might be cleaner to just require that the observer is a parameter to the constructor and offer no way to reset it. That will avoid these kinds of edge cases and ensure people use this class in a way that is not error prone. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:48: DCHECK(observer_); Either remove this check or document it in the header. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:95: if (was_waiting_for_user_to_accept_eula_) { DCHECK that this is true instead. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:35: // If the ResourceRequestsAllowed returns true the first time, Nit: remove the first "the". http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:59: // Sets |observer| as the observing service to be notified. Mention that this is a weak reference. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:65: // Returns true iff all resource request criteria is met. If not, this call Nit: "is met" -> "are met". ("criteria" is plural, the singular is "criterion"). http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:87: // net::NetworkChangeNotifier::ConnectionTypeObserver implementation. Nit: Use a consistent style for the comment. You use "content::NotificationObserver overrides:" above, but "net::NetworkChangeNotifier::ConnectionTypeObserver implementation." here. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:88: virtual void OnConnectionTypeChanged( I don't think this should be in an ifdef CHROMEOS block.
Okay, I think I've addressed everything from this round. Note that there was some significant test refactoring from some of the comments. Please note the new test_util files. I think it's definitely worth giving those tests another full once-over. Steve http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:31: // observer. On 2012/09/19 19:49:01, Ilya Sherman wrote: > nit: Please mention that the WizardController is a singleton, which is why > listening for AllSources() is ok. Done. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:43: DCHECK(!observer_ && observer); Will document in header due to the next comment... http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:44: observer_ = observer; I like your latter suggestion of setting the observer at construction-time, which avoids a whole ton of troubles. Plus, it doesn't make sense for an RRAN to exist without someone to notify - it's raison d'etre, if you will :). I've made those changes. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:48: DCHECK(observer_); Will document in header due to the previous comment... http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:95: if (was_waiting_for_user_to_accept_eula_) { Thanks - done. That caught a few test issues that I fixed. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:35: // If the ResourceRequestsAllowed returns true the first time, On 2012/09/19 20:05:35, Alexei Svitkine wrote: > Nit: remove the first "the". Done. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:59: // Sets |observer| as the observing service to be notified. Removed this completely since we now take in that dependency in the ctor. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:65: // Returns true iff all resource request criteria is met. If not, this call On 2012/09/19 20:05:35, Alexei Svitkine wrote: > Nit: "is met" -> "are met". ("criteria" is plural, the singular is "criterion"). Done. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:87: // net::NetworkChangeNotifier::ConnectionTypeObserver implementation. Using "overrides:" http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:88: virtual void OnConnectionTypeChanged( Correct. Will have to rerun those other trybots and retest this on chromeos=0... http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:106: local_state_->SetUserPref("EulaAccepted", Done. Note that this made fairly substantial changes to the testing code (and some of the base code). One change of note is that we now call IsEulaAccepted from the newly-added Init method rather than the ctor due to the issue with calling virtuals from the ctor. Let me know if that is not acceptable. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:189: resource_request_allowed_notifier_->ClearObserver(); This is no longer applicable. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:62: virtual void DoActualFetch(); I separated all the URLFetchRequest stuff into this method (notice that FetchVariationsSeed still exists above) so I can mock this method to see if it gets called based on notifications from the RRANotifier. Let me know if there is a preferable pattern. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:71: bool create_trials_from_seed_called_; On 2012/09/19 19:44:28, Alexei Svitkine wrote: > I suggest instead making *ForTesting() getters and setters and keeping this > private. Done. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:109: // TODO(stevet): This is not great since it creates useless overhead for non- This is gone now since we're using that virtual IsEulaAccepted method. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:123: void SetUp() { This class has been removed. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:128: local_state_->SetUserPref("EulaAccepted", Value::CreateBooleanValue(true)); This class has been removed. http://codereview.chromium.org/10917120/diff/24001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:133: TestingPrefService* local_state_; This class has been removed. http://codereview.chromium.org/10917120/diff/24001/chrome/common/chrome_notif... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10917120/diff/24001/chrome/common/chrome_notif... chrome/common/chrome_notification_types.h:1025: // Sent when the EULA has been accepted in the first-run wizard. On 2012/09/19 19:44:28, Alexei Svitkine wrote: > Mention that it doesn't sent if the wizard never runs (e.g. if the eula had > previously been accepted)? Done.
More comments your way, mostly about the tests now. Getting closer. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:21: net::NetworkChangeNotifier::AddConnectionTypeObserver(this); Do this in Init() so that there's no timing hole between construction and Init() where we could be notified. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:25: net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this); After the above change, surround this in if (observer_), so that this does the right thing if Init() wasn't called. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:71: if (observer_ && ResourceRequestsAllowed()) { No need to check for |observer_| here once the above changed are done. If |observer_| wasn't set - i.e. Init() wasn't called, then we wouldn't be listening for notifications. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:18: class WizardController; Is this needed? I don't see the header using it. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:54: // the WizardController if the EULA was not accepted at startup (ChromeOS Nit: Prefix WizardController with its namespace in this comment. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc:46: else Nit: remove the else and unindent the next line. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:15: class TestRequesterService : public ResourceRequestAllowedNotifier::Observer { I wonder if you could just merge this with the ResourceRequestAllowedNotifierTest class - so that class would be the service (and would implement Observer). Then, this code can be a bit simpler. What do you think? http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:36: resource_request_allowed_notifier_.OverrideRequestsAllowed(override); Maybe just expose a getter for |resource_request_allowed_notifier_| and remove these plumbing methods? Moot if these classes are merged per my above comment. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:158: TestingPrefService* local_state_; Do you need the local state / scoped testing local state now that you're mocking these away? http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:179: VariationsService::GetResourceRequestAllowedNotifierForTesting() { Nit: Indent 4. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:39: // This constructor is created to injecting a mock notifier. It is meant for Nit: wording. This constructor exists for injecting a mock notifier. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:59: void FetchVariationsSeed(); Why is this public? http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:65: ResourceRequestAllowedNotifier* GetResourceRequestAllowedNotifierForTesting(); I think this won't be needed anymore, after you follow my suggestions for the tests. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:163: // This field is protected so test classes can modify it. Not protected anymore, remove this comment. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:29: void SetRequestsAllowed(bool allowed); It seems both SetRequestsAllowed() and NotifyObservers() just pipe things through to the underlying TestRequestAllowedNotifier and the constructor of this class is quite complex. I think you can simplify things by just having TestVariationsService() offer the same constructor that takes the RequestAllowedNotifier as a param. Then, the test itself could just allocate the TestRequestedAllowedNotifier and call stuff on it (like SetRequestsAllowed() and NotifyObservers()). e.g. the test code in ResourceRequestAllowedNotifierTest look like: TestRequestAllowedNotifier* test_notifier = new TestRequestAllowedNotifier; TestVariationsService test_variations_service(test_notifier); test_notifier->SetRequestsAllowed(false); .... For the other tests that don't need to call these functions, they can either do: TestVariationsService test_variations_service(new TestRequestAllowedNotifier); Or you can add a default constructor that does that too.
Thanks for the comments - tests are looking a lot nicer now. Back to you for another look. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:21: net::NetworkChangeNotifier::AddConnectionTypeObserver(this); On 2012/09/20 21:39:03, Alexei Svitkine wrote: > Do this in Init() so that there's no timing hole between construction and Init() > where we could be notified. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:25: net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this); On 2012/09/20 21:39:03, Alexei Svitkine wrote: > After the above change, surround this in if (observer_), so that this does the > right thing if Init() wasn't called. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:71: if (observer_ && ResourceRequestsAllowed()) { On 2012/09/20 21:39:03, Alexei Svitkine wrote: > No need to check for |observer_| here once the above changed are done. If > |observer_| wasn't set - i.e. Init() wasn't called, then we wouldn't be > listening for notifications. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:18: class WizardController; Removed now. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:54: // the WizardController if the EULA was not accepted at startup (ChromeOS On 2012/09/20 21:39:03, Alexei Svitkine wrote: > Nit: Prefix WizardController with its namespace in this comment. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc:46: else On 2012/09/20 21:39:03, Alexei Svitkine wrote: > Nit: remove the else and unindent the next line. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:15: class TestRequesterService : public ResourceRequestAllowedNotifier::Observer { Yes! That works and is definitely simpler. Thanks. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:36: resource_request_allowed_notifier_.OverrideRequestsAllowed(override); Not applicable due to the above refactoring. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:158: TestingPrefService* local_state_; Thanks for catching that - forgot to remove it after the last round of refactoring. Done! http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:179: VariationsService::GetResourceRequestAllowedNotifierForTesting() { Removed. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:39: // This constructor is created to injecting a mock notifier. It is meant for On 2012/09/20 21:39:03, Alexei Svitkine wrote: > Nit: wording. This constructor exists for injecting a mock notifier. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:59: void FetchVariationsSeed(); This has been made private now. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:65: ResourceRequestAllowedNotifier* GetResourceRequestAllowedNotifierForTesting(); Yes - removed. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:163: // This field is protected so test classes can modify it. On 2012/09/20 21:39:03, Alexei Svitkine wrote: > Not protected anymore, remove this comment. Done. http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/31001/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:29: void SetRequestsAllowed(bool allowed); Thanks for that suggestion - made things a lot nicer. Getting rid of the GetRRANotifierForTesting method...
Thanks, tests are looking much better. Almost there, just a few more comments. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:15: ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier() : Nit: ":" should go on the next line. Perhaps reorder |was_waiting_for_user_to_accept_eula_| and |was_waiting_for_network_| so that the ifdef will be cleaner once you put ":" on the next line. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:71: void ResourceRequestAllowedNotifier::MaybeNotifyObservers() { Nit: Should be MaybeNotifyObserver() (singular). http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:94: MaybeNotifyObservers(); This still doesn't work when net::NetworkChangeNotifier::IsOffline() is out of sync with the notification, per an earlier comment from mmenke on another review. The problem is that if we get notified that network is available but IsOffline() still returns true, then MaybeNotifyObservers() won't notify the observer and as long as network doesn't change state, the observer won't get notified again! Basically, the issue is that the code called by the body of this if statement shouldn't be checking for net::NetworkChangeNotifier::IsOffline() and should instead just set |was_waiting_for_network_| based on the notification. I think one way to solve this would be to only check net::NetworkChangeNotifier::IsOffline() in Init(), similarly to how IsEulaAccepted() is done on ChromeOS. Then, this if body can set was_waiting_for_network_ = true; and ResourceRequestsAllowed() would only act on the state of the two variables without making the IsOffline() call. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:10: // A light wrapper class around ResourceRequestAllowedNotifier used to expose I don't think "wrapper" is the right terminology here, I think "subclass" is the correct term to use instead. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:11: // some functionality for testing. Nit: Add an empty line within the comment here. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:30: void OverrideRequestsAllowed(bool override); Could this be simplified and merged with SetRequestsAllowed? e.g. only have one method instead of two, and call it: void SetRequestsAllowedOverride(bool allowed); This would set |override_requests_allowed_| to true and |requests_allowed_| to |allowed|. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:232: FetchVariationsSeed(); Should this call DoActualFetch() instead? Since we were notified that requests are allowed, I don't think it needs to call FetchVariationsSeed() that does an extra check. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:59: // the response. If the network is down at the time, sets a flag to retry when Update this comment, since this class no longer manages the "flag to retry" itself, since that's all done by the notifier now. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:25: // mock operations on the notifier. Is the comment about the test_notifier true? I notice none of your tests actually pass in NULL. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:31: void SetRequestsAllowed(bool allowed); Remove. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:33: void NotifyObservers(); Remove. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:44: TestRequestAllowedNotifier* test_notifier_; Remove. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:55: } Nit: Inline this into the class declaration above. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:57: TestVariationsService::~TestVariationsService() { Nit: Inline this into the class declaration above.
Fixed issues, updated tests, rebased files, and made some non-CrOS compiler fixes (trybots underway again). Whew. PTAL. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:15: ResourceRequestAllowedNotifier::ResourceRequestAllowedNotifier() : On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Nit: ":" should go on the next line. Perhaps reorder > |was_waiting_for_user_to_accept_eula_| and |was_waiting_for_network_| so that > the ifdef will be cleaner once you put ":" on the next line. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:71: void ResourceRequestAllowedNotifier::MaybeNotifyObservers() { On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Nit: Should be MaybeNotifyObserver() (singular). Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.cc:94: MaybeNotifyObservers(); As discussed offline, we addressed this by shifting the IsOffline logic as you suggested, but also added a new observer_requested_permission_ field to ensure that we don't notify the observer/service unless it made a call to ResourceRequestsAllowed, which denotes its interest in being alerted. Note that there were some test changes that accompanied this new field. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:10: // A light wrapper class around ResourceRequestAllowedNotifier used to expose On 2012/09/21 15:52:05, Alexei Svitkine wrote: > I don't think "wrapper" is the right terminology here, I think "subclass" is the > correct term to use instead. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:11: // some functionality for testing. On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Nit: Add an empty line within the comment here. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:30: void OverrideRequestsAllowed(bool override); That simplified things a bit, yeah. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.cc:232: FetchVariationsSeed(); Yeah I think you're right in that we don't need to recheck, since we expect that we have the OK to go if this is called. I suppose there is always a risk that the network could go down again between here and FetchVariationsSeed, but we would simply encounter a failed URLFetcher request and reattempt in 5 hours. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service.h (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service.h:59: // the response. If the network is down at the time, sets a flag to retry when On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Update this comment, since this class no longer manages the "flag to retry" > itself, since that's all done by the notifier now. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:25: // mock operations on the notifier. Sorry - I had tried this earlier before taking your advice to directly pass in a new TestRRAN for tests that do not care. It didn't work but I forgot to remove this comment. Remove this comment. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:31: void SetRequestsAllowed(bool allowed); On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Remove. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:33: void NotifyObservers(); On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Remove. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:44: TestRequestAllowedNotifier* test_notifier_; On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Remove. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:55: } On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Nit: Inline this into the class declaration above. Done. http://codereview.chromium.org/10917120/diff/33005/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:57: TestVariationsService::~TestVariationsService() { On 2012/09/21 15:52:05, Alexei Svitkine wrote: > Nit: Inline this into the class declaration above. Done.
One question inline for Nikita. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/wizard_controller.cc:139: previous_screen_(NULL), Question for Nikita: In non-official builds, are we guaranteed that IsEulaAccepted() always returns true? If not, then I think I can have ResourceRequestAllowedNotifier::IsEulaAccepted return true if defined(GOOGLE_CHROME_BUILD).
Looking good, just a few more comments, mostly about comments and names and asking for one more test. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:47: // Note that the constructor has some logic to register for notifications from Please update the comment, since what's described here is now done in Init(). http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc:31: // notify observers. Nit: Expand comment to say that MaybeNotifyObserver() checks for "ResourceRequestsAllowed()". http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:31: // Notify observers that requests are allowed. Expand comment to say that this will only work if the observer is expecting a notification (i.e. it previously asked whether requests are allowed). Also, change the method name to singular Observer instead of Observers. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:42: bool requests_allowed_; Nit: Move this below "override_requests_allowed_" and add a comment that it's used only if override_requests_allowed_ is true. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:22: void SimulateNetworkConnectionChange( Nit: Add a comment. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:34: net::NetworkChangeNotifier::ConnectionType connection_type_to_return_; Nit: Add a comment. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:57: bool request_attempted() const { return was_notified_; } Nit: Rename the getter too. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:131: TEST_F(ResourceRequestAllowedNotifierTest, DoNotRequestIfOffline) { Nit: Rename tests to not say "request", use the word "notify" instead. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:182: // get notified, even if the criteria is met. Can you add a similar test but with the Eula condition changing? http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:497: EXPECT_FALSE(test_service.fetch_attempted()); Can you add a comment explaining why this is expected to be false? (I assume because by default, TestRequestAllowedNotifier returns that the network is not available.) Can you add a second test to this file that will check what happens if TestRequestAllowedNotifier will initially return that the network is available? (In which case, StartRepeatedVariationsSeedFetch() should result in fetch_attempted() being true.)
On 2012/09/24 15:41:13, SteveT wrote: > One question inline for Nikita. > > http://codereview.chromium.org/10917120/diff/38002/chrome/browser/chromeos/lo... > File chrome/browser/chromeos/login/wizard_controller.cc (right): > > http://codereview.chromium.org/10917120/diff/38002/chrome/browser/chromeos/lo... > chrome/browser/chromeos/login/wizard_controller.cc:139: previous_screen_(NULL), > Question for Nikita: > > In non-official builds, are we guaranteed that IsEulaAccepted() always returns > true? It will return false. > > If not, then I think I can have ResourceRequestAllowedNotifier::IsEulaAccepted > return true if defined(GOOGLE_CHROME_BUILD). Yes, this is one of the options.
Back to you. TODOs left: - Manual re-testing on CrOS and non-CrOS to ensure things are ticking. - Will probably have to make that change as discussed with Nikita... will get to after my manual testing since it makes testing difficult. - Making the trybots green. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:47: // Note that the constructor has some logic to register for notifications from Removed this comment and updated Init's to describe where the check(s) occur. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.cc:31: // notify observers. On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Nit: Expand comment to say that MaybeNotifyObserver() checks for > "ResourceRequestsAllowed()". Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:31: // Notify observers that requests are allowed. On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Expand comment to say that this will only work if the observer is expecting a > notification (i.e. it previously asked whether requests are allowed). > > Also, change the method name to singular Observer instead of Observers. Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:42: bool requests_allowed_; Done, but this makes the constructor implementation a bit grosser (see cc file). http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:22: void SimulateNetworkConnectionChange( On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Nit: Add a comment. Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:34: net::NetworkChangeNotifier::ConnectionType connection_type_to_return_; On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Nit: Add a comment. Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:57: bool request_attempted() const { return was_notified_; } On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Nit: Rename the getter too. Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:131: TEST_F(ResourceRequestAllowedNotifierTest, DoNotRequestIfOffline) { On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Nit: Rename tests to not say "request", use the word "notify" instead. Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:182: // get notified, even if the criteria is met. On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Can you add a similar test but with the Eula condition changing? Done. http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:497: EXPECT_FALSE(test_service.fetch_attempted()); On 2012/09/24 15:59:36, Alexei Svitkine wrote: > Can you add a comment explaining why this is expected to be false? (I assume > because by default, TestRequestAllowedNotifier returns that the network is not > available.) > > Can you add a second test to this file that will check what happens if > TestRequestAllowedNotifier will initially return that the network is available? > (In which case, StartRepeatedVariationsSeedFetch() should result in > fetch_attempted() being true.) I have to push back on this because I believe that the individual criteria were supposed to be mocked and tested in the RRAN unit tests, and not here in VariationsServiceTest, which does not care about the more granular criteria checks. But this comment brings up an issue in my code: This was succeeding only because the EULA accepted state was false (hence the first fetch_attempted returned false). Since I don't want to test EULA (or network) criteria here, the correct thing to do is to force the first result to be false with ResourceRequestsAllowedOverride. (I also think this might have been related to why my non-CrOS tests were failing - rerunning bots now)
http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:497: EXPECT_FALSE(test_service.fetch_attempted()); Sorry, what I actually meant was: Can you add a second test to this file that will check what happens if TestRequestAllowedNotifier will initially return that requests "are allowed"? (In which case, StartRepeatedVariationsSeedFetch() should result in fetch_attempted() being true.)
Back to you. Status: - Bots are looking greener, though ASAN tests seem to be failing on something unrelated. - The ifdefing and flipping of IsEulaAccepted is done. - Manual tests look good on CrOS. Doing Non-CrOS now... http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/variations_service_unittest.cc (right): http://codereview.chromium.org/10917120/diff/38002/chrome/browser/metrics/var... chrome/browser/metrics/variations/variations_service_unittest.cc:497: EXPECT_FALSE(test_service.fetch_attempted()); Oh, yes, of course :)
Just a few remaining comments. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:62: void SetObserverRequestedPermissionForTesting(bool requested); Instead of this method, can the test simply call ResourceRequestAllowed()? http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:69: // Notifies observers if all criteria needed for resource requests are met. Nit: "observers" -> "observer". http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:108: content::NotificationRegistrar registrar_; Nit: Add a comment to mention its for the eula notifications. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:10: // A light subclass around ResourceRequestAllowedNotifier used to expose some Nit: "subclass around" -> "subclass of". "light" -> "light weight", or just omit that word. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:116: SetObserverRequestedPermission(true); I would suggest to default to false and have tests actually request it, so that the behaviour being tested is clearer.
Over to you. Manual testing looking good.. finishing last round tomorrow (EULA+Network tests). http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:62: void SetObserverRequestedPermissionForTesting(bool requested); Some refactoring done to make this happen. Please see TestRequestAllowedNotifier::ResourceRequestsAllowed's definition and let me know if that's acceptable... It's the only way to do your way without keeping this ForTesting method (aside from making a field private->protected). http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:69: // Notifies observers if all criteria needed for resource requests are met. On 2012/09/24 20:47:05, Alexei Svitkine wrote: > Nit: "observers" -> "observer". Done. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:108: content::NotificationRegistrar registrar_; On 2012/09/24 20:47:05, Alexei Svitkine wrote: > Nit: Add a comment to mention its for the eula notifications. Done. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h:10: // A light subclass around ResourceRequestAllowedNotifier used to expose some On 2012/09/24 20:47:05, Alexei Svitkine wrote: > Nit: "subclass around" -> "subclass of". "light" -> "light weight", or just omit > that word. Done. http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/32016/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:116: SetObserverRequestedPermission(true); N/A after your comment about getting rid of SetObserverRequestedPermissionForTesting.
LGTM, thanks! http://codereview.chromium.org/10917120/diff/57001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/57001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:82: ResourceRequestsAllowed(); Nit: I think this can fit on the previous line.
Thanks for that epic review, Alexei. Over for OWNERS review now. +sky: Mind taking a look, Scott? Feel free to pass this on if there is a better chrome/ owner for this. http://codereview.chromium.org/10917120/diff/57001/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc (right): http://codereview.chromium.org/10917120/diff/57001/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:82: ResourceRequestsAllowed(); On 2012/09/25 15:34:34, Alexei Svitkine wrote: > Nit: I think this can fit on the previous line. Done.
+sky for realz for OWNERS review of chrome/
LGTM
lgtm http://codereview.chromium.org/10917120/diff/51002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/10917120/diff/51002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/wizard_controller.cc:469: content::NotificationSource(content::Source<WizardController>(this)), As you're essentially using AllSources in ResourceRequestAllowedNotifier, do you need to pass source here? http://codereview.chromium.org/10917120/diff/51002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/51002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Should ResourceRequestAllowedNotifier be moved to somewhere out of chrome/browser/metrics/variations? As far as I understand intention was to make it useful for other services as well (like Translate).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/10917120/51002
Let's see if the CQ gives us some more love than these manual bot runs. Responses for Nikita inline... no changes were needed. http://codereview.chromium.org/10917120/diff/51002/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/10917120/diff/51002/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/wizard_controller.cc:469: content::NotificationSource(content::Source<WizardController>(this)), This was suggested by Ilya. I believe we want to be as specific as we can, when we can. It'll make things a step easier to update if we ever clean up this dependency issue. It's also more precise. http://codereview.chromium.org/10917120/diff/51002/chrome/browser/metrics/var... File chrome/browser/metrics/variations/resource_request_allowed_notifier.h (right): http://codereview.chromium.org/10917120/diff/51002/chrome/browser/metrics/var... chrome/browser/metrics/variations/resource_request_allowed_notifier.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Again, it used to be under chrome/browser, but an early comment had me move it here since we're the only consumer so far. We will leave the fun of migrating it to the next user (of which there may be many... we have to spend a bit of time investigating that to make sure we're obeying the Eula).
Change committed as 158648 |
