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 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.

(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 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?

(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.

(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".

(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.

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.

(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.

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.

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?

(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 on attachment 767926 [details] [diff] [review] Remove useless code from file picker Thanks!

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

(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).

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 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.

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

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.

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.

and here is the inter-diff.

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."

(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.

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 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.

(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"

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.

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)

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

(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.

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

(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!

(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."

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))

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.

(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.

Need to fix up the include_next.

(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)