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

Only show notification for low 3g data for the last data-based plan.
BUG=chromium-os:10010
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74319

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

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
File chrome/browser/chromeos/network_message_observer.cc (right):

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:151: void
NetworkMessageObserver::OnCellularDataPlanChanged(NetworkLibrary* obj) {
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.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:171: (*iter)->plan_type ==
CELLULAR_DATA_PLAN_METERED_BASE) {
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.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:177: 
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.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:180:
CELLULAR_DATA_PLAN_UNKNOWN;
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.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:191: }
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.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:197: if
(!should_show_notifications) {
This bool isn't used anywhere else, so we could eliminate it and substitute in a
direct call to ShouldShowMobilePlanNotifications().

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:199:
notification_no_data_.Hide();
I see these two lines repeated several times. Maybe replace with a
HideAllNotifications() method.

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

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
File chrome/browser/chromeos/network_message_observer.cc (right):

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:151: void
NetworkMessageObserver::OnCellularDataPlanChanged(NetworkLibrary* obj) {
On 2011/02/02 21:47:43, Vince Laviano wrote:
> 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.

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:171: (*iter)->plan_type ==
CELLULAR_DATA_PLAN_METERED_BASE) {
On 2011/02/02 21:47:43, Vince Laviano wrote:
> 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.
>  

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:177: 
On 2011/02/02 21:47:43, Vince Laviano wrote:
> 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.

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:180:
CELLULAR_DATA_PLAN_UNKNOWN;
On 2011/02/02 21:47:43, Vince Laviano wrote:
> 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.

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:191: }
On 2011/02/02 21:47:43, Vince Laviano wrote:
> 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.

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:197: if
(!should_show_notifications) {
On 2011/02/02 21:47:43, Vince Laviano wrote:
> This bool isn't used anywhere else, so we could eliminate it and substitute in
a
> direct call to ShouldShowMobilePlanNotifications().

Done.

http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
chrome/browser/chromeos/network_message_observer.cc:199:
notification_no_data_.Hide();
On 2011/02/02 21:47:43, Vince Laviano wrote:
> I see these two lines repeated several times. Maybe replace with a
> HideAllNotifications() method.

Done.

[Vince Laviano, 2011-02-08 02:12:27]

On 2011/02/08 02:09:16, Charlie Lee wrote:
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> File chrome/browser/chromeos/network_message_observer.cc (right):
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:151: void
> NetworkMessageObserver::OnCellularDataPlanChanged(NetworkLibrary* obj) {
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:171: (*iter)->plan_type ==
> CELLULAR_DATA_PLAN_METERED_BASE) {
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > 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.
> >  
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:177: 
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:180:
> CELLULAR_DATA_PLAN_UNKNOWN;
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:191: }
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:197: if
> (!should_show_notifications) {
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > This bool isn't used anywhere else, so we could eliminate it and substitute
in
> a
> > direct call to ShouldShowMobilePlanNotifications().
> 
> Done.
> 
>
http://codereview.chromium.org/6347044/diff/2001/chrome/browser/chromeos/netw...
> chrome/browser/chromeos/network_message_observer.cc:199:
> notification_no_data_.Hide();
> On 2011/02/02 21:47:43, Vince Laviano wrote:
> > I see these two lines repeated several times. Maybe replace with a
> > HideAllNotifications() method.
> 
> Done.

LGTM