New wifi icons animation.

New wifi icons animation.
BUG=none
TEST=25538

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

[Charlie Lee (do not use), 2009-10-22 22:31:18]

http://codereview.chromium.org/315008/diff/3008/4039
File chrome/browser/chromeos/network_library.h (right):

http://codereview.chromium.org/315008/diff/3008/4039#newcode57
Line 57: // Called when the network has changed. (wifi networs, and ethernet)
I already fixed "networs" locally.

[sky, 2009-10-22 22:56:59]

http://codereview.chromium.org/315008/diff/3008/4023
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/315008/diff/3008/4023#newcode418
Line 418: <include name="IDR_STATUSBAR_WARNING"
file="statusbar_wifi_warning.png" type="BINDATA" />
Are all these images already in the repository?

http://codereview.chromium.org/315008/diff/3008/4038
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/3008/4038#newcode195
Line 195: void NetworkLibrary::CheckNetworkTraffic() {
How often is this called? It seems like a lot. Is it going to impact performance
poorly? For example, could you end up spawning a ton of tasks on the ui thread?

http://codereview.chromium.org/315008/diff/3008/4039
File chrome/browser/chromeos/network_library.h (right):

http://codereview.chromium.org/315008/diff/3008/4039#newcode59
Line 59: // Called when network traffic has been detected.
nit: add a newline between 58 and 59.

http://codereview.chromium.org/315008/diff/3008/4039#newcode61
Line 61: bool uploading) = 0;
Use a bitmask with enum values rather than two booleans. That way it's clearer
rather than having to remember was download first, or upload.

http://codereview.chromium.org/315008/diff/3008/4039#newcode64
Line 64: // This gets the singleton NetworkLibrary
nit: add a period to the end.

http://codereview.chromium.org/315008/diff/3008/4031
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/315008/diff/3008/4031#newcode158
Line 158: if (value > 1)
nit: this should be an else.

http://codereview.chromium.org/315008/diff/3008/4031#newcode188
Line 188: //   If value < value range of this image, draw at kMinOpacity
opacity.
it isn't clear what 'value range of this image' means. It's also hard to follow
when you have the same word on both sides of the operator, eg value < value.

http://codereview.chromium.org/315008/diff/3008/4031#newcode203
Line 203: value = 0;
How come you reset value here?

[Charlie Lee (do not use), 2009-10-22 23:35:01]

http://codereview.chromium.org/315008/diff/3008/4023
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/315008/diff/3008/4023#newcode418
Line 418: <include name="IDR_STATUSBAR_WARNING"
file="statusbar_wifi_warning.png" type="BINDATA" />
On 2009/10/22 22:56:59, sky wrote:
> Are all these images already in the repository?

Yes, davemoore added it this week.

http://codereview.chromium.org/315008/diff/3008/4038
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/3008/4038#newcode195
Line 195: void NetworkLibrary::CheckNetworkTraffic() {
On 2009/10/22 22:56:59, sky wrote:
> How often is this called? It seems like a lot. Is it going to impact
performance
> poorly? For example, could you end up spawning a ton of tasks on the ui
thread?

Yeah, it gets called a lot. What's a better way? Is there a way to check tasks
on the UI thread?

http://codereview.chromium.org/315008/diff/3008/4039
File chrome/browser/chromeos/network_library.h (right):

http://codereview.chromium.org/315008/diff/3008/4039#newcode59
Line 59: // Called when network traffic has been detected.
On 2009/10/22 22:56:59, sky wrote:
> nit: add a newline between 58 and 59.

Done.

http://codereview.chromium.org/315008/diff/3008/4039#newcode61
Line 61: bool uploading) = 0;
On 2009/10/22 22:56:59, sky wrote:
> Use a bitmask with enum values rather than two booleans. That way it's clearer
> rather than having to remember was download first, or upload.

Done.

http://codereview.chromium.org/315008/diff/3008/4039#newcode64
Line 64: // This gets the singleton NetworkLibrary
On 2009/10/22 22:56:59, sky wrote:
> nit: add a period to the end.

Done.

http://codereview.chromium.org/315008/diff/3008/4031
File chrome/browser/chromeos/network_menu_button.cc (right):

http://codereview.chromium.org/315008/diff/3008/4031#newcode158
Line 158: if (value > 1)
On 2009/10/22 22:56:59, sky wrote:
> nit: this should be an else.

Done.

http://codereview.chromium.org/315008/diff/3008/4031#newcode188
Line 188: //   If value < value range of this image, draw at kMinOpacity
opacity.
On 2009/10/22 22:56:59, sky wrote:
> it isn't clear what 'value range of this image' means. It's also hard to
follow
> when you have the same word on both sides of the operator, eg value < value.
> 

Done.

http://codereview.chromium.org/315008/diff/3008/4031#newcode203
Line 203: value = 0;
On 2009/10/22 22:56:59, sky wrote:
> How come you reset value here?

I added a comment.

[sky, 2009-10-23 00:00:28]

> http://codereview.chromium.org/315008/diff/3008/4038#newcode195
> Line 195: void NetworkLibrary::CheckNetworkTraffic() {
> On 2009/10/22 22:56:59, sky wrote:
>>
>> How often is this called? It seems like a lot. Is it going to impact
>
> performance
>>
>> poorly? For example, could you end up spawning a ton of tasks on the
>
> ui thread?
>
> Yeah, it gets called a lot. What's a better way? Is there a way to check
> tasks on the UI thread?

You should throttle then, perhaps one message a second at most.

  -Scott

[Charlie Lee (do not use), 2009-10-23 22:49:12]

Added network traffic notification throttling.

[sky, 2009-10-23 23:33:32]

http://codereview.chromium.org/315008/diff/14006/15022
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/14006/15022#newcode205
Line 205: bool downloading = false;
Rather than using two bools here, can you directly use traffic_type_? You should
also have a check first thing to see if traffic_type_ == DOWNLOAD|UPLOAD and if
so return.

http://codereview.chromium.org/315008/diff/14006/15022#newcode212
Line 212: if (job->GetByteReadCount() > 0)
Only check these if traffic_type_ & XXX == 0

http://codereview.chromium.org/315008/diff/14006/15022#newcode214
Line 214: if (job->GetUploadProgress() > 0)
I don't know much about this method. If an upload stalls out does this still
return > 0?

http://codereview.chromium.org/315008/diff/14006/15023
File chrome/browser/chromeos/network_library.h (right):

http://codereview.chromium.org/315008/diff/14006/15023#newcode59
Line 59: typedef enum {
typedef enum -> enum TrafficTypes

http://codereview.chromium.org/315008/diff/14006/15023#newcode128
Line 128: // If there is network traffic then notify all listeners.
notify -> start timer that invokes NetworkTrafficTimerFired.

[Charlie Lee (do not use), 2009-10-27 00:01:59]

Ricardo, can you review my use of URLRequestJobTracker?

http://codereview.chromium.org/315008/diff/14006/15022
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/14006/15022#newcode205
Line 205: bool downloading = false;
On 2009/10/23 23:33:32, sky wrote:
> Rather than using two bools here, can you directly use traffic_type_? You
should
> also have a check first thing to see if traffic_type_ == DOWNLOAD|UPLOAD and
if
> so return.

Done.

http://codereview.chromium.org/315008/diff/14006/15022#newcode212
Line 212: if (job->GetByteReadCount() > 0)
On 2009/10/23 23:33:32, sky wrote:
> Only check these if traffic_type_ & XXX == 0

Done.

http://codereview.chromium.org/315008/diff/14006/15022#newcode214
Line 214: if (job->GetUploadProgress() > 0)
On 2009/10/23 23:33:32, sky wrote:
> I don't know much about this method. If an upload stalls out does this still
> return > 0?

Done.

http://codereview.chromium.org/315008/diff/14006/15023
File chrome/browser/chromeos/network_library.h (right):

http://codereview.chromium.org/315008/diff/14006/15023#newcode59
Line 59: typedef enum {
On 2009/10/23 23:33:32, sky wrote:
> typedef enum -> enum TrafficTypes 

Done.

http://codereview.chromium.org/315008/diff/14006/15023#newcode128
Line 128: // If there is network traffic then notify all listeners.
On 2009/10/23 23:33:32, sky wrote:
> notify -> start timer that invokes NetworkTrafficTimerFired.

Done.

[sky, 2009-10-27 00:19:19]

LGTM

[rvargas (doing something else), 2009-10-27 02:06:36]

http://codereview.chromium.org/315008/diff/14018/15050
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/14018/15050#newcode210
Line 210: for (it = g_url_request_job_tracker.begin();
It's sad to walk the list of jobs with each notification. Maybe we should
consider having some equivalent of OnBytesRead for uploads so that there is no
need to do query the jobs, and we only set the traffic_type_ for those two
callbacks. I guess that would not be part of this cl.

http://codereview.chromium.org/315008/diff/14018/15050#newcode212
Line 212: it++) {
use preincrement.

http://codereview.chromium.org/315008/diff/14018/15050#newcode215
Line 215: job->GetByteReadCount() > 0)
I don't think you should be using this method. It is intended only for filters,
and you already received a notification of bytes read.

http://codereview.chromium.org/315008/diff/14018/15050#newcode218
Line 218: job->GetUploadProgress() > 0)
if there is no progress with the up/download, you will still be firing
notifications (both counters are monotonic)

[DaveMoore, 2009-10-27 16:54:57]

The button changes LGTM. Others have commented on the rest.

[Charlie Lee (do not use), 2009-10-28 00:54:22]

http://codereview.chromium.org/315008/diff/14018/15050
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/14018/15050#newcode210
Line 210: for (it = g_url_request_job_tracker.begin();
On 2009/10/27 02:06:36, rvargas wrote:
> It's sad to walk the list of jobs with each notification. Maybe we should
> consider having some equivalent of OnBytesRead for uploads so that there is no
> need to do query the jobs, and we only set the traffic_type_ for those two
> callbacks. I guess that would not be part of this cl.

Is this something you can work on? If not, who?

http://codereview.chromium.org/315008/diff/14018/15050#newcode212
Line 212: it++) {
On 2009/10/27 02:06:36, rvargas wrote:
> use preincrement.

Done.

http://codereview.chromium.org/315008/diff/14018/15050#newcode215
Line 215: job->GetByteReadCount() > 0)
On 2009/10/27 02:06:36, rvargas wrote:
> I don't think you should be using this method. It is intended only for
filters,
> and you already received a notification of bytes read.

Done.

http://codereview.chromium.org/315008/diff/14018/15050#newcode218
Line 218: job->GetUploadProgress() > 0)
On 2009/10/27 02:06:36, rvargas wrote:
> if there is no progress with the up/download, you will still be firing
> notifications (both counters are monotonic)

This is ok for now. Once we have the upload callback, we should be good.

[rvargas (doing something else), 2009-10-28 01:21:15]

LGTM after removing the extra check.

http://codereview.chromium.org/315008/diff/14018/15050
File chrome/browser/chromeos/network_library.cc (right):

http://codereview.chromium.org/315008/diff/14018/15050#newcode210
Line 210: for (it = g_url_request_job_tracker.begin();
> Is this something you can work on? If not, who?

I guess that opening a bug would be the first step. :) cc me.

http://codereview.chromium.org/315008/diff/21002/28016
File chrome/browser/chromeos/cros_network_library.cc (right):

http://codereview.chromium.org/315008/diff/21002/28016#newcode214
Line 214: if (traffic_type_ == (Observer::TRAFFIC_DOWNLOAD |
Now we don't have to do this. Just break with the first upload.