Closed Bug 165039 Opened 18 years ago Closed 18 years ago

improve user interactivity during long page loads - Interruptible parsing needs better user input detection

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: kmcclusk, Assigned: kmcclusk)

References

Details

(Keywords: perf, Whiteboard: [adt3])

Attachments

(1 file, 6 obsolete files)

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.
Keywords: nsbeta1+
Priority: -- → P1
Whiteboard: [adt3]
Target Milestone: --- → mozilla1.2beta
Note: This patch must be hand-applied. It also requires the patch in bug 157144
be checked in first.
Depends on: 157144
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.
Attachment #97099 - Attachment is obsolete: true
Attachment #97125 - Attachment is obsolete: true
Keywords: perf
Summary: Interruptible parsing needs better user input detection → improve user interactivity during long page loads - Interruptible parsing needs better user input detection
With attachment 97865 [details] [diff] [review], I experienced very smooth UI responsiveness while large
documents were loading.
the patch seems cross platform, 
  Is this bug for windows only?
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.
Blocks: 166758
I see.
do you have plan to add more code about this for linux ?
No longer blocks: 166758
I will file separate bugs for Linux and Mac. 
Blocks: 91643
Attachment #97865 - Attachment is obsolete: true
Attachment #100593 - Attachment is obsolete: true
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
Attachment #100721 - Flags: review+
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 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))) {
I was planning on getting rid of the view manager's GetLastUserEventTime()
method after all of the platforms supported nsIWidget::GetLastInputEventTime.
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))) {
Attachment #100957 - Flags: superreview+
Attachment #100957 - Flags: review+
Fix checked in to the trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
What are the Mac and Linux specific bugs for this?
bug 172499 for Linux
bug 172501 for Mac
QA Contact: rakeshmishra → trix
verified on latest trunk build 2002122708
Status: RESOLVED → VERIFIED
Kevin McCluskey wrote:
> bug 172499 for Linux
> bug 172501 for Mac

Did you file bugs for the Xlib, BeOS and Photon ports, too ?
OS/2 - bug 194571
BeOS - bug 194572


XLIB bug 194623

How do I file bugs against Photon? 
Do I file against OS:Neutrino?
Does this patch helps to, for example, create new tab/window faster on page load?
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.
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;
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.