Summary: | [Windows] Optionally invert colors when drawing to a WebView's backing store. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Estes <aestes> | ||||||
Component: | New Bugs | Assignee: | Andy Estes <aestes> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | macpherson, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 77352, 77354, 77355, 77356, 77358, 77359 | ||||||||
Attachments: |
|
Description
Andy Estes
2012-01-26 17:59:00 PST
Created attachment 124230 [details]
Patch
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. (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? (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. Created attachment 124246 [details]
Patch
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. Committed r106274: <http://trac.webkit.org/changeset/106274> |