Bug 116849 - It is too easy to accidentally construct a Color
Summary: It is too easy to accidentally construct a Color
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-27 20:03 PDT by Sam Weinig
Modified: 2017-05-09 15:58 PDT (History)
5 users (show)

See Also:

Attachments
Patch (74.54 KB, patch)
2013-05-27 20:05 PDT, Sam Weinig 		no flags Details | Formatted Diff | Diff
Patch (80.91 KB, text/plain)
2013-05-29 10:46 PDT, Sam Weinig 		no flags Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-05-27 20:03:04 PDT 
It is too easy to accidentally construct a Color
Comment 1 Sam Weinig 2013-05-27 20:05:50 PDT 
Created attachment 203019 [details]
Patch
Comment 2 Sam Weinig 2013-05-27 20:09:43 PDT 
This is likely to break other ports. So if any kind soul would help me out, that would be great!
Comment 3 Early Warning System Bot 2013-05-27 20:17:23 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/670158
Comment 4 Early Warning System Bot 2013-05-27 20:20:19 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/735142
Comment 5 kov's GTK+ EWS bot 2013-05-27 20:33:22 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/710276
Comment 6 Build Bot 2013-05-27 20:34:33 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/703198
Comment 7 EFL EWS Bot 2013-05-27 20:55:42 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/693212
Comment 8 EFL EWS Bot 2013-05-28 01:31:52 PDT 
Comment on attachment 203019 [details]
Patch

Attachment 203019 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/740036
Comment 9 Darin Adler 2013-05-28 08:57:31 PDT 
Comment on attachment 203019 [details]
Patch

I suggest doing this in a couple steps. I’d add the transparentColor(), whiteColor(), blackColor(), and other three functions first and adopt them without the rest of the change, since that seems to be more than half of the patch.

If you want to do this the way I did AdoptCF, then you should have the thing set up locally with explicit constructors, but not land that part until you separately land all the adoption. Then you can have a patch that just tries to flip the switch.

RenderTheme::platformTapHighlightColor seems to be the reason for the EFL and Qt failures. prepareCairoContextSource the reason for the GTK failure. There are Windows failures. And probably iOS ones too.

I’m going to say review-, but I did not spot any problems with the patch other than the fact that it fails to compile on all the non-Mac platforms.
Comment 10 Sam Weinig 2013-05-29 10:46:50 PDT 
Created attachment 203215 [details]
Patch
Comment 11 Darin Adler 2013-05-29 12:43:20 PDT 
Comment on attachment 203215 [details]
Patch

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

I can’t tell what you intend, Sam. Besides adoption of fooColor functions, this patch contains many examples of adding explicit Color() casts. Did you mean to include them or save them for a separate patch?

> Source/WebCore/WebCore.exp.in:1
> +__ZN7WebCore14SecurityOrigin16setDomainFromDOMERKN3WTF6StringE

Oops.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:99
> +		7C54FC711754712F00A8B391 /* SecurityOriginHash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7C54FC6F17546D3000A8B391 /* SecurityOriginHash.cpp */; };

Oops.

> Tools/TestWebKitAPI/Tests/WebCore/SecurityOriginHash.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Oops.
Comment 12 Sam Weinig 2013-05-29 13:40:45 PDT 
Comment on attachment 203215 [details]
Patch

I didn't mean to upload this at all :(.  Sorry for the noise.
Comment 13 Sam Weinig 2017-05-09 15:58:09 PDT 
Color has changed a bit since I filed this. I don't think this is worth keeping around.