Bug 77168 - [Windows] Optionally invert colors when drawing to a WebView's backing store.
Summary: [Windows] Optionally invert colors when drawing to a WebView's backing store.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks: 77359 77352 77354 77355 77356 77358
  Show dependency treegraph
 
Reported: 2012-01-26 17:59 PST by Andy Estes
Modified: 2012-01-30 12:18 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2012-01-26 18:06 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2012-01-26 20:01 PST, Andy Estes
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2012-01-26 17:59:00 PST
[Windows] Optionally invert colors when drawing to a WebView's backing store.
Comment 1 Andy Estes 2012-01-26 18:06:38 PST
Created attachment 124230 [details]
Patch
Comment 2 Darin Adler 2012-01-26 18:17:06 PST
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.
Comment 3 Andy Estes 2012-01-26 18:27:47 PST
(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?
Comment 4 Andy Estes 2012-01-26 19:33:54 PST
(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.
Comment 5 Andy Estes 2012-01-26 20:01:52 PST
Created attachment 124246 [details]
Patch
Comment 6 Sam Weinig 2012-01-30 11:27:44 PST
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.
Comment 7 Andy Estes 2012-01-30 12:01:46 PST
Committed r106274: <http://trac.webkit.org/changeset/106274>