Only show notification for low 3g data for the last data-based plan....

chrome/browser/chromeos/network_message_observer.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/network_message_observer.h"
 
 #include "base/callback.h"
 #include "base/stl_util-inl.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
@@ skipping 130 matching lines @@
   if (!new_failed_network.empty()) {
     // Hide if already shown to force show it in case user has closed it.
     if (notification_connection_error_.visible())
       notification_connection_error_.Hide();
     notification_connection_error_.Show(l10n_util::GetStringFUTF16(
         IDS_NETWORK_CONNECTION_ERROR_MESSAGE,
         ASCIIToUTF16(new_failed_network)), false, false);
   }
 }
 
 void NetworkMessageObserver::OnCellularDataPlanChanged(NetworkLibrary* obj) {

[Vince Laviano, 2011/02/02 21:47:43]

I'm going to comment on the entire method rather than just on the new block of
code that this CL adds.

Overall, I find this method confusing and think that it should be broken up into
smaller methods with clear names that indicate the purpose of each subtask. This
is particularly true of the code at the bottom of the method that throws up the
various notifications. They could be replaced with calls to
ShowNoDataWarning(plan), ShowLowDataWarning(plan), etc. 

This also serves as a form of commentless commenting. If we would normally
write:

// show a low data notification
blah
blah
blah

we can instead factor that all out into a method that is named similarly to the
comment, ShowLowDataNotification(), and the call itself will provide the same
information as the comment.

[Charlie Lee, 2011/02/08 02:09:16]

Done.

   const CellularNetwork* cellular = obj->cellular_network();
   if (!cellular)
     return;
-  const CellularDataPlan* plan = cellular->GetSignificantDataPlan();
+
+  // Check to see if the active plan is time-based or data-based.
+  // If data-based, then only do notification if there's 1 data-based plan left.
+  const CellularDataPlan* plan = NULL;
+  const CellularDataPlanVector& plans = cellular->GetDataPlans();
+  for (CellularDataPlanVector::const_iterator iter = plans.begin();
+       iter != plans.end();
+       ++iter) {
+    if (plan == NULL) {  // Looking at first active plan
+      plan = *iter;
+      // If time-based, then no need to look further.
+      if (plan->plan_type == CELLULAR_DATA_PLAN_UNLIMITED)
+        break;
+    } else {
+      // If we find any other data-based plan, then no need for notification.
+      if ((*iter)->plan_type == CELLULAR_DATA_PLAN_METERED_PAID ||
+          (*iter)->plan_type == CELLULAR_DATA_PLAN_METERED_BASE) {

[Vince Laviano, 2011/02/02 21:47:43]

This logic assumes that the data-based plan that we've found is available now
rather than having a start time well into the future.

See also my other comments attached to issue 10010. 

I'd like to see code that is robust to receiving a plan list of arbitrary
length, with each plan having arbitrary values for plan name, type, start time,
end time, max bytes, and used bytes.

Assuming implicit constraints that are true with our current plans but may not
be true of future plans could come back to bite us later.
 

[Charlie Lee, 2011/02/08 02:09:16]

Done.

+        plan = NULL;
+        break;
+      }
+    }
+  }
+

[Vince Laviano, 2011/02/02 21:47:43]

It looks like |plan| is set to non-NULL if there is exactly one data-based plan
in the list, and NULL otherwise. This is not really captured in the name "plan."

If the above code were factored out into a separate method, what would that
method be named? (Maybe we should actually do that factoring.)  

I think, once this question is answered, a better name for |plan| will emerge.

[Charlie Lee, 2011/02/08 02:09:16]

Done.

   std::string new_plan_name = plan ? plan->plan_name : "";
   CellularDataPlanType new_plan_type = plan ? plan->plan_type :
                                               CELLULAR_DATA_PLAN_UNKNOWN;

[Vince Laviano, 2011/02/02 21:47:43]

I think that we could eliminate these two vars and set |cellular_data_plan_name|
and |cellular_data_plan_type| directly from |plan| if the plan is determined to
be new. As it is, they are confusing in that they are used whether or not a plan
is really new.

[Charlie Lee, 2011/02/08 02:09:16]

Done.

 
   // If connected cellular network changed, or data plan is different, then
   // it's a new network. Then hide all previous notifications.
   bool new_plan = false;
   if (cellular->service_path() != cellular_service_path_) {
     cellular_service_path_ = cellular->service_path();
     new_plan = true;
   } else if (new_plan_name != cellular_data_plan_name_ ||
       new_plan_type != cellular_data_plan_type_) {
     new_plan = true;
   }

[Vince Laviano, 2011/02/02 21:47:43]

Not sure that testing just name and type is sufficient to determine whether two
plans are different. By this logic, two unlimited daypasses for different days
would be considered the same plan. IMO, a plan is uniquely described by the
union of name, type, start time, end time, and max bytes.

[Charlie Lee, 2011/02/08 02:09:16]

Done.

 
   cellular_data_plan_name_ = new_plan_name;
   cellular_data_plan_type_ = new_plan_type;
 
   bool should_show_notifications = ShouldShowMobilePlanNotifications();
   if (!should_show_notifications) {

[Vince Laviano, 2011/02/02 21:47:43]

This bool isn't used anywhere else, so we could eliminate it and substitute in a
direct call to ShouldShowMobilePlanNotifications().

[Charlie Lee, 2011/02/08 02:09:16]

Done.

     notification_low_data_.Hide();
     notification_no_data_.Hide();

[Vince Laviano, 2011/02/02 21:47:43]

I see these two lines repeated several times. Maybe replace with a
HideAllNotifications() method.

[Charlie Lee, 2011/02/08 02:09:16]

Done.

     return;
   }
 
   if (new_plan) {
     notification_low_data_.Hide();
     notification_no_data_.Hide();
     if (!plan && cellular->needs_new_plan()) {
       notification_no_data_.set_title(
           l10n_util::GetStringFUTF16(IDS_NETWORK_NO_DATA_PLAN_TITLE,
                                      UTF8ToUTF16(cellular->service_name())));
@@ skipping 48 matching lines @@
         NewCallback(this, &NetworkMessageObserver::OpenMoreInfoPage),
         false, false);
   } else {
     // Got data, so hide notifications.
     notification_low_data_.Hide();
     notification_no_data_.Hide();
   }
 }
 
 }  // namespace chromeos