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

VERIFIED FIXED in mozilla1.2beta

Status

()

Core
Event Handling
P1
normal
VERIFIED FIXED
15 years ago
10 years ago

People

(Reporter: Kevin McCluskey (gone), Assigned: Kevin McCluskey (gone))

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.2beta
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Updated

15 years ago
Keywords: nsbeta1+
Priority: -- → P1
Whiteboard: [adt3]
Target Milestone: --- → mozilla1.2beta
(Assignee)

Comment 1

15 years ago
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.
(Assignee)

Updated

15 years ago
Depends on: 157144
(Assignee)

Comment 2

15 years ago
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.
(Assignee)

Comment 3

15 years ago
Created attachment 97865 [details] [diff] [review]
improved patch which will apply to the trunk
Attachment #97099 - Attachment is obsolete: true
Attachment #97125 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Keywords: perf
Summary: Interruptible parsing needs better user input detection → improve user interactivity during long page loads - Interruptible parsing needs better user input detection

Comment 4

15 years ago
With attachment 97865 [details] [diff] [review], I experienced very smooth UI responsiveness while large
documents were loading.

Comment 5

15 years ago
the patch seems cross platform, 
  Is this bug for windows only?
(Assignee)

Comment 6

15 years ago
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.

Updated

15 years ago
Blocks: 166758

Comment 7

15 years ago
I see.
do you have plan to add more code about this for linux ?
No longer blocks: 166758
(Assignee)

Comment 8

15 years ago
I will file separate bugs for Linux and Mac. 

Updated

15 years ago
Blocks: 91643
(Assignee)

Comment 9

15 years ago
Created attachment 100593 [details] [diff] [review]
Improved patch. Moves the platform dependent code into nsIWidget
Attachment #97865 - Attachment is obsolete: true
(Assignee)

Comment 10

15 years ago
Created attachment 100721 [details] [diff] [review]
Same as last patch - synchronized with the trunk
Attachment #100593 - Attachment is obsolete: true

Comment 11

15 years ago
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+
(Assignee)

Comment 12

15 years ago
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.
Attachment #100721 - Attachment is obsolete: true

Comment 13

15 years ago
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

15 years ago
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))) {
(Assignee)

Comment 15

15 years ago
I was planning on getting rid of the view manager's GetLastUserEventTime()
method after all of the platforms supported nsIWidget::GetLastInputEventTime.

Comment 16

15 years ago
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+
(Assignee)

Comment 17

15 years ago
Created attachment 101180 [details] [diff] [review]
patch with Kin's suggestion + synchronized with the current trunk
Attachment #100957 - Attachment is obsolete: true
(Assignee)

Comment 18

15 years ago
Fix checked in to the trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 19

15 years ago
What are the Mac and Linux specific bugs for this?
(Assignee)

Comment 20

15 years ago
bug 172499 for Linux
bug 172501 for Mac

Updated

15 years ago
QA Contact: rakeshmishra → trix

Comment 21

15 years ago
verified on latest trunk build 2002122708
Status: RESOLVED → VERIFIED

Comment 22

15 years ago
Kevin McCluskey wrote:
> bug 172499 for Linux
> bug 172501 for Mac

Did you file bugs for the Xlib, BeOS and Photon ports, too ?
(Assignee)

Comment 23

15 years ago
OS/2 - bug 194571
BeOS - bug 194572


(Assignee)

Comment 24

15 years ago
XLIB bug 194623

(Assignee)

Comment 25

15 years ago
How do I file bugs against Photon? 
(Assignee)

Comment 26

15 years ago
Do I file against OS:Neutrino?

Comment 27

15 years ago
Does this patch helps to, for example, create new tab/window faster on page load?
(Assignee)

Comment 28

15 years ago
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

15 years ago
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;
You need to log in before you can comment on or make changes to this bug.