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

chrome/browser/chromeos/network_message_observer.cc

Index: chrome/browser/chromeos/network_message_observer.cc
===================================================================
--- chrome/browser/chromeos/network_message_observer.cc	(revision 72888)
+++ chrome/browser/chromeos/network_message_observer.cc	(working copy)
@@ -152,7 +152,29 @@
   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.