Bug 128043 - Remove CachedImageManual class
Summary: Remove CachedImageManual class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-01 09:30 PST by David Kilzer (:ddkilzer)
Modified: 2014-02-03 11:30 PST (History)
10 users (show)

See Also:


Attachments
Patch v1 (11.30 KB, patch)
2014-02-01 09:37 PST, David Kilzer (:ddkilzer)
darin: review+
ddkilzer: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2014-02-01 09:30:35 PST
Remove the CachedImageManual class that was upstreamed for iOS by folding the functionality into CachedImage.
Comment 1 David Kilzer (:ddkilzer) 2014-02-01 09:37:32 PST
Created attachment 222883 [details]
Patch v1
Comment 2 Darin Adler 2014-02-01 21:21:10 PST
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?
Comment 3 David Kilzer (:ddkilzer) 2014-02-02 20:36:59 PST
(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?
Comment 4 Darin Adler 2014-02-02 23:18:26 PST
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.
Comment 5 David Kilzer (:ddkilzer) 2014-02-03 11:10:29 PST
(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?
Comment 6 David Kilzer (:ddkilzer) 2014-02-03 11:30:10 PST
Committed r163318: <http://trac.webkit.org/changeset/163318>