Bug 110527 – [chromium] Lower priority of preloaded images

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

[chromium] Lower priority of preloaded images

[James Simonsen]

Created attachment 189634 [details]
Patch

[James Simonsen]

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.

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 189634 [details]
Patch

Attachment 189634 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16693423

[Build Bot]

Comment on attachment 189634 [details]
Patch

Attachment 189634 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16716172

[James Simonsen]

Ping? Any thoughts on this? Did I go overboard to test it? Is there someone else who should review it?

[Eric Seidel (no email)]

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?

[James Simonsen]

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

[Nate Chapin]

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.

[James Simonsen]

Created attachment 190615 [details]
Patch

[James Simonsen]

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

[Eric Seidel (no email)]

Comment on attachment 190615 [details]
Patch

Looks reasonable to me.  Waiting for chapin's comment.

[WebKit Review Bot]

Comment on attachment 190615 [details]
Patch

Clearing flags on attachment: 190615

Committed r144248: <http://trac.webkit.org/changeset/144248>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.