Bug 42651 - prefetch categorization is exactly wrong
Summary: prefetch categorization is exactly wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-20 10:30 PDT by Gavin Peters
Modified: 2010-07-20 17:26 PDT (History)
5 users (show)

See Also:


Attachments
proposed fix to prefetching (1.11 KB, patch)
2010-07-20 11:23 PDT, Gavin Peters
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
now with a test (54.76 KB, patch)
2010-07-20 15:01 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
now sans tabs (54.78 KB, patch)
2010-07-20 15:05 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
don't lie about dumprendertree (54.71 KB, patch)
2010-07-20 15:08 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
remediate to Darin's review (55.54 KB, patch)
2010-07-20 15:34 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2010-07-20 10:30:57 PDT
The implementation of CachedResource::isPrefetch() is exactly backwords; all non-prefetches are reported as prefetches, and all prefetches are returned as non-prefetches.  This has been causing the DocLoader to get confused, and resources to not always be available at page onload, etc...  In chrome this manifest as a failure in a browser test if prefetching was enabled.

The comparison in this header file should be reversed.

I have a patch, I'm preparing it and will upload it later today.
Comment 1 Gavin Peters 2010-07-20 11:23:56 PDT
Created attachment 62093 [details]
proposed fix to prefetching
Comment 2 Adam Barth 2010-07-20 11:26:12 PDT
Comment on attachment 62093 [details]
proposed fix to prefetching

Ugg...  I'm embarrassed that I missed this in the code review.

WebCore/ChangeLog:8
 +          No new tests. (OOPS!)
Can we test this issue?  In case, we can't land the patch with the OOPS here.
Comment 3 Gavin Peters 2010-07-20 15:01:15 PDT
Created attachment 62117 [details]
now with a test

I have now created a test (onload events issued early in the buggy version, so this test just looks at the x,y of a contained image from a page at the time of onload).
Comment 4 WebKit Review Bot 2010-07-20 15:03:14 PDT
Attachment 62117 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Gavin Peters 2010-07-20 15:05:07 PDT
Created attachment 62118 [details]
now sans tabs
Comment 6 Gavin Peters 2010-07-20 15:08:22 PDT
Created attachment 62120 [details]
don't lie about dumprendertree

abarth reminded me this test runs fine in a browser, so don't say it won't.
Comment 7 Darin Adler 2010-07-20 15:10:03 PDT
Comment on attachment 62118 [details]
now sans tabs

Why does the test say it requires DumpRenderTree? I don’t see anything in the test that requires it. Why doesn’t the test explain what success looks like and what failure looks like?

A good test explains what it tests, and makes failure vs. success clear.

Fix is fine, although I'd prefer to make the #if even cleaner by putting the body outside the class definition:

    inline bool CachedResource::isPrefetch() const
    {
    #if ENABLE(LINK_PREFETCH)
        return type() == LinkPrefetch;
    #else
        return false;
    #endif
    }

Then the class definition will be easier to read.

review- just because it seems the test could be much clearer
Comment 8 Darin Adler 2010-07-20 15:10:47 PDT
(In reply to comment #7)
> Why does the test say it requires DumpRenderTree? I don’t see anything in the test that requires it.

Good, you fixed that.

> Why doesn’t the test explain what success looks like and what failure looks like?
> 
> A good test explains what it tests, and makes failure vs. success clear.

Please consider fixing this too.
Comment 9 Gavin Peters 2010-07-20 15:34:18 PDT
Created attachment 62125 [details]
remediate to Darin's review

Darin,

Thanks for the review.  You were right that the test was too obtuse; the new version explains what it is and reports its result in a much more user friendly way. 

I've also reformatted the header per your advice.
Comment 10 Adam Barth 2010-07-20 15:40:47 PDT
Comment on attachment 62125 [details]
remediate to Darin's review

The test could still be more beautiful (e.g., properly indented, etc), but I think this is fine.  (In general, I like diversity in the tests because you get more coverage of things you don't expect.)
Comment 11 WebKit Commit Bot 2010-07-20 17:26:20 PDT
Comment on attachment 62125 [details]
remediate to Darin's review

Clearing flags on attachment: 62125

Committed r63792: <http://trac.webkit.org/changeset/63792>
Comment 12 WebKit Commit Bot 2010-07-20 17:26:27 PDT
All reviewed patches have been landed.  Closing bug.