Bug 32940 – [GTK] Progress notification update taking longer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Estêvão Samuel Procópio Amaral]

The progress notification in WebKit GTK implementation is throttled in order to consume less CPU. Although it's not updating properly. Sometimes it takes a long time to show statuses updates (only working after passing 3% of the download). To some file sizes it's a pain not to know if the download is properly occuring.

[Estêvão Samuel Procópio Amaral]

Created attachment 45500 [details]
Changing the download throttle conditions

This patch is a proposal for changing the download notification throttle.

The changes are quite simple: 
The code only updates lastElapsed when a progress notification occurs. If we passed the lastElapsed by more then 0.7 secs from now, we notify again.
I added another static variable, lastNotifiedProgress, that holds the value of the last notified progress. If we passed more than 1% of the last notified progress, we notify again.

I must state that this patch is not the final one. I think it's important that some of you guys try this patch to see if it's good or if we need to think a better way to do this throttle.

[WebKit Review Bot]

Attachment 45500 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitdownload.cpp:867:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebKit/gtk/webkit/webkitdownload.cpp:868:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2

[Estêvão Samuel Procópio Amaral]

Created attachment 45501 [details]
New version of throttle conditions

Another version of the patch respecting the coding conventions and calling less functions

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45501 [details] without any errors.

[Gustavo Noronha (kov)]

Comment on attachment 45501 [details]
New version of throttle conditions

ok, two more things - I would preffer if you said in the ChangeLog why you are doing the change (i.e., to fix the notification being bumpy), and second, the && operators should be at the end of the previous line, not at the beggining of the next line (I wrote it wrongly at first ;D). THanks.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> (From update of attachment 45501 [details])
> ok, two more things - I would preffer if you said in the ChangeLog why you are
> doing the change (i.e., to fix the notification being bumpy), and second, the
> && operators should be at the end of the previous line, not at the beggining of
> the next line (I wrote it wrongly at first ;D). THanks.

ok, ignore the && one /me hits his head on the keyboard

[Estêvão Samuel Procópio Amaral]

Created attachment 45503 [details]
Conforming with comment #5

[WebKit Commit Bot]

Comment on attachment 45503 [details]
Conforming with comment #5

Rejecting patch 45503 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against bugzilla-tool.
Failed to run "['WebKitTools/Scripts/bugzilla-tool', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--quiet', '45503']" exit_code: 1
Last 500 characters of output:
)
  File "/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/modules/statusbot.py", line 73, in update_status
    response = self.browser.submit()
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 547, in submit
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 209, in open
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 261, in _mech_open
mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

[Gustavo Noronha (kov)]

Comment on attachment 45503 [details]
Conforming with comment #5

Let's see if that was a temporary problem.

[WebKit Commit Bot]

Comment on attachment 45503 [details]
Conforming with comment #5

Rejecting patch 45503 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against bugzilla-tool.
Failed to run "['WebKitTools/Scripts/bugzilla-tool', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--quiet', '45503']" exit_code: 1
Last 500 characters of output:
)
  File "/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/modules/statusbot.py", line 73, in update_status
    response = self.browser.submit()
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 547, in submit
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 209, in open
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 261, in _mech_open
mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

[Adam Barth]

Eric, this is a strange commit-queue message

[Eric Seidel (no email)]

Comment on attachment 45503 [details]
Conforming with comment #5

This is just app engine giving us 500 errors as it does.  We should be catching these, but I expect this is a temporary error.  I'll file a bug about it.

[Eric Seidel (no email)]

Filed bug 32976 about handling the 500 errors better.

[WebKit Commit Bot]

Comment on attachment 45503 [details]
Conforming with comment #5

Rejecting patch 45503 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against bugzilla-tool.
Failed to run "['WebKitTools/Scripts/bugzilla-tool', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--quiet', '45503']" exit_code: 1
Last 500 characters of output:
)
  File "/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/modules/statusbot.py", line 73, in update_status
    response = self.browser.submit()
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 547, in submit
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 209, in open
  File "build/bdist.macosx-10.5-i386/egg/mechanize/_mechanize.py", line 261, in _mech_open
mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

[Eric Seidel (no email)]

Adam suggests the commit-queue might be trying to post a huge log to the server, or that in some way the log it is posting is causing a failure.  I can look at the log on the server, sec.

[Eric Seidel (no email)]

The failure it's actually hitting is:
Failed to run "['svn', 'commit', '-m', '2009-12-27  Est\xc3\xaav\xc3\xa3o Samuel Proc\xc3\xb3pio  <[e-mail]>\n\n        Reviewed by noone (OOPS!).\n\n        Bug 32940: [GTK] Changing the download throttle conditions.\n        https://bugs.webkit.org/show_bug.cgi?id=32716\n\n        The WebKitDownload progress notification was taking long to\n        update. This fix makes notification happens each 0.7 secs\n        or when the progress ups in 1%.\n\n        * WebKit/gtk/webkit/webkitdownload.cpp:\n']" exit_code: 1

I suspect there may be an encoding issue.

[Eric Seidel (no email)]

Bug 26927 and bug 32343 are related to this type of failure.

[Eric Seidel (no email)]

I got my comments out of order, but here:

Oh.  Well, the ChangeLog has also been edited, so svn commit is likely rejecting it due to having OOPS! in the ChangeLog because it couldn't find "NOBODY(OOPS!)" to replace with the reviewer.

So the cq- is correct, this can't be landed via the commit queue.  There also might be encoding failures at play here, not sure.

[Estêvão Samuel Procópio Amaral]

(In reply to comment #18)
> So the cq- is correct, this can't be landed via the commit queue.  There also
> might be encoding failures at play here, not sure.

Well... I'll take a look at the changelog and encoding stuff and submit another patch.
I committed locally before running prepare-Changelog... That's why it's screwed. Sorry for the trouble... :]

[Eric Seidel (no email)]

The only problem with this patch that I can see is that you changed the "Reviewed by" line from the default prepare-ChangeLog template.  You replaced "NOBODY" with "noone".  Yes, it's lame that our scripts depend on that, but for now they do. :(

If you fix the NOBODY bit in the ChangeLog and re-submit a patch then the commit-queue should work just fine.

[Estêvão Samuel Procópio Amaral]

Created attachment 45540 [details]
patch with right ChangeLog

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45540 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45540 [details]
patch with right ChangeLog

Rejecting patch 45540 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against bugzilla-tool.
Failed to run "['WebKitTools/Scripts/bugzilla-tool', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--quiet', '45540']" exit_code: 1
Last 500 characters of output:
134, in fetch_attachments_from_bug
    bug = self.fetch_bug(bug_id)
  File "/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/modules/bugzilla.py", line 130, in fetch_bug
    return self._parse_bug_page(page)
  File "/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/modules/bugzilla.py", line 123, in _parse_bug_page
    bug["attachments"] = [self._parse_attachment_element(element, bug_id) for element in soup.findAll('attachment')]
NameError: global name 'bug_id' is not defined

[Adam Barth]

The rejection is due to a regression Eric introduced last night.  The bug has been fixed but Eric might need to kick the commit-queue.

[Eric Seidel (no email)]

Comment on attachment 45540 [details]
patch with right ChangeLog

Sorry, I broke the commit-queue, but Adam fixed it in http://trac.webkit.org/changeset/52595.  Its working again now.  Apologies for the trouble.

[WebKit Commit Bot]

Comment on attachment 45540 [details]
patch with right ChangeLog

Clearing flags on attachment: 45540

Committed r52597: <http://trac.webkit.org/changeset/52597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.