Implemented upload progress indicator in status bar.

Implemented upload progress indicator in status bar.
When sending an HTTP POST request with sufficiently large payload data (such as uploading a file or several), the status bar should display the progress in the form of a percentage. The ResourceDispatcherHost periodically sends upload progress messages to the UI thread as well as to the renderer. If an upload is occurring, other potential LoadStateChanged messages to the same RenderViewHost are ignored; this prevents the upload progress display from being interrupted periodically by "Waiting for <site>" messages.

Original patch by simonrad@chromium.org
r=darin@chromium.org
BUG=4715
TEST=Try uploading one or more files (e.g. at a file hosting site) and confirm that the percentage is shown in the status bar.

[darin (slow to review), 2009-09-23 07:36:50]

http://codereview.chromium.org/211050/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/211050/diff/1/2#newcode2831
Line 2831: Sending request... (uploading <ph
name="PROGRESS_PERCENT">$1<ex>99</ex></ph>% complete)
I believe that the UI Glen wanted was:

  "Sending request (XX%)..."

You should probably double-check with him (glen@chromium.org).

http://codereview.chromium.org/211050/diff/1/9
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/211050/diff/1/9#newcode382
Line 382: uint64 upload_size = 0, uint64 upload_position = 0);
nit: the google c++ style guide does not permit default values in parameter
lists.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

nit: it is more common when listing arguments for an offset 'position' in a
stream of length 'size' to list the offset before the length.  that way it reads
more like a fraction: x, y => x / y.

http://codereview.chromium.org/211050/diff/1/6
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/211050/diff/1/6#newcode1623
Line 1623: std::pair<int, int> key(info->child_id(), info->route_id());
nit: the initialization of key should be moved down to where it is first needed.

http://codereview.chromium.org/211050/diff/1/6#newcode1625
Line 1625: if (load_state != net::LOAD_STATE_SENDING_REQUEST)
nit: it might help readability to just call request->GetLoadState() here instead
of creating a temporary for a single use.

http://codereview.chromium.org/211050/diff/1/6#newcode1626
Line 1626: upload_size = 0;
can you avoid populating the map in this case?  (if we're not sending for this
view, then there is probably no need to insert upload size into the map for this
view.)

http://codereview.chromium.org/211050/diff/1/6#newcode1665
Line 1665: load_info.upload_position = request->GetUploadProgress();
hmm... GetUploadProgress was already called within MaybeUpdateUploadProgress.

it would be better to only call it once in case its value changes with each
call.

http://codereview.chromium.org/211050/diff/1/6#newcode1681
Line 1681: // Sends upload progress IPC messages to the renderer.
nit: this function doesn't actually send any IPCs.  it calls out to the
ResourceHandler, which may itself send IPCs.  the comment here might be
confusing to folks who don't see any IPCs being sent in _this_ function.

[Simon Radford, 2009-09-25 02:47:02]

New patch uploaded. Fixed all comments except one. Using "Uploading (xx%)..." as
the UI text, waiting on confirmation.

http://codereview.chromium.org/211050/diff/1/6
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/211050/diff/1/6#newcode1665
Line 1665: load_info.upload_position = request->GetUploadProgress();
On 2009/09/23 07:36:50, darin wrote:
> hmm... GetUploadProgress was already called within MaybeUpdateUploadProgress.
> 
> it would be better to only call it once in case its value changes with each
> call.

As we discussed, it's probably not an issue if the value changes between calls.
GetUploadProgress also seems pretty lightweight.

[darin (slow to review), 2009-09-25 03:52:06]

LGTM

[darin (slow to review), 2009-09-25 04:24:39]

Committed as r27169 :-)

On Thu, Sep 24, 2009 at 8:52 PM, <darin@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/211050
>