Bug 128043 – Remove CachedImageManual class

• Component: Page Loading
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Remove the CachedImageManual class that was upstreamed for iOS by folding the functionality into CachedImage.

[David Kilzer (:ddkilzer)]

Created attachment 222883 [details]
Patch v1

[Darin Adler]

Comment on attachment 222883 [details]
Patch v1

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

> Source/WebCore/loader/cache/CachedImage.cpp:100
> +#if USE(CG)
> +    if (UNLIKELY(isManuallyCached())) {
> +        // Use the incoming URL in the response field. This ensures that code
> +        // using the response directly, such as origin checks for security,
> +        // actually see something.
> +        m_response.setURL(url);
> +    }
> +#endif

I’m not sure I understand why the manually cached flag is unconditional, but this code is CG-only. I’d expect the two to be consistent. If ManuallyCached is temporarily not yet used in non-CG configurations, this code would still do no harm.

> Source/WebCore/loader/cache/CachedImage.cpp:627
> +// FIXME: Remove the USE(CG) once we make MemoryCache::addImageToCache() platform-independent.

Why do we have to wait? I don’t see any harm in making this code unconditional even if isManuallyCached() is going to be always false on platforms that have MemoryCache::addImageToCache() yet.

> Source/WebCore/loader/cache/CachedImage.h:85
> +#if USE(CG)
> +    // FIXME: Remove the USE(CG) once we make MemoryCache::addImageToCache() platform-independent.

Same comment as above. Doesn’t seem that this needs to be conditional. Of if we do want it conditional, then the data member should also be conditional.

> Source/WebCore/loader/cache/CachedImage.h:150
> +    unsigned char m_isManuallyCached : 1;
> +    unsigned char m_shouldPaintBrokenImage : 1;

Why unsigned char instead of unsigned for these bit fields?

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 222883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222883&action=review
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:100
> > +#if USE(CG)
> > +    if (UNLIKELY(isManuallyCached())) {
> > +        // Use the incoming URL in the response field. This ensures that code
> > +        // using the response directly, such as origin checks for security,
> > +        // actually see something.
> > +        m_response.setURL(url);
> > +    }
> > +#endif
> 
> I’m not sure I understand why the manually cached flag is unconditional, but this code is CG-only. I’d expect the two to be consistent. If ManuallyCached is temporarily not yet used in non-CG configurations, this code would still do no harm.
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:627
> > +// FIXME: Remove the USE(CG) once we make MemoryCache::addImageToCache() platform-independent.
> 
> Why do we have to wait? I don’t see any harm in making this code unconditional even if isManuallyCached() is going to be always false on platforms that have MemoryCache::addImageToCache() yet.
> 
> > Source/WebCore/loader/cache/CachedImage.h:85
> > +#if USE(CG)
> > +    // FIXME: Remove the USE(CG) once we make MemoryCache::addImageToCache() platform-independent.
> 
> Same comment as above. Doesn’t seem that this needs to be conditional. Of if we do want it conditional, then the data member should also be conditional.

Okay, I'll remove the USE(CG) statements.  The only place it is truly needed is MemoryCache.cpp, so I'll leave it there.

> > Source/WebCore/loader/cache/CachedImage.h:150
> > +    unsigned char m_isManuallyCached : 1;
> > +    unsigned char m_shouldPaintBrokenImage : 1;
> 
> Why unsigned char instead of unsigned for these bit fields?

So that the bitfields would use 1 byte instead of 4 bytes since there were only 2 bitfields, and a bool only uses 1 byte.

Should I use 'unsigned' instead?

[Darin Adler]

Comment on attachment 222883 [details]
Patch v1

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

>>> Source/WebCore/loader/cache/CachedImage.h:150
>>> +    unsigned char m_shouldPaintBrokenImage : 1;
>> 
>> Why unsigned char instead of unsigned for these bit fields?
> 
> So that the bitfields would use 1 byte instead of 4 bytes since there were only 2 bitfields, and a bool only uses 1 byte.
> 
> Should I use 'unsigned' instead?

I don’t think it works that way. If "unsigned char" actually makes the object smaller, that’s fine, but I believe that using “unsigned” does not have the effect of using 4 bytes instead of one when there are those two bit fields.

If it wasn’t for the strange Windows rule where it won’t combine bit fields of different types, I would say we should use “bool” here, but to accommodate Windows we should use “unsigned”. Unless it really does have the effect you say, and, if so, unsigned char is fine with me.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (From update of attachment 222883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222883&action=review
> 
> I don’t think it works that way. If "unsigned char" actually makes the object smaller, that’s fine, but I believe that using “unsigned” does not have the effect of using 4 bytes instead of one when there are those two bit fields.

It does make a difference.

$ cat test_size.cpp
#include <stdio.h>

class A {
public:
    bool m_bool;
};

class B {
public:
    unsigned char m_a : 1;
    unsigned char m_b : 1;
};

class C {
public:
    unsigned m_a : 1;
    unsigned m_b : 1;
};

class D {
public:
    unsigned m_unsigned;
};

int main(int argc, char** argv)
{
    fprintf(stdout, "sizeof(A) = %lu\n", sizeof(A));
    fprintf(stdout, "sizeof(B) = %lu\n", sizeof(B));
    fprintf(stdout, "sizeof(C) = %lu\n", sizeof(C));
    fprintf(stdout, "sizeof(D) = %lu\n", sizeof(D));
    return 0;
}

$ clang -arch i386 -arch x86_64 -o test_size test_size.cpp
$ arch -arch i386 ./test_size
sizeof(A) = 1
sizeof(B) = 1
sizeof(C) = 4
sizeof(D) = 4
$ arch -arch x86_64 ./test_size
sizeof(A) = 1
sizeof(B) = 1
sizeof(C) = 4
sizeof(D) = 4

> If it wasn’t for the strange Windows rule where it won’t combine bit fields of different types, I would say we should use “bool” here, but to accommodate Windows we should use “unsigned”. Unless it really does have the effect you say, and, if so, unsigned char is fine with me.

Roger/Brent: Was this Windows rule only true of the older MS Visual C++ compiler, or does this still apply to the current compiler?

[David Kilzer (:ddkilzer)]

Committed r163318: <http://trac.webkit.org/changeset/163318>