| Summary: | Remove duplicate error() impls in CachedResource subclasses | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nate Chapin <japhet> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, koivisto, morrita, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 81330, 82302 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Nate Chapin
2012-03-14 15:24:14 PDT
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. 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> All reviewed patches have been landed. Closing bug. 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> All reviewed patches have been landed. Closing bug. |