User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 Build ID: 20160106234723 Steps to reproduce: Bug for tracking GTK implementation of a date picker.

Before applying: requires patches for Bug 825294 and Bug 1241239.

Comment on attachment 8710080 [details] [diff] [review] Create GTK widget for <input type=date> >-Add GTK widget for date input I looked at these changes in widget/gtk, which are looking close thanks. My apologies for the delay in getting to this. >-Move usefull date functions from HTMLInputElement to nsContentUtils "useful" Did smaug give you feedback on this change? That makes sense to me, but it is not really my area, so we should get him or someone to else to approve that. I can review the moving of code, if the right person has approved this. >+ mDialog = gtk_dialog_new_with_buttons(title, parent_window, flags, >+ ToNewUTF8String(cancelString), GTK_RESPONSE_REJECT, >+ ToNewUTF8String(selectString), GTK_RESPONSE_ACCEPT, >+ NULL); ToNewUTF8String() allocates a new string, which does not get released here. Better to use something like this which uses a stack buffer if the string is small: NS_ConvertUTF16toUTF8(cancelString).get(); That's also better than ToNewUTF8String for title, or you can use NS_ConvertUTF16toUTF8 title(mTitle); GTK color dialogs have an OK button, so I wonder whether OK would be more consistent than "Select". But "Select" seems more precise, so I'm happy if you'd prefer to keep that. >+ gtk_widget_destroy(mDialog); >+ NS_RELEASE_THIS(); Please reset mDialog and mCalendar, before release, so that we can be sure that these are not used after free. >+ case GTK_RESPONSE_ACCEPT: >+ { See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures for positioning of {}. >+ if(!mCallback) break; if( -> if ( Here and a couple of places below. >+ nsString result; nsAutoString >+ result.AppendInt(year); >+ result.Append('-'); >+ if(month < 10) result.Append('0'); >+ result.AppendInt(month); >+ result.Append('-'); >+ if(day < 10) result.Append('0'); >+ result.AppendInt(day); I'm guessing this can be simplified to AppendPrintf("%04u-%02u-%02u", year, month, day) I assume the year should have 4 digits?

As before - requires latest patch for Bug 825294 first. That should be all the changes made and updated to apply to latest nightly. I've kept the button names as they were. As for moving the functions to nsContentUtils Smaug is aware of the move and is ok with it.

Updated patch to go with changes to Bug 825294

Comment on attachment 8749994 [details] [diff] [review] Create GTK widget for <input type=date> >+ if (!nsContentUtils::DigitSubStringToNumber(aValue, 0, 2, &hours) || hours > 23) { >+ if (!nsContentUtils::DigitSubStringToNumber(aValue, 3, 2, &minutes) || minutes > 59) { >+ if (!nsContentUtils::DigitSubStringToNumber(aValue, 6, 2, &seconds) || seconds > 59) { >+ if (!nsContentUtils::DigitSubStringToNumber(aValue, 9, aValue.Length() - 9, &fractionsSeconds)) { Please break lines to stay within 80 columns. This file breaks lines after operators. >+ * @param aValue the string on which the sub-string will be extracted and parsed. >+ * @param aStart the beginning of the sub-string in aValue. >+ * @param aLen the length of the sub-string. >+ * @param aResult the parsed number. >+ * @return whether the sub-string has been parsed successfully. >+ */ >+ static bool DigitSubStringToNumber(const nsAString& aStr, Update the doc for the first and last parameter name changes. >+ //nsXPIDLCString title; >+ NS_ConvertUTF16toUTF8 title(mTitle); >+ nsXPIDLString selectString, cancelString; >+ //title.Adopt(ToNewUTF8String(mTitle)); Please remove the commented out nsXPIDLCString code. >+ mDialog = gtk_dialog_new_with_buttons(title.get(), parent_window, flags, >+ NS_ConvertUTF16toUTF8(cancelString).get(), GTK_RESPONSE_REJECT, >+ NS_ConvertUTF16toUTF8(selectString).get(), GTK_RESPONSE_ACCEPT, >+ NULL); Please break the lines after ',' or indent the arguments less (2 spaces from the start of gtk_dialog_new_with_buttons), to stay within 80 columns. Replace NULL with nullptr. >+ switch (response) { >+ case GTK_RESPONSE_ACCEPT: { >+ if (!mCallback) break; >+ >+ guint year, month, day; >+ gtk_calendar_get_date(GTK_CALENDAR(mCalendar), &year, &month, &day); >+ ++month; //GTK calendar months go from 0-11, we want 1-12 >+ >+ nsAutoString result; >+ result.AppendPrintf("%04u-%02u-%02u", year, month, day); >+ >+ mCallback->Done(result); >+ mCallback = nullptr; >+ break; >+ } >+ case GTK_RESPONSE_REJECT: >+ case GTK_RESPONSE_DELETE_EVENT: >+ mCallback->Cancel(); >+ mCallback = nullptr; >+ break; >+ default: >+ NS_WARNING("nsDatePicker: Unexpected response"); >+ break; Only the first case is handling !mCallback. I suspect this is not necessary, but if it is then the second case also needs to handle it. If we get an unexpected response from GTK, the signal handler is removed, so I think mCallback->Cancel() should be called. Move "default:" and NS_WARNING() to before "case GTK_RESPONSE_REJECT:" and remove the break, so that default falls through to the Cancel code. r+ with these changes. (In reply to jordandev678 from comment #3) > As for moving the functions to nsContentUtils Smaug is aware of the move and > is ok with it. That was https://bugzilla.mozilla.org/show_bug.cgi?id=825294#c58

The UI people have decided to use a generic date/week picker for the desktop instead of a native one - closing the GTK specific bug.

(In reply to jordandev678 from comment #6) > The UI people have decided to use a generic date/week picker for the desktop > instead of a native one - closing the GTK specific bug. What bug number is related to that decision? I don't see recent activity in bug 1069609

(In reply to Mardeg from comment #7) > (In reply to jordandev678 from comment #6) > > The UI people have decided to use a generic date/week picker for the desktop > > instead of a native one - closing the GTK specific bug. > > What bug number is related to that decision? I don't see recent activity in > bug 1069609 I believe the discussion in bug 1069609 got shunted into bug 888320. bug 888320, comment 30 seems to be the decision. If there are more questions I would ask in there.