875753 - Implement <input type="color">: GTK widget/color picker

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Comment 1

[Arnaud Bienner]

Attachment: Color input: Gtk widget

Mounir, Karl, could you please kindly review this patch?
@Karl: if you want to import the patch to test it "for real", you will need to apply layout (bug 875275) and color picker IDL (bug 875747 and bug 885996) patches first, as there are not integrated in mozilla-central atm.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 766369 [details] [diff] [review]
Color input: Gtk widget

Please include function names in diff hunks of future patches.

>+#include <gtk/gtk.h>
>+
>+// Some gobject functions expect functions for gpointer arguments.
>+// gpointer is void* but C++ doesn't like casting functions to void*.
>+template<class T> static inline gpointer
>+FuncToGpointer(T aFunction)

Please include glib.h instead of gtk/gtk.h here,
and add a #ifndef nsGtkUtils_h_ include guard.

>   if (allowPlatformPicker) {
>-      picker = new nsFilePicker;
>+    picker = new nsFilePicker;
>   } else {

>+nsColorPickerConstructor(nsISupports *aOuter, REFNSIID aIID,
>+                        void **aResult)
>+{
>+  *aResult = nullptr;

Please no whitespace changes in existing code.
Most of this file is following the 4-space indentation specified in the
mode-line so please use 4-spaces for the new function, and touch up the
indentation of the parameter declaration.

>+int nsColorPicker::convertGdkColorComponent(guint16 color_component) {
>+  // GdkColor value is in range [0..65535]. We need something in range [0..255]
>+  return (int(color_component)*255)/65535;
>+}

Would rounding to nearest be better?  (color_component * 255 + 127)/65535

>+
>+guint16 nsColorPicker::convertToGdkColorComponent(int color_component) {
>+  return (color_component)*65535/255;
>+}

Unnecessary parentheses here.
(Rounding is not necessary here because 65535 = 255 * 257.)

>+  nsCOMPtr<nsIWidget> widget = mozilla::widget::WidgetUtils::DOMWindowToWidget(parent);
>+  mParentWidget = widget;

Assign directly to mParentWidget, so |widget| is not needed. 

>+  NS_ASSERTION(initialColor.Length() == 7,
>+      "Input color string should be 7 lengh (i.e. a string representing a valid "
>+      "simple color)");

If this is an XPCOM method, which it looks like, then this should handle
unrecognized strings by returning and error code.

>+  nsDependentString withoutHash(nsAutoString(initialColor).get() + 1, initialColor.Length() - 1);
>+  NS_HexToRGB(withoutHash, &color);

StringTail() can make this a little simpler.

>+NS_IMETHODIMP nsColorPicker::Open(nsIColorPickerShownCallback *aColorPickerShownCallback)

Need to handle Open() being called twice, probably by checking mCallback.

>+  gtk_window_set_modal(window, FALSE);

Are you sure you don't want this to be modal, even when there is a parent?
If so, this call shouldn't be necessary, as I expect the dialog is not default
modal.

>+    GtkWindowGroup *parentGroup = gtk_window_get_group(parent_widget);
>+    if (parentGroup) {
>+      gtk_window_group_add_window(parentGroup, window);
>+    }

The function you want here is gtk_window_set_transient_for(), which will
handle the groups automatically.  (The group code in nsFilePicker actually
does nothing because the transient-for parent was set in the file chooser
dialog constructor.)

>+  gtk_color_selection_set_has_opacity_control(
>+      GTK_COLOR_SELECTION(gtk_color_selection_dialog_get_color_selection(
>+          GTK_COLOR_SELECTION_DIALOG(color_chooser))),
>+      FALSE);

No need for this call because the default value of "has-opacity-control" is
false.

>+  gtk_color_selection_get_current_color(
>+    GTK_COLOR_SELECTION(gtk_color_selection_dialog_get_color_selection(
>+      GTK_COLOR_SELECTION_DIALOG(color_chooser))),

configure.in requires only GTK+ version 2.10, and that will remain until bug
795354 is fixed.  gtk_color_selection_dialog_get_color_selection is only
available since 2.14, so please either get the "color-selection" property or
make compilation of the file conditional on the GTK+ version.

>+  std::stringstream stream;
>+  stream << '#' << std::hex <<  std::setfill('0');
>+  stream << std::setw(2) << convertGdkColorComponent(rgba.red);
>+  stream << std::setw(2) << convertGdkColorComponent(rgba.green);
>+  stream << std::setw(2) << convertGdkColorComponent(rgba.blue);

I don't know whether string stream is safe to use when compiling without
exceptions, as in Gecko.  I'd suggest Mozilla string types and functions, but
perhaps you could find someone who knows enough about this to review
stringstream.

>+  mColor = NS_ConvertASCIItoUTF16(stream.str().c_str());

Convert directly into mColor using AppendASCIItoUTF16() instead of using a
temporary NS_ConvertASCIItoUTF16 object.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> gtk_color_selection_dialog_get_color_selection is only
> available since 2.14, so please either get the "color-selection" property or
> make compilation of the file conditional on the GTK+ version.

Might be easiest to add a gtk_color_selection_dialog_get_color_selection method for older GTK under widget/gtk2/compat that gets the color-selection property.

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 766369 [details] [diff] [review]
Color input: Gtk widget

Review of attachment 766369 [details] [diff] [review]:
-----------------------------------------------------------------

Given the detailed review Karl did, I am just going to add a few comments.

::: widget/gtk2/nsColorPicker.cpp
@@ +46,5 @@
> +  mParentWidget = widget;
> +  mTitle.Assign(title);
> +
> +  nscolor color;
> +  NS_ASSERTION(initialColor.Length() == 7,

Please use MOZ_ASSERT().

@@ +52,5 @@
> +      "simple color)");
> +  nsDependentString withoutHash(nsAutoString(initialColor).get() + 1, initialColor.Length() - 1);
> +  NS_HexToRGB(withoutHash, &color);
> +
> +  mDefaultColor = convertToGdkColor(color);

You do not need to do that. You should just save the initialColor to mColor and when ::Open() is called, you could do the conversion from nsAString color to the gdk color. In addition of saving memory, you can return the initial color in ::GetColor() for free as long as the picker is not closed.

@@ +60,5 @@
> +
> +/* readonly attribute AString color; */
> +NS_IMETHODIMP nsColorPicker::GetColor(nsAString & aColor)
> +{
> +  aColor = mColor;

Shouldn't that be aColor.Assign(mColor); ?

@@ +135,5 @@
> +    case GTK_RESPONSE_DELETE_EVENT:
> +      result = nsIColorPicker::returnCancel;
> +      break;
> +    default:
> +      NS_WARNING("Unexpected response");

Shouldn't that be something like MOZ_ASSERT?

Comment 5

[Arnaud Bienner]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> and add a #ifndef nsGtkUtils_h_ include guard.
Ooops :(

> Please no whitespace changes in existing code.
> Most of this file is following the 4-space indentation specified in the
> mode-line so please use 4-spaces for the new function, and touch up the
> indentation of the parameter declaration.

Most of the function is following 2-spaces indentation, and having only one line 4-spaces indented looks ugly. That's why I changed this.
Btw, Mozilla's coding rules specifies we should "use two spaces for indentation" and my IDE is configured to respect those rules with autoindent. I'm OK to change indentation in the new function if you really think it's mandatory, but it's boring to have coding rules changing from a file to another.

> Would rounding to nearest be better?  (color_component * 255 + 127)/65535

Probably indeed. I will change this.

> Are you sure you don't want this to be modal, even when there is a parent?

I have no strong opinion about this.
It's just that Gtk applications I've used doesn't make the color picker modal, so I prefer to keep the standard behavior.
Also, IMHO as long as it doesn't seem to be necessary, I prefer to not have windows being modal. e.g. a user might want to access some rgb values described in a background tab and might want to copy-paste in the color picker or something like this.


> The function you want here is gtk_window_set_transient_for()

Thanks! I didn't known this function, and it seems to do exactly what I want.

> (The group code in nsFilePicker actually
> does nothing because the transient-for parent was set in the file chooser
> dialog constructor.)

Indeed, I had a look to the nsFilePicker to write nsColorPicker, so I reused this part of the code. So if I understand well, this code is useless in nsFilePicker too? So maybe we should remove it? But this should probably be discussed/done in another bug.

> I don't know whether string stream is safe to use when compiling without
> exceptions, as in Gecko.  I'd suggest Mozilla string types and functions, but
> perhaps you could find someone who knows enough about this to review
> stringstream.

I think stingstream, and particularly << operator and .str() function doesn't throw exceptions by default (i.e. if you don't modify |exceptions| bitmask attribute) but set errors flags instead.

Do you know someone who might know this better?
Otherwise, I might use Mozilla API string "AppendInt" function with base 16 parameter, but if will have to perform custom checks to append padding zero when necessary.

Comment 6

[Arnaud Bienner]

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Please use MOZ_ASSERT().

Regarding Karl's comments I thought it's better to return an error code. Or is MOZ_ASSERT doing something acceptable here?


> You do not need to do that. You should just save the initialColor to mColor
> and when ::Open() is called, you could do the conversion from nsAString
> color to the gdk color. In addition of saving memory, you can return the
> initial color in ::GetColor() for free as long as the picker is not closed.

GetColor is always "free" as it only copies the value, but doesn't convert anything. Conversions occur in "Init" and "Done" functions (the latter being called when the color picker is closed).
Moving the "to gdk color" from "Init" to "Open" will not change anything in practice, as long as Init and Open are supposed to be called only once.
IMO, it's actually even worse, as the the Init function should check the initial_color parameter is correct (by doing the conversion). Having the "Open" function failing because of a bad parameter passed to "Init" function looks less obvious to me.

> Shouldn't that be aColor.Assign(mColor); ?

I thought the = operator was redefined to do the same thing.

> 
> @@ +135,5 @@
> > +    case GTK_RESPONSE_DELETE_EVENT:
> > +      result = nsIColorPicker::returnCancel;
> > +      break;
> > +    default:
> > +      NS_WARNING("Unexpected response");
> 
> Shouldn't that be something like MOZ_ASSERT?

If MOZ_ASSERT is making the function failing, I don't think it's good idea: in case something went wrong with the color picker for a unknown reason, it seems to be better to act like if it was closed with "cancel".

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Arnaud from comment #5)

> Most of the function is following 2-spaces indentation, and having only one
> line 4-spaces indented looks ugly. That's why I changed this.
> Btw, Mozilla's coding rules specifies we should "use two spaces for
> indentation" and my IDE is configured to respect those rules with
> autoindent. I'm OK to change indentation in the new function if you really
> think it's mandatory, but it's boring to have coding rules changing from a
> file to another.

Sorry about the inconvenience of varying file styles, but easier archaeology
wins of prettiness.  The style guide says "For existing code, use the
prevailing style in a file or module".

I'm pleased you chose 2 spaces for the new file, even though I wouldn't have
used 4 spaces for continuation lines.

> It's just that Gtk applications I've used doesn't make the color picker
> modal, so I prefer to keep the standard behavior.
> Also, IMHO as long as it doesn't seem to be necessary, I prefer to not have
> windows being modal. e.g. a user might want to access some rgb values
> described in a background tab and might want to copy-paste in the color
> picker or something like this.

Sounds good to me.

> > (The group code in nsFilePicker actually
> > does nothing because the transient-for parent was set in the file chooser
> > dialog constructor.)
> 
> Indeed, I had a look to the nsFilePicker to write nsColorPicker, so I reused
> this part of the code. So if I understand well, this code is useless in
> nsFilePicker too? So maybe we should remove it? But this should probably be
> discussed/done in another bug.

Yes, removing sounds good.  A separate patch attached to this bug is fine with
me, or file a new bug if you like.

> I think stingstream, and particularly << operator and .str() function
> doesn't throw exceptions by default (i.e. if you don't modify |exceptions|
> bitmask attribute) but set errors flags instead.

One of the issues with using standard libraries is that much of the
implementation is in the header.  If the header was not designed to be
compiled without exceptions then perhaps it will not behave as expected, but
I'm not familiar with the details of this argument.

> Do you know someone who might know this better?

Try :glandium perhaps.

> Otherwise, I might use Mozilla API string "AppendInt" function with base 16
> parameter, but if will have to perform custom checks to append padding zero
> when necessary.

Maybe Gecko has something like snprintf.
I suspect there must be similar use cases elsewhere, but I don't know where to
look, sorry.

Comment 8

[Karl Tomlinson (:karlt)]

In reply to Arnaud from comment #6)
> (In reply to Mounir Lamouri (:mounir) from comment #4)
> > Please use MOZ_ASSERT().
> 
> Regarding Karl's comments I thought it's better to return an error code. Or
> is MOZ_ASSERT doing something acceptable here?

MOZ_ASSERT will abort the program in a debug build, so not appropriate when
verifying XPIDL parameters that can be passed from JS.  (MOZ_ASSERT may be
appropriate for a out parameter pointer if XPIDL ensures that it is always
non-null when called from JS.)

> Having the
> "Open" function failing because of a bad parameter passed to "Init" function
> looks less obvious to me.

Yes, Init() should at least do verification of the parameter, which I guess pretty much involves extracting the color.

> > Shouldn't that be aColor.Assign(mColor); ?
> 
> I thought the = operator was redefined to do the same thing.

Yes, the only difference is that operator= returns *this, which the compiler
would optimize away.  I'm not aware of any style conventions one way or the other.

> > > +    case GTK_RESPONSE_DELETE_EVENT:
> > > +      result = nsIColorPicker::returnCancel;
> > > +      break;
> > > +    default:
> > > +      NS_WARNING("Unexpected response");
> > 
> > Shouldn't that be something like MOZ_ASSERT?
> 
> If MOZ_ASSERT is making the function failing, I don't think it's good idea:
> in case something went wrong with the color picker for a unknown reason, it
> seems to be better to act like if it was closed with "cancel".

I don't think we can be sure that the GTK dialog implementation won't change
in the future to return something different, possibly even a currently
undefined enum.

Here we're just trying to make a best guess of the right thing to do.
That won't be aborting the program.

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Arnaud from comment #6)
> (In reply to Mounir Lamouri (:mounir) from comment #4)
> > Please use MOZ_ASSERT().
> 
> Regarding Karl's comments I thought it's better to return an error code. Or
> is MOZ_ASSERT doing something acceptable here?

I am fine to MOZ_ASSERT() in ::Init(). This is a private API and assuming that we get expected values isn't much to ask. 

> > You do not need to do that. You should just save the initialColor to mColor
> > and when ::Open() is called, you could do the conversion from nsAString
> > color to the gdk color. In addition of saving memory, you can return the
> > initial color in ::GetColor() for free as long as the picker is not closed.
> 
> GetColor is always "free" as it only copies the value, but doesn't convert
> anything. Conversions occur in "Init" and "Done" functions (the latter being
> called when the color picker is closed).
> Moving the "to gdk color" from "Init" to "Open" will not change anything in
> practice, as long as Init and Open are supposed to be called only once.
> IMO, it's actually even worse, as the the Init function should check the
> initial_color parameter is correct (by doing the conversion). Having the
> "Open" function failing because of a bad parameter passed to "Init" function
> looks less obvious to me.

I would be fine in assuming that the passed colour is valid because it is a private API and we should trust the consumers to pass valid data.

> > @@ +135,5 @@
> > > +    case GTK_RESPONSE_DELETE_EVENT:
> > > +      result = nsIColorPicker::returnCancel;
> > > +      break;
> > > +    default:
> > > +      NS_WARNING("Unexpected response");
> > 
> > Shouldn't that be something like MOZ_ASSERT?
> 
> If MOZ_ASSERT is making the function failing, I don't think it's good idea:
> in case something went wrong with the color picker for a unknown reason, it
> seems to be better to act like if it was closed with "cancel".

MOZ_ASSERT() doesn't do anything on release builds. Using it on debug builds will actually make sure that developers might look into what is happening. Warnings are ignored. Crashed might get attention.

Comment 10

[Arnaud Bienner]

OK, thanks for the explanation regarding MOZ_ASSERT.
Karlt, do you agree with Mounir about how to use MOZ_ASSERT? So I can resubmit a new patch with all your comments applied.

Comment 11

[Arnaud Bienner]

Attachment: Remove useless code from file picker

As discussed in comment 2 and comment 7, here is a patch to remove useless code in Gtk's nsFilePicker.
Karl: could you please kindly review this patch?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Mounir Lamouri (:mounir) from comment #9)
> This is a private API and assuming
> that we get expected values isn't much to ask. 

I doubt we can control what might use this API.
Bug 875747 comment 3 even says "We want to be able to create JS-based implementations for Firefox OS".
JS should not abort on JS error.  It should throw.

> I would be fine in assuming that the passed colour is valid because it is a
> private API and we should trust the consumers to pass valid data.

The API in attachment 766269 [details] [diff] [review] doesn't even say what a valid color is.
The API should document that the error in parameters passed to init may be thrown in open, and that should be reviewed with the API, if that is what happens, but I would not implement it that way.

> MOZ_ASSERT() doesn't do anything on release builds. Using it on debug builds
> will actually make sure that developers might look into what is happening.
> Warnings are ignored. Crashed might get attention.

A JS or WebRT developer should not be unable to use his browser because of an assert in widget code that we don't know will remain true.

MOZ_ASSERT should only be used for situations that are always true.

(In reply to Arnaud from comment #10)
> OK, thanks for the explanation regarding MOZ_ASSERT.
> Karlt, do you agree with Mounir about how to use MOZ_ASSERT?

No,

> So I can resubmit a new patch with all your comments applied.

but this is widget code.  We don't need to come to an agreement with Mounir.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 767926 [details] [diff] [review]
Remove useless code from file picker

Thanks!

Comment 14

[Mounir Lamouri (:mounir)]

Thanks for the patch Arnaud. I think we should probably wait for nsIColorPicker.idl to land before landing this.

Comment 15

[Arnaud Bienner]

(In reply to Mounir Lamouri (:mounir) from comment #14)
> Thanks for the patch Arnaud. I think we should probably wait for
> nsIColorPicker.idl to land before landing this.

You're welcome :)
Indeed, I will wait for bug 875747 to be fixed before rewriting a new patch for nsColorPicker for Gtk.

But attachment 767926 [details] [diff] [review] is about removing useless code for Gtk file picker, so we can land it safely independently from the patch about color picker. (Karl spotted this is useless in comment 2 and told me it's fine to attach a patch for this in the same bug in comment 7, so if it's OK for you, I would glad if someone can checkin it, as I only have level 1 commit access so I can't do it myself).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3708552959e7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3708552959e7

Comment 18

[Arnaud Bienner]

Attachment: Color input: Gtk widget (3rd)

Here is an new patch with comments applied, and updated to match changes done in the IDL as part of bug 875747.

Main changes:
- I handle errors in parameters by returning an error code. I have no strong opinion about using error code or MOZ_ASSERT: IMO the most important is to have the error handled in some way.
- I've used Mozilla's strings API instead of stringstream.
- I've added a compat function for gtk_color_selection_dialog_get_color_selection. I've tested it by using the corresponding code directly in nsColorPicker.cpp, as I don't have a environment with Gtk < 2.10

And that time I've included function names in diff hunks ;)

Let me know if you have any questions about this.

Comment 19

[Arnaud Bienner]

Comment on attachment 771067 [details] [diff] [review]
Color input: Gtk widget (3rd)

Review of attachment 771067 [details] [diff] [review]:
-----------------------------------------------------------------

...and I also forgot to correct indentation in nsWidgetFactory.cpp
I will do it in the next patch, I promise!

::: widget/gtk2/nsColorPicker.cpp
@@ +5,5 @@
> +
> +
> +#include <iomanip>
> +#include <iostream>
> +#include <sstream>

Oups, I forgot to remove these (now useless) includes :(
But this should not prevent to perform the review.

Comment 20

[Arnaud Bienner]

Attachment: Color input: Gtk widget (4th)

Same as the previous one, but with the few missing things I noticed corrected.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 771403 [details] [diff] [review]
Color input: Gtk widget (4th)

I see you added an |mCallback = nullptr;| to handle the open|done|open|done
sequence, but a test on mCallback in Open() is still required to return a failure to prevent the open|open|done|done sequence, which won't work out well.

>+  nsDependentCSubstring withoutHash(StringTail(nsAutoCString(initialColor), 6)
);

Remove the nsAutoCString.  It is actually making |withoutHash| point to the
stack-allocated buffer in the nsAutoCString temporary, which won't exist after
this statement, and so the buffer will no longer be set aside.
StringTail(initialColor, 6) should work.

>+  gtk_object_get(GTK_OBJECT(colorseldialog), "color-selection", &colorsel, NULL);

Use g_object_get(), then no cast is required.

Please move this to a new gtkcolorseldialog.h.

Comment 22

[Arnaud Bienner]

Attachment: Color input: Gtk widget (5th)

Here is a new patch, with your comments applied + updated to take into account the IDL changes (use of nsString instead of nsCString).

Just one thing: StringTail returns a nsDependentSubstring, which inherits nsAString but not nsString, so I wasn't able to use NS_HexToRGB function to convert it. So I changed NS_HexToRGB to accept nsAString instead of nsString. I suppose there is no real reasons to not have nsAString here. But as NS_HexToRGB is used in lot of places, I would understand that you don't want to have this interface change as part of this patch. In that case, I suggest to do something like "nsDependentString withoutHash(initialColor).get() + 1, initialColor.Length() - 1);" like I did in my first patch, as nsDependentString will be accepted as a parameter for NS_HexToRGB.

Comment 23

[Arnaud Bienner]

Attachment: Color input: Gtk widget (5th) (inter-diff)

and here is the inter-diff.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 773449 [details] [diff] [review]
Color input: Gtk widget (5th)

>+  const nsAString& withoutHash  = StringTail(nsAutoString(initialColor), 6);

Still need to remove the nsAutoString temporary, which will be destroyed after this line.

In 12.2 Temporary Objects:

"A temporary holding the result of
an initializer expression for a declarator that declares a reference persists
until the end of the scope in which the reference declaration occurs.
[...]
In all these cases, the temporaries
created during the evaluation of the expression initializing the reference,
except the temporary to which the reference is bound, are destroyed at the end
of the full-expression in which they are created and in the reverse order of
the completion of their construction."

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to Arnaud from comment #22)
> Just one thing: StringTail returns a nsDependentSubstring, which inherits
> nsAString but not nsString, so I wasn't able to use NS_HexToRGB function to
> convert it. So I changed NS_HexToRGB to accept nsAString instead of
> nsString. I suppose there is no real reasons to not have nsAString here.

Yes, that is the right thing to do here.  nsString should not in general be used for parameters.

Comment 26

[Arnaud Bienner]

Attachment: Color input: Gtk widget (6th)

Indeed, I forgot to remove the nsAutoString; we don't need this here anyway.
Here is an updated version of the patch (hopefully the last one ;), with the nsAutoString removed.

I don't think it's worth to attach a interdiff as this is the only difference with the previous patch.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 780604 [details] [diff] [review]
Color input: Gtk widget (6th)

Please give your patches useful commit messages so I don't have to guess for you.

Comment 28

[Arnaud Bienner]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Please give your patches useful commit messages so I don't have to guess for
> you.

Sorry: you can use the bug's name:  "Implement <input type="color">: GTK widget/color picker"

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e6f23138fe

Comment 30

[Daniel Holbert [:dholbert]]

This introduced the following build warning:
{
23:40.40 In file included from ../../dist/include/nsISupportsUtils.h:26:0,
23:40.40                  from ../../dist/include/nsCOMPtr.h:34,
23:40.40                  from /mozilla-inbound/widget/gtk2/nsColorPicker.h:11,
23:40.40                  from /mozilla-inbound/widget/gtk2/nsColorPicker.cpp:10:
23:40.40 /mozilla-inbound/widget/gtk2/nsColorPicker.cpp: In member function 'virtual nsrefcnt nsColorPicker::Release()':
23:40.40 Warning: -Wdelete-non-virtual-dtor in /mozilla-inbound/../obj/dist/include/nsISupportsImpl.h: deleting object of polymorphic class type 'nsColorPicker' which has non-virtual destructor might cause undefined behaviour
23:40.40 ../../dist/include/nsISupportsImpl.h:506:52: warning: deleting object of polymorphic class type 'nsColorPicker' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
23:40.40    NS_IMPL_RELEASE_WITH_DESTROY(_class, delete (this))
23:40.40                                                     ^
23:40.41 ../../dist/include/nsISupportsImpl.h:486:5: note: in definition of macro 'NS_IMPL_RELEASE_WITH_DESTROY'
23:40.41      _destroy;                                                                 \
23:40.41      ^
23:40.41 ../../dist/include/nsISupportsImpl.h:1232:3: note: in expansion of macro 'NS_IMPL_RELEASE'
23:40.41    NS_IMPL_RELEASE(_class)                                                     \
23:40.41    ^
23:40.41 /mozilla-inbound/widget/gtk2/nsColorPicker.cpp:15:1: note: in expansion of macro 'NS_IMPL_ISUPPORTS1'
23:40.41  NS_IMPL_ISUPPORTS1(nsColorPicker, nsIColorPicker)
}

I intend to fix this by pushing a followup, annotating nsColorPicker as MOZ_FINAL.

Comment 31

[Daniel Holbert [:dholbert]]

Landed that followup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5b43a6b50a
(with blanket-r=ehsan for this sort of fix granted in bug 829975 comment 3)

Comment 32

[Arnaud Bienner]

Oops, I forgot this :/
Indeed, making the class final sounds like a good idea here.
Thanks Daniel for fixing this :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Arnaud from comment #28)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> > Please give your patches useful commit messages so I don't have to guess for
> > you.
> 
> Sorry: you can use the bug's name:  "Implement <input type="color">: GTK
> widget/color picker"

Your commit message should be saying what each patch does, not just restating the bug title.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63e6f23138fe
https://hg.mozilla.org/mozilla-central/rev/3b5b43a6b50a

Comment 35

[Arnaud Bienner]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33)
> Your commit message should be saying what each patch does, not just
> restating the bug title.

I know, but the bug title is what my patch does here.
Also, I thought a best practice in bugzilla is to have "one bug = one patch" (except for very small patches) so I'm used to have the bug title really reflecting what I want to commit.
But I understood I should take care of having a meaningful commit message in my patch files ;)
I will do in the future, I promise!

Comment 36

[Mounir Lamouri (:mounir)]

(In reply to Arnaud from comment #35)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33)
> > Your commit message should be saying what each patch does, not just
> > restating the bug title.
> 
> I know, but the bug title is what my patch does here.
> Also, I thought a best practice in bugzilla is to have "one bug = one patch"
> (except for very small patches) so I'm used to have the bug title really
> reflecting what I want to commit.
> But I understood I should take care of having a meaningful commit message in
> my patch files ;)
> I will do in the future, I promise!

You are right but for bug filed for real bugs (as opposed to features), you might have a title like "Doing Foo doesn't work when I also do Bar" but the patch will unlikely have that name and might be something like "Prevent race condition between Duck and Bird."

Comment 37

[Octoploid]

With --enable-default-toolkit=cairo-gtk2 I get:
...
/home/markus/mozilla-central/widget/gtk2/nsColorPicker.cpp:72:30: error: use of undeclared identifier 'gtk_color_selection_dialog_new'; did you mean
      'gtk_font_selection_dialog_new'?
  GtkWidget *color_chooser = gtk_color_selection_dialog_new(title);
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                             gtk_font_selection_dialog_new
/usr/include/gtk-2.0/gtk/gtkfontsel.h:176:12: note: 'gtk_font_selection_dialog_new' declared here
GtkWidget *gtk_font_selection_dialog_new               (const gchar            *title);
           ^
/home/markus/mozilla-central/widget/gtk2/nsColorPicker.cpp:83:9: error: use of undeclared identifier 'GTK_COLOR_SELECTION_DIALOG'
        GTK_COLOR_SELECTION_DIALOG(color_chooser))),
        ^
/usr/include/gtk-2.0/gtk/gtkcolorsel.h:41:66: note: expanded from macro 'GTK_COLOR_SELECTION'
#define GTK_COLOR_SELECTION(obj)                        (G_TYPE_CHECK_INSTANCE_CAST ((obj), GTK_TYPE_COLOR_SELECTION, GtkColorSelection))
                                                                                      ^
/usr/include/glib-2.0/gobject/gtype.h:481:80: note: expanded from macro 'G_TYPE_CHECK_INSTANCE_CAST'
#define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type)    (_G_TYPE_CIC ((instance), (g_type), c_type))
                                                                               ^
/usr/include/glib-2.0/gobject/gtype.h:1733:57: note: expanded from macro '_G_TYPE_CIC'
    ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt))
                                                        ^
/home/markus/mozilla-central/widget/gtk2/nsColorPicker.cpp:154:7: error: use of undeclared identifier 'GTK_COLOR_SELECTION_DIALOG'
      GTK_COLOR_SELECTION_DIALOG(color_chooser))),
      ^
/usr/include/gtk-2.0/gtk/gtkcolorsel.h:41:66: note: expanded from macro 'GTK_COLOR_SELECTION'
#define GTK_COLOR_SELECTION(obj)                        (G_TYPE_CHECK_INSTANCE_CAST ((obj), GTK_TYPE_COLOR_SELECTION, GtkColorSelection))
                                                                                      ^
/usr/include/glib-2.0/gobject/gtype.h:481:80: note: expanded from macro 'G_TYPE_CHECK_INSTANCE_CAST'
#define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type)    (_G_TYPE_CIC ((instance), (g_type), c_type))
                                                                               ^
/usr/include/glib-2.0/gobject/gtype.h:1733:57: note: expanded from macro '_G_TYPE_CIC'
    ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt))

Comment 38

[Arnaud Bienner]

Which version of Gtk are you using?
This function (gtk_font_selection_dialog_new) doesn't exist in old versions of Gtk.
A compat version was done, but it was buggy, and fixed in bug 901880.
If you can't wait for the patch to be landed, maybe you can try to apply it and let us know if the problem still occurs, or if it's something else?
The patch just rename widget/gtk2/compat/gtk/gtkcolorselectiondialog.h to widget/gtk2/compat/gtk/gtkcolorseldialog.h
You can do the same thing manually if you are not using Mercurial.

Comment 39

[Octoploid]

(In reply to Arnaud from comment #38)
> Which version of Gtk are you using?

2.24.20

> This function (gtk_font_selection_dialog_new) doesn't exist in old versions
> of Gtk.
> A compat version was done, but it was buggy, and fixed in bug 901880.
> If you can't wait for the patch to be landed, maybe you can try to apply it
> and let us know if the problem still occurs, or if it's something else?
> The patch just rename widget/gtk2/compat/gtk/gtkcolorselectiondialog.h to
> widget/gtk2/compat/gtk/gtkcolorseldialog.h
> You can do the same thing manually if you are not using Mercurial.

The patch that renames is already in my tree.

Comment 40

[Karl Tomlinson (:karlt)]

Need to fix up the include_next.

Comment 41

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #5)
> > Are you sure you don't want this to be modal, even when there is a parent?
> 
> I have no strong opinion about this.
> It's just that Gtk applications I've used doesn't make the color picker
> modal, so I prefer to keep the standard behavior. [etc]

(For reference, I filed bug 917917 to possibly re-evaluate this choice)