Bug 109222 – [Qt][WK2] Move Color picker on top of the C API

• Component: New Bugs
• Status: RESOLVED INVALID

[Pierre Rossi]

[Qt][WK2] Move Color picker on top of the C API

[Pierre Rossi]

Created attachment 187171 [details]
Patch

[Simon Hausmann]

Comment on attachment 187171 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:200
> +void QtWebPageUIClient::showColorPicker(WKPageRef, WKStringRef initialColor, WKColorPickerResultListenerRef listener, const void *clientInfo)
> +{
> +    toQtWebPageUIClient(clientInfo)->showColorPicker(listener, QColor(toWTFString(initialColor)));
> +}
> +
> +void QtWebPageUIClient::hideColorPicker(WKPageRef, const void *clientInfo)
> +{
> +    toQtWebPageUIClient(clientInfo)->hideColorPicker();
> +}

Given the few lines of code I suggest to merge this with the non-static showColorPicker and then just wrote

d->m_colorPicker.reset(new QWebColorPicker(...));

[Simon Hausmann]

Patch looks good to me overall, modulo the small suggested change.

Pinging Benjamin for WK2 sign-off :)

[Benjamin Poulain]

Comment on attachment 187171 [details]
Patch

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

I'll have to look at this again. Simon, can you review then ping me?

> Source/WebKit2/Target.pri:829
> +# FIXME: move this along once the QQuickWebView is a bit less tangled.

I think this could be a little more descriptive. It is not clear what you mean.

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:3
> + * Copyright (C) 2012 Intel Corporation. All rights reserved.
> + * Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies)

Hum, hum, no love for Digia in 2013? ;)

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:33
> +#include <wtf/text/StringBuilder.h>

Do you use it somewhere?

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:79
> +QWebColorPicker::~QWebColorPicker()
> +{
> +}

I don't think you will need this :)

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:87
> +    if (!component) {
> +        delete contextObject;
> +        return;
> +    }

This looks so fragile :(

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:102
> +    // Needs to be enqueue because it might trigger deletion.

I am not sure what is the "it" that can trigger deletion.
It could be useful to explain the cases where that would happen.

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:130
> +    WKRetainPtr<WKStringRef> colorString(AdoptWK, WKStringCreateWithUTF8CString(coreColor.serialized().utf8().data()));

toApi(coreColor.serialized()) would be enough?

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:131
> +    WKColorPickerResultListenerSetColor(m_listener, colorString.get());

I am such a huge fan of this API. Passing a color by string is brilliant :(

That's why we cannot have nice things I guess.
As far as I am concerned, you are free to break it or expend it for something better.

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker_p.h:2
> + * Copyright (C) 2012 Intel Corporation. All rights reserved.

... :)

> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker_p.h:40
> +namespace WebCore {
> +class Color;
> +}

You need this in the header?

[Pierre Rossi]

Comment on attachment 187171 [details]
Patch

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

Thanks benjaminp, gonna try and clean that up a little... :)

>> Source/WebKit2/Target.pri:829
>> +# FIXME: move this along once the QQuickWebView is a bit less tangled.
> 
> I think this could be a little more descriptive. It is not clear what you mean.

yeah, I figured that was too cryptic. Hence the link below ;)
But I'll rephrase it.

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:3
>> + * Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies)
> 
> Hum, hum, no love for Digia in 2013? ;)

Dude, this thing is way too complicated to memorize ! Actually this shows up as a rename from WebColorChooserProxyQt in git, but the review tool doesn't help much there...

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:33
>> +#include <wtf/text/StringBuilder.h>
> 
> Do you use it somewhere?

Stray stuff after I was puzzled with how to deal with the string representation of a color, nice catch !

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:79
>> +}
> 
> I don't think you will need this :)

Probably should clean that up indeed, did a dumb massive rename.

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:87
>> +    }
> 
> This looks so fragile :(

Just moving stuff around. It seemed to work before...
Indeed, that pattern might not be the most pleasing to the eye...

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker.cpp:131
>> +    WKColorPickerResultListenerSetColor(m_listener, colorString.get());
> 
> I am such a huge fan of this API. Passing a color by string is brilliant :(
> 
> That's why we cannot have nice things I guess.
> As far as I am concerned, you are free to break it or expend it for something better.

Will do ! (break I mean) I actually wanted to add the element rect in there anyway ;)

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker_p.h:2
>> + * Copyright (C) 2012 Intel Corporation. All rights reserved.
> 
> ... :)

Hey, this is originally a generous donation done by an Intel guy, I'm not gonna take away the credit ! But indeed, maybe Digia could get a bit of love...

>> Source/WebKit2/UIProcess/API/qt/qwebcolorpicker_p.h:40
>> +}
> 
> You need this in the header?

meh, probably not anymore... nice catch :)

[Pierre Rossi]

Created attachment 190730 [details]
Patch

Since the use of Color(const String&) is apparently only intended for use by the DOM, and not WebKit, let's revamp that API a bit.

[Simon Hausmann]

Comment on attachment 190730 [details]
Patch

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

LGTM overall. Two small nitpicks.

> Source/WebKit2/Shared/API/c/WKBase.h:96
> +typedef uint32_t WKARGB;

I suggest something a little more verbose (might be misread as "ARGH!" :)

How about WKColorARGB32? or WKARGB32Color?

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:226
> +    notImplemented();

I'd leave out the notImplemented() because it is implemented after all.

Alternative the pure virtual method could become one with an empty default impl like this, since we want to promote the C API then and ultimately it can be removed.

[Pierre Rossi]

Comment on attachment 190730 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKBase.h:96
>> +typedef uint32_t WKARGB;
> 
> I suggest something a little more verbose (might be misread as "ARGH!" :)
> 
> How about WKColorARGB32? or WKARGB32Color?

Haha, agreed, people would ruin their keyboard trying to pronounce this. I'll go with your first proposition ;)

>> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:226
>> +    notImplemented();
> 
> I'd leave out the notImplemented() because it is implemented after all.
> 
> Alternative the pure virtual method could become one with an empty default impl like this, since we want to promote the C API then and ultimately it can be removed.

true, that'd clean things up once and for all.

[Pierre Rossi]

Created attachment 190742 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Anders Carlsson]

Comment on attachment 190742 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.