Bug 77168 – [Windows] Optionally invert colors when drawing to a WebView's backing store.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Windows] Optionally invert colors when drawing to a WebView's backing store.

[Andy Estes]

Created attachment 124230 [details]
Patch

[Darin Adler]

Comment on attachment 124230 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValueMappings.h:291
> +        default:
> +            ASSERT_NOT_REACHED();
> +            break;

By adding default here, we won’t get warnings if we accidentally leave out one value from the switch statement. We want those warnings at compile time. To get the run-time assertion we are losing the compile-time check.

If we want both, then we need to change the break statements into return statements, and then we can put an ASSERT_NOT_REACHED after the switch statement.

> Source/WebKit/win/WebView.cpp:1159
> +            gc.fillRect(dirtyRect, Color(0xFF, 0xFF, 0xFF), ColorSpaceDeviceRGB, CompositeDifference);

Should we use Color::white or 0xFFFFFFFF instead of Color(0xFF, 0xFF, 0xFF)?

> Source/WebKit/win/WebView.h:1004
> +    bool m_shouldInvertColors;

A public data member is not the right style for something you can change on a WebView. Normally we’d use a getter/setter function pair or something along those lines.

[Andy Estes]

(In reply to comment #2)
> (From update of attachment 124230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124230&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:291
> > +        default:
> > +            ASSERT_NOT_REACHED();
> > +            break;
> 
> By adding default here, we won’t get warnings if we accidentally leave out one value from the switch statement. We want those warnings at compile time. To get the run-time assertion we are losing the compile-time check.
> 
> If we want both, then we need to change the break statements into return statements, and then we can put an ASSERT_NOT_REACHED after the switch statement.

This is in a ctor so I think I can just remove the default label to achieve what we want here.
> 
> > Source/WebKit/win/WebView.cpp:1159
> > +            gc.fillRect(dirtyRect, Color(0xFF, 0xFF, 0xFF), ColorSpaceDeviceRGB, CompositeDifference);
> 
> Should we use Color::white or 0xFFFFFFFF instead of Color(0xFF, 0xFF, 0xFF)?

Sure. I'll use Color::white
> 
> > Source/WebKit/win/WebView.h:1004
> > +    bool m_shouldInvertColors;
> 
> A public data member is not the right style for something you can change on a WebView. Normally we’d use a getter/setter function pair or something along those lines.

This is in the private block, isn't it?

[Andy Estes]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 124230 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124230&action=review
> > 
> > > Source/WebKit/win/WebView.h:1004
> > > +    bool m_shouldInvertColors;
> > 
> > A public data member is not the right style for something you can change on a WebView. Normally we’d use a getter/setter function pair or something along those lines.
> 
> This is in the private block, isn't it?

I should add that I plan to add the API to enable this feature in a follow-on patch, hence the private variable with no getter or setter.

[Andy Estes]

Created attachment 124246 [details]
Patch

[Sam Weinig]

Comment on attachment 124246 [details]
Patch

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

You should probably file bugs on other graphics backends to implement difference.  Especially Cairo, since they use win/WebView.

> Source/WebKit/win/WebView.cpp:335
> +    : m_shouldInvertColors(false)
> +    , m_refCount(0)

I would be good if refCount remained first.

[Andy Estes]

Committed r106274: <http://trac.webkit.org/changeset/106274>