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

Status: VERIFIED FIXED

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

Description

[Kevin McCluskey (gone)]

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)]

Attachment: 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)]

Attachment: 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)]

Attachment: improved patch which will apply to the trunk

Comment 4

[Peter Lubczynski]

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

Comment 5

[Jerry]

the patch seems cross platform, 
  Is this bug for windows only?

Comment 6

[Kevin McCluskey (gone)]

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]

I see.
do you have plan to add more code about this for linux ?

Comment 8

[Kevin McCluskey (gone)]

I will file separate bugs for Linux and Mac. 

Comment 9

[Kevin McCluskey (gone)]

Attachment: Improved patch. Moves the platform dependent code into nsIWidget

Comment 10

[Kevin McCluskey (gone)]

Attachment: Same as last patch - synchronized with the trunk

Comment 11

[rods (gone)]

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)]

Attachment: 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)]

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]

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)]

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

Comment 16

[kinmoz]

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=[e-mail]

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)]

Attachment: patch with Kin's suggestion + synchronized with the current trunk

Comment 18

[Kevin McCluskey (gone)]

Fix checked in to the trunk.

Comment 19

[nnooiissee]

What are the Mac and Linux specific bugs for this?

Comment 20

[Kevin McCluskey (gone)]

bug 172499 for Linux
bug 172501 for Mac

Comment 21

[Trix Supremo]

verified on latest trunk build 2002122708

Comment 22

[Roland Mainz]

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)]

OS/2 - bug 194571
BeOS - bug 194572

Comment 24

[Kevin McCluskey (gone)]

XLIB bug 194623

Comment 25

[Kevin McCluskey (gone)]

How do I file bugs against Photon? 

Comment 26

[Kevin McCluskey (gone)]

Do I file against OS:Neutrino?

Comment 27

[Sergei Dolgov]

Does this patch helps to, for example, create new tab/window faster on page load?

Comment 28

[Kevin McCluskey (gone)]

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]

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;