prefetch categorization is exactly wrong

• Component: Page Loading
• Status: RESOLVED FIXED

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 62093 [details]
proposed fix to prefetching

[Adam Barth]

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.

[Gavin Peters]

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

[WebKit Review Bot]

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.

[Gavin Peters]

Created attachment 62118 [details]
now sans tabs

[Gavin Peters]

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.

[Darin Adler]

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

[Darin Adler]

(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.

[Gavin Peters]

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.

[Adam Barth]

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

[WebKit Commit Bot]

Comment on attachment 62125 [details]
remediate to Darin's review

Clearing flags on attachment: 62125

Committed r63792: <http://trac.webkit.org/changeset/63792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.