Closed
Bug 398824
Opened 17 years ago
Closed 17 years ago
smooth out status notifications
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: sayrer, Assigned: sayrer)
References
(Depends on 1 open bug)
Details
(Keywords: perf)
Attachments
(2 files, 1 obsolete file)
9.96 KB,
patch
|
mconnor
:
superreview+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
83.78 KB,
image/png
|
Details |
We send a lot of notifications for status text and progress bars. With the rise of Web 2.0, many sites rely on several so-called widgets to populate a page. This makes our progress go backwards. On techcrunch.com, our status bar changes directions 5 or 6 times. This patch decreases notification frequency, sends status text only when there is new text, and prevents status bars from moving backwards.
Assignee | ||
Comment 1•17 years ago
|
||
Assignee: nobody → sayrer
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Attachment #283812 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•17 years ago
|
Attachment #283812 -
Flags: superreview?(mconnor)
Comment 2•17 years ago
|
||
Comment on attachment 283812 [details] [diff] [review] smooth out status notifications why is mCurrentPercentage a 64-bit integer? you added some tabs to the cpp file + nsString statusMsg = nsDependentString(aMessage); + if (!statusMsg.Equals(mCurrentStatusMsg)) { why not just: if (mCurrentStatusMsg.Equals(aMessage)) + mStatusIsDirty = PR_TRUE; + mStatusMsg = statusMsg; indentation here is 4 spaces, not two... also here: + MaybeSendStatus(); + StartDelayTimer(); + double progress = 0; + LL_L2D(progress, mCurProgress); you don't have to use those macros anymore, you can just do: double progress = mCurProgress; though it seems like you don't need the temporary variable then + if (percentage > (mCurrentPercentage + 3)) { some sort of explanation about the significance of 3 would be nice
Attachment #283812 -
Flags: review?(cbiesinger) → review+
Comment 3•17 years ago
|
||
if you could get jag to sr this that'd be great
Assignee | ||
Comment 4•17 years ago
|
||
Additional review from jag is welcome, looking for mconnor's approval on the behavior changes.
Attachment #283812 -
Attachment is obsolete: true
Attachment #283855 -
Flags: superreview?(mconnor)
Attachment #283812 -
Flags: superreview?(mconnor)
Comment 5•17 years ago
|
||
Comment on attachment 283855 [details] [diff] [review] address biesi's comments sr=mconnor, let's try this and see what happens with perf.
Attachment #283855 -
Flags: superreview?(mconnor)
Attachment #283855 -
Flags: superreview+
Attachment #283855 -
Flags: approval1.9+
Comment 6•17 years ago
|
||
Comment on attachment 283855 [details] [diff] [review] address biesi's comments >Index: xpfe/browser/src/nsBrowserStatusFilter.cpp >+nsBrowserStatusFilter::MaybeSendProgress() >+ // The progress meter only updates for increases greater than 3 percent I think it would be a good idea to mention that this behavior is currently defined in progressmeter.xml, and that we're optimizing for the listener that updates the browser UI's progressmeter (this filter can used with other listeners, e.g. ones added by an extension via tabbrowser.addProgressListener).
Updated•17 years ago
|
Assignee | ||
Comment 7•17 years ago
|
||
I tried this patch with timeouts of 360ms 320ms 280ms 240ms 200ms and only 200ms didn't feel like an obvious slowdown. However, we are rendering this text using the high-quality path. We might be able to get away with 200 or 240 if we could write the status text faster, but XUL labels currently don't obey the CSS rules for this.
Comment 8•17 years ago
|
||
(In reply to comment #7) > and only 200ms didn't feel like an obvious slowdown. However, we are rendering > this text using the high-quality path. We might be able to get away with 200 or > 240 if we could write the status text faster, but XUL labels currently don't > obey the CSS rules for this. Bug on that, blocking this one? /be
Assignee | ||
Comment 9•17 years ago
|
||
This is from qm-mini-xp02, available on the MozillaTest tinderbox. The main boxes seem a little noisy, but showing improvements as well.
Yeah, I should be able to fix that fairly easily, but I don't know that you'll see much speedup. The text run cache should be doing a superb job here.
Assignee | ||
Comment 11•17 years ago
|
||
(In reply to comment #10) > Yeah, I should be able to fix that fairly easily, but I don't know that you'll > see much speedup. The text run cache should be doing a superb job here. After discussing with roc, this sounds pretty likely. Thinking more about the status text updates, I think the problem with slower ones is that they get noticeably rhythmic. If we take this further, we should probably implement the progress meter in C++ at 120ms or so, and vary the frequency of status text updates.
Could you resolve this bug and file a new bug for any remaining work/issues?
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•