Several CachedResource subclasses override error()...with the exact same implementation. It appears this is only necessary because checkNotify() isn't virtual. Let's make checkNotify() virtual and delete some duplicate code. CachedImage does some extra work in error() though, so it'll still need to override.
Created attachment 131934 [details] patch
Attachment 131934 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebCore/ChangeLog:13: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 131939 [details] fix style issues Protip: run check-webkit-style *after* prepare-ChangeLog.
Comment on attachment 131939 [details] fix style issues View in context: https://bugs.webkit.org/attachment.cgi?id=131939&action=review Do these checkNotify() functions need to be public? > Source/WebCore/loader/cache/CachedImage.cpp:388 > setStatus(status); > - ASSERT(errorOccurred()); > m_data.clear(); > notifyObservers(); > - setLoading(false); > - checkNotify(); > + CachedResource::error(status); This is not so great, with setStatus being called twice.
(In reply to comment #4) > (From update of attachment 131939 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131939&action=review > > Do these checkNotify() functions need to be public? I don't think so. I just left it as-is, which seems a bit silly in hindsight. > > > Source/WebCore/loader/cache/CachedImage.cpp:388 > > setStatus(status); > > - ASSERT(errorOccurred()); > > m_data.clear(); > > notifyObservers(); > > - setLoading(false); > > - checkNotify(); > > + CachedResource::error(status); > > This is not so great, with setStatus being called twice. I agree. Unfortunately, it does seem to break several tests things if it's not set there. I'll see if it works if CachedResource::error() is called first.
Created attachment 132118 [details] Patch for landing
Comment on attachment 132118 [details] Patch for landing Clearing flags on attachment: 132118 Committed r110976: <http://trac.webkit.org/changeset/110976>
All reviewed patches have been landed. Closing bug.
This was rolled out in r110986.
The problem is actually in chromium side. I'm fixing it and will re-land this once the fix available. I'm sorry for the inconvenience.
(In reply to comment #10) > The problem is actually in chromium side. I'm fixing it and will re-land this once the fix available. > I'm sorry for the inconvenience. Is there a bug open for the chromium issue? I couldn't immediately find it. Let me know how I can help you resolve this.
(In reply to comment #11) > (In reply to comment #10) > > The problem is actually in chromium side. I'm fixing it and will re-land this once the fix available. > > I'm sorry for the inconvenience. > > Is there a bug open for the chromium issue? I couldn't immediately find it. > > Let me know how I can help you resolve this. Oh, I'm going to re-land this because the change is now available.
Created attachment 133936 [details] Patch for landing
Comment on attachment 133936 [details] Patch for landing Clearing flags on attachment: 133936 Committed r112201: <http://trac.webkit.org/changeset/112201>
This got rolled out again, reopening again.
(In reply to comment #16) > This got rolled out again, reopening again. I just tested this manually on chromium-mac and chromium-win and the offending test is passing, so I'm going to try to land this again...
Created attachment 150204 [details] Patch for landing
Comment on attachment 150204 [details] Patch for landing Rejecting attachment 150204 [details] from commit-queue. New failing tests: compositing/color-matching/pdf-image-match.html Full output: http://queues.webkit.org/results/13121384
Created attachment 150242 [details] Archive of layout-test-results from ec2-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 162095 [details] patch This is identical to previous versions, except a slightly cleaner CachedImage::error(), which does not duplicating any of the work in CachedResource::error(). Verified locally that webkit_unit_tests pass.
Comment on attachment 162095 [details] patch Rejecting attachment 162095 [details] from commit-queue. New failing tests: http/tests/cache/subresource-expiration-1.html Full output: http://queues.webkit.org/results/13742948
Comment on attachment 162095 [details] patch That test failure is showing up for a bunch of other patches on the cq bots, going to try adding it to the queue again.
Comment on attachment 162095 [details] patch Clearing flags on attachment: 162095 Committed r127695: <http://trac.webkit.org/changeset/127695>