Bug 41592

Summary: ProgressTracker looks not smart
Product: WebKit Reporter: Ryuan Choi <bunhere>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Enhancement CC: cmarcelo, gyuyoung.kim
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
my test log
none
Patch abarth: review-

Description Ryuan Choi 2010-07-05 01:24:32 PDT
I checked ProgressTracker because I can't get fully loaded contents in several web page.
I realized that those site had bad resource which takes very long times.

but in this case,
ProgressTracker also wasn't updated well.

attachment is my test log using GtkLauncher.
as you can see, series of finishedCallback were called without got-chunk callback sometimes.
so, incrementProgress wasn't called

After simply adding incrementProgress in ResourceLoadNotifier::dispatchDidFinishLoading,
I could get more accurate progress.

but, I don't know whether it's right approach or not.
Comment 1 Ryuan Choi 2010-07-05 01:25:32 PDT
Created attachment 60501 [details]
my test log
Comment 2 Ryuan Choi 2010-07-07 01:49:53 PDT
Created attachment 60705 [details]
Patch
Comment 3 Adam Barth 2010-07-07 02:30:27 PDT
Comment on attachment 60705 [details]
Patch

Thanks for the patch, but a number of things are unclear to me:

WebCore/loader/ProgressTracker.cpp:184
 +      if (frame->loader()->client())
Do we need to null-check the client here?  Is the client disappearing somehow?

WebCore/loader/ProgressTracker.cpp: 
 +      // FIXME: Can this ever happen?
Did we resolve this FIXME?  It might be worth explaining when item might be NULL.

In general, we would like to see a test with each patch.  Is there a way to test this code change?  I'm not 100% sure this is the right direction.  It's difficult to tell from this bug report and from the ChangeLog exactly what problem we're solving here.  It's also unclear to me how this change effects all the different clients of resource load notifier.
Comment 4 Ryuan Choi 2010-07-07 19:27:24 PDT
Thank you for your interesting

(In reply to comment #3)
> (From update of attachment 60705 [details])
> Thanks for the patch, but a number of things are unclear to me:
> 
> WebCore/loader/ProgressTracker.cpp:184
>  +      if (frame->loader()->client())
> Do we need to null-check the client here?  Is the client disappearing somehow?
> 
It's my mistake. I'll remove it.

> WebCore/loader/ProgressTracker.cpp: 
>  +      // FIXME: Can this ever happen?
> Did we resolve this FIXME?  It might be worth explaining when item might be NULL.
> 
this patch make a case which item was null.
so I moved this check logic like below
-    item->bytesReceived += bytesReceived;
-    if (item->bytesReceived > item->estimatedLength) {
-        m_totalPageAndResourceBytesToLoad += ((item->bytesReceived * 2) - item->estimatedLength);
-        item->estimatedLength = item->bytesReceived * 2;
+
+    if (item) {
+        item->bytesReceived += bytesReceived;
+        if (item->bytesReceived > item->estimatedLength) {
+            m_totalPageAndResourceBytesToLoad += ((item->bytesReceived * 2) - item->estimatedLength);
+            item->estimatedLength = item->bytesReceived * 2;
+        }
     }

> In general, we would like to see a test with each patch.  Is there a way to test this code change?  I'm not 100% sure this is the right direction.  It's difficult to tell from this bug report and from the ChangeLog exactly what problem we're solving here.  It's also unclear to me how this change effects all the different clients of resource load notifier.

you are right, but I don't know how to make proper test case.
In almost other network situation, this wasn't happened.
especially I am testing wifi through slow proxy server.

anyway I'll prepare some video file and more log for you.
If I am possible, I can show you this issues using android browser (based on proyo) in my test environment.