Bug 110527 - [chromium] Lower priority of preloaded images
Summary: [chromium] Lower priority of preloaded images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Simonsen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-21 16:24 PST by James Simonsen
Modified: 2013-02-27 15:49 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.16 KB, patch)
2013-02-21 16:34 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (26.81 KB, patch)
2013-02-27 15:16 PST, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2013-02-21 16:24:23 PST
[chromium] Lower priority of preloaded images
Comment 1 James Simonsen 2013-02-21 16:34:25 PST
Created attachment 189634 [details]
Patch
Comment 2 James Simonsen 2013-02-21 16:36:02 PST
This is the priority change I chatted with you about yesterday. I had to jump through a lot of hoops just to test that it works as I want. If you know an easier way, let me know. Otherwise, I think think this gives us a good foundation for testing future network prioritization changes.
Comment 3 WebKit Review Bot 2013-02-21 16:36:17 PST
Attachment 189634 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/loading/promote-img-preload-priority-expected.txt', u'LayoutTests/http/tests/loading/promote-img-preload-priority.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/FrameLoaderClient.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResourceLoader.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/public/WebFrameClient.h', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.cpp', u'Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.h', u'Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp']" exit_code: 1
Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:442:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2013-02-21 17:31:30 PST
Comment on attachment 189634 [details]
Patch

Attachment 189634 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16693423
Comment 5 Build Bot 2013-02-22 13:46:03 PST
Comment on attachment 189634 [details]
Patch

Attachment 189634 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16716172
Comment 6 James Simonsen 2013-02-27 13:55:17 PST
Ping? Any thoughts on this? Did I go overboard to test it? Is there someone else who should review it?
Comment 7 Eric Seidel (no email) 2013-02-27 14:20:41 PST
Comment on attachment 189634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189634&action=review

Seems reasonable, but I would like Nate Chapin to give his (unofficial?) review first.

> Source/WebCore/loader/cache/CachedResource.cpp:920
> +        m_loader->frameLoader()->client()->dispatchDidChangeResourcePriority(m_loader->identifier(), loadPriority);

Bleh.  So ugly that CachedResource talks to FLC IMO.

>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:442
>> +void FrameLoaderClientImpl::dispatchDidChangeResourcePriority(unsigned long identifier,
>> +                                                              ResourceLoadPriority priority)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

We don't wrap to 80c in the rest of webkit, but maybe this is matching the chromium-specific style of this file?
Comment 8 James Simonsen 2013-02-27 14:28:38 PST
(In reply to comment #7)
> (From update of attachment 189634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189634&action=review
> 
> Seems reasonable, but I would like Nate Chapin to give his (unofficial?) review first.
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:920
> > +        m_loader->frameLoader()->client()->dispatchDidChangeResourcePriority(m_loader->identifier(), loadPriority);
> 
> Bleh.  So ugly that CachedResource talks to FLC IMO.

Yeah, I'd love another way to do it. The signal normally goes through the ResourceHandle, but TestRunner doesn't seem to have any knowledge of those. I'm open to suggestions.

> 
> >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:442
> >> +void FrameLoaderClientImpl::dispatchDidChangeResourcePriority(unsigned long identifier,
> >> +                                                              ResourceLoadPriority priority)
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> We don't wrap to 80c in the rest of webkit, but maybe this is matching the chromium-specific style of this file?

Indeed.
Comment 9 Nate Chapin 2013-02-27 14:29:00 PST
Comment on attachment 189634 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189634&action=review

>> Source/WebCore/loader/cache/CachedResource.cpp:920
>> +        m_loader->frameLoader()->client()->dispatchDidChangeResourcePriority(m_loader->identifier(), loadPriority);
> 
> Bleh.  So ugly that CachedResource talks to FLC IMO.

Agreed, but it's also ugly that SubresourceLoader and ResourceLoader talk to FLC. :(

That said, I could see a case for pushing this doing to ResourceLoader in order to minimize the number of places that know about FrameLoaderClient.

WDYT, simonjam?

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:216
> +    default:
> +        return "VeryUnresolved";
> +    }

VeryUnresolved? :)

> LayoutTests/http/tests/loading/promote-img-preload-priority.html:4
> +<html>
> +  <head>
> +    <script>
> +      if (window.testRunner) {

Do we care about indentation in tests? This is all 2-space.
Comment 10 James Simonsen 2013-02-27 15:16:32 PST
Created attachment 190615 [details]
Patch
Comment 11 James Simonsen 2013-02-27 15:17:43 PST
(In reply to comment #9)
> (From update of attachment 189634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189634&action=review
> 
> >> Source/WebCore/loader/cache/CachedResource.cpp:920
> >> +        m_loader->frameLoader()->client()->dispatchDidChangeResourcePriority(m_loader->identifier(), loadPriority);
> > 
> > Bleh.  So ugly that CachedResource talks to FLC IMO.
> 
> Agreed, but it's also ugly that SubresourceLoader and ResourceLoader talk to FLC. :(
> 
> That said, I could see a case for pushing this doing to ResourceLoader in order to minimize the number of places that know about FrameLoaderClient.
> 
> WDYT, simonjam?

Sounds good. Done.

> 
> > Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:216
> > +    default:
> > +        return "VeryUnresolved";
> > +    }
> 
> VeryUnresolved? :)

Fixed. Though I intend to make it my mission to find a useful reason to have an enum named VeryUnresolved. :P

> 
> > LayoutTests/http/tests/loading/promote-img-preload-priority.html:4
> > +<html>
> > +  <head>
> > +    <script>
> > +      if (window.testRunner) {
> 
> Do we care about indentation in tests? This is all 2-space.

Dunno. Fixed it though.
Comment 12 Eric Seidel (no email) 2013-02-27 15:20:34 PST
Comment on attachment 190615 [details]
Patch

Looks reasonable to me.  Waiting for chapin's comment.
Comment 13 WebKit Review Bot 2013-02-27 15:49:47 PST
Comment on attachment 190615 [details]
Patch

Clearing flags on attachment: 190615

Committed r144248: <http://trac.webkit.org/changeset/144248>
Comment 14 WebKit Review Bot 2013-02-27 15:49:51 PST
All reviewed patches have been landed.  Closing bug.