Last Comment Bug 165039 - improve user interactivity during long page loads - Interruptible parsing needs better user input detection
: improve user interactivity during long page loads - Interruptible parsing nee...
Status: VERIFIED FIXED
[adt3]
: perf
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: mozilla1.2beta
Assigned To: Kevin McCluskey (gone)
: Trix Supremo
: Andrew Overholt [:overholt]
Mentors:
Depends on: 157144
Blocks: 91643
  Show dependency treegraph
 
Reported: 2002-08-27 16:22 PDT by Kevin McCluskey (gone)
Modified: 2007-12-10 12:09 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch which looks and pending native WIN32 message queue as well as the last input time in nsViewManager.cpp to switch the parser into low-frequency interactive mode (1.86 KB, patch)
2002-08-28 16:23 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
Changes to content sink to improve page load performance when user is interacting with the UI or page during the load (941 bytes, patch)
2002-08-28 21:29 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
improved patch which will apply to the trunk (3.09 KB, patch)
2002-09-04 16:48 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
Improved patch. Moves the platform dependent code into nsIWidget (8.12 KB, patch)
2002-09-25 11:37 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
Same as last patch - synchronized with the trunk (8.14 KB, patch)
2002-09-26 07:34 PDT, Kevin McCluskey (gone)
rods: review+
Details | Diff | Splinter Review
improved patch which moves logic for hooking for the window move notification from plevent.c into the widget module. Also changes PL_FavorPerformanceHint so that calls to it can be nested. (16.38 KB, patch)
2002-09-27 18:16 PDT, Kevin McCluskey (gone)
kinmoz: review+
kinmoz: superreview+
Details | Diff | Splinter Review
patch with Kin's suggestion + synchronized with the current trunk (16.42 KB, patch)
2002-09-30 16:08 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review

Description Kevin McCluskey (gone) 2002-08-27 16:22:15 PDT
On WIN32, when the user moves the window or clicks on menu items in an embedded
application the parser is not interrupted frequently enough. This can be seen
when loading a large local. If you move the window during the page load it is
sluggish and jumps to the new location instead of dragging smoothly. Dragging
over menu items in an embedded application (use MfcEmbed as a test) is also
sluggish.

This is because the logic to determine when the user has interacted with Gecko
is based on stamping the time that an event arrives within the ViewManager. The
logic needs to be improved to include the detection of events outside the Gecko
window.
Comment 1 Kevin McCluskey (gone) 2002-08-28 16:23:36 PDT
Created attachment 97099 [details] [diff] [review]
Patch which  looks and pending native WIN32 message queue as well as the last input time in nsViewManager.cpp to switch the parser into low-frequency interactive mode

Note: This patch must be hand-applied. It also requires the patch in bug 157144
be checked in first.
Comment 2 Kevin McCluskey (gone) 2002-08-28 21:29:38 PDT
Created attachment 97125 [details] [diff] [review]
Changes to content sink to improve page load performance when user is interacting with the UI or page during the load

With attachment 97099 [details] [diff] [review] and other improvements we will be able to increase the
content update interval when the user is interacting with the UI/page during a
long page load.
Comment 3 Kevin McCluskey (gone) 2002-09-04 16:48:29 PDT
Created attachment 97865 [details] [diff] [review]
improved patch which will apply to the trunk
Comment 4 Peter Lubczynski 2002-09-04 17:33:07 PDT
With attachment 97865 [details] [diff] [review], I experienced very smooth UI responsiveness while large
documents were loading.
Comment 5 Jerry 2002-09-04 23:51:42 PDT
the patch seems cross platform, 
  Is this bug for windows only?
Comment 6 Kevin McCluskey (gone) 2002-09-05 15:53:22 PDT
The patch will only affect WIN32. See:

PR_IMPLEMENT(PRBool)
PL_IsNativeEventPending()

For the patch to have an effect on Linux or Mac PL_IsNativeEventPending() would
need to have additional code inserted to determine if there is a pending native
event on that platform.
Comment 7 Jerry 2002-09-09 01:50:28 PDT
I see.
do you have plan to add more code about this for linux ?
Comment 8 Kevin McCluskey (gone) 2002-09-09 16:10:53 PDT
I will file separate bugs for Linux and Mac. 
Comment 9 Kevin McCluskey (gone) 2002-09-25 11:37:51 PDT
Created attachment 100593 [details] [diff] [review]
Improved patch. Moves the platform dependent code into nsIWidget
Comment 10 Kevin McCluskey (gone) 2002-09-26 07:34:30 PDT
Created attachment 100721 [details] [diff] [review]
Same as last patch - synchronized with the trunk
Comment 11 rods (gone) 2002-09-26 13:52:28 PDT
Comment on attachment 100721 [details] [diff] [review]
Same as last patch - synchronized with the trunk

Just a nit:
NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_FAILURE);
can be replaced with 
NS_ENSURE_SUCCESS(rv, rv);
r=rods
Comment 12 Kevin McCluskey (gone) 2002-09-27 18:16:51 PDT
Created attachment 100957 [details] [diff] [review]
improved patch which moves logic for hooking for the window move notification from plevent.c into the widget module. Also changes PL_FavorPerformanceHint so that calls to it can be nested.
Comment 13 rods (gone) 2002-09-28 05:09:48 PDT
Comment on attachment 100957 [details] [diff] [review]
improved patch which moves logic for hooking for the window move notification from plevent.c into the widget module. Also changes PL_FavorPerformanceHint so that calls to it can be nested.

r=rods
Comment 14 kinmoz 2002-09-30 12:47:49 PDT
Comment on attachment 100957 [details] [diff] [review]
improved patch which moves logic for hooking for the window move notification from plevent.c into the widget module. Also changes PL_FavorPerformanceHint so that calls to it can be nested.

==== Rather than make the content sink dependent on widget stuff, should this
code be in the view manager's GetLastUserEventTime() method?


+    nsCOMPtr<nsIWidget> widget;
+    nsresult rv = vm->GetWidget(getter_AddRefs(widget));
+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_FAILURE);
+    if (! NS_SUCCEEDED(widget->GetLastInputEventTime(eventTime))) {
+	 // If we can't get the last input time from the widget
+	 // then we will get it from the viewmanager.
+	 rv = vm->GetLastUserEventTime(eventTime);
+	 NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_FAILURE);
+    }
+


==== Use |NS_FAILED()| instead of |!NS_SUCCEEDED()|:


+    if (! NS_SUCCEEDED(widget->GetLastInputEventTime(eventTime))) {
Comment 15 Kevin McCluskey (gone) 2002-09-30 12:58:38 PDT
I was planning on getting rid of the view manager's GetLastUserEventTime()
method after all of the platforms supported nsIWidget::GetLastInputEventTime.
Comment 16 kinmoz 2002-09-30 15:23:11 PDT
Comment on attachment 100957 [details] [diff] [review]
improved patch which moves logic for hooking for the window move notification from plevent.c into the widget module. Also changes PL_FavorPerformanceHint so that calls to it can be nested.

sr=kin@netscape.com

Btw, I noticed that nsViewManager::GetWidget() always returns NS_OK, even if it
returns a null widget, so we might want to check for a null widget before
dereferencing it:

+    nsresult rv = vm->GetWidget(getter_AddRefs(widget));
+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv), NS_ERROR_FAILURE);
+    if (! NS_SUCCEEDED(widget->GetLastInputEventTime(eventTime))) {

Perhaps something like:

+    if (!widget || NS_FAILED(widget->GetLastInputEventTime(eventTime))) {
Comment 17 Kevin McCluskey (gone) 2002-09-30 16:08:08 PDT
Created attachment 101180 [details] [diff] [review]
patch with Kin's suggestion + synchronized with the current trunk
Comment 18 Kevin McCluskey (gone) 2002-09-30 20:17:21 PDT
Fix checked in to the trunk.
Comment 19 nnooiissee 2002-10-03 16:04:54 PDT
What are the Mac and Linux specific bugs for this?
Comment 20 Kevin McCluskey (gone) 2002-10-03 17:25:59 PDT
bug 172499 for Linux
bug 172501 for Mac
Comment 21 Trix Supremo 2002-12-27 12:54:20 PST
verified on latest trunk build 2002122708
Comment 22 Roland Mainz 2003-02-23 10:13:16 PST
Kevin McCluskey wrote:
> bug 172499 for Linux
> bug 172501 for Mac

Did you file bugs for the Xlib, BeOS and Photon ports, too ?
Comment 23 Kevin McCluskey (gone) 2003-02-23 10:21:19 PST
OS/2 - bug 194571
BeOS - bug 194572


Comment 24 Kevin McCluskey (gone) 2003-02-23 10:29:43 PST
XLIB bug 194623

Comment 25 Kevin McCluskey (gone) 2003-02-23 10:39:52 PST
How do I file bugs against Photon? 
Comment 26 Kevin McCluskey (gone) 2003-02-23 10:41:25 PST
Do I file against OS:Neutrino?
Comment 27 Sergei Dolgov 2003-02-23 14:03:39 PST
Does this patch helps to, for example, create new tab/window faster on page load?
Comment 28 Kevin McCluskey (gone) 2003-02-23 20:26:55 PST
This only affects interacting with Mozilla while it is loading page. This is
most commonly seen when loading a very large page, then opening a new tab. So if
you are seeing slow performance loading a new tab while another page is still
loading then it should help.  If you are seeing slow performance just opening a
new tab, then this will not help.
Comment 29 Sergei Dolgov 2003-03-03 18:07:58 PST
Only thing i'm wondering now is why in nsIWidget.h in GetLastInputEventTime()
description you use MILLIseconds while it must be compatible with
PR_IntervalToMicroseconds () - MICROseconds.
I'm confused;

Note You need to log in before you can comment on or make changes to this bug.