Bug 81161 – Remove duplicate error() impls in CachedResource subclasses

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 131934 [details]
patch

[WebKit Review Bot]

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.

[Nate Chapin]

Created attachment 131939 [details]
fix style issues

Protip: run check-webkit-style *after* prepare-ChangeLog.

[Alexey Proskuryakov]

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.

[Nate Chapin]

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

[Nate Chapin]

Created attachment 132118 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132118 [details]
Patch for landing

Clearing flags on attachment: 132118

Committed r110976: <http://trac.webkit.org/changeset/110976>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This was rolled out in r110986.

[Hajime Morrita]

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.

[Nate Chapin]

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

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 133936 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133936 [details]
Patch for landing

Clearing flags on attachment: 133936

Committed r112201: <http://trac.webkit.org/changeset/112201>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

This got rolled out again, reopening again.

[Nate Chapin]

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

[Nate Chapin]

Created attachment 150204 [details]
Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

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.

[WebKit Review Bot]

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

[Nate Chapin]

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.

[WebKit Review Bot]

Comment on attachment 162095 [details]
patch

Clearing flags on attachment: 162095

Committed r127695: <http://trac.webkit.org/changeset/127695>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.