[webkit-reviews] review denied: [Bug 26950] Make the summary and alias fields support click-to-edit : [Attachment 32225] patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 26950: Make the summary and alias fields support click-to-edit
https://bugs.webkit.org/show_bug.cgi?id=26950

Attachment 32225: patch
https://bugs.webkit.org/attachment.cgi?id=32225&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
>-function hideEditableField( container, input, action, field_id,
original_value ) {
>+function hideEditableField( container, input, action, field1_text,
field2_text, field_id, original_value ) {
>     YAHOO.util.Dom.setStyle(container, 'display', 'inline');
>     YAHOO.util.Dom.setStyle(input, 'display', 'none');
>-    YAHOO.util.Event.addListener(action, 'click', showEditableField,
>+    YAHOO.util.Event.addListener(action, 'click', showEditableFieldFocusLast,

>+				   new Array(container, input));
>+    YAHOO.util.Event.addListener(field2_text, 'click',
showEditableFieldFocusLast,
>+				   new Array(container, input));
>+    YAHOO.util.Event.addListener(field1_text, 'click', showEditableField,
>				   new Array(container, input));

Since field2_text is field_id with "_nonedit_display" appended and field1_text
is "alias_nonedit_display", I don't think you need to add two parameters here.

Also, adding a new function, showEditableFieldFocusLast(), instead of adding
arguments to the array object goes against the design of the JavaScript used
elsewhere in the file.

>-function showEditableField (e, ContainerInputArray) {
>+function showEditableField (e, ContainerInputArray, focusLast) {

The "focusLast" argument should be passed in with the ContainerInputArray
argument.

>     }
>+
>+
>     YAHOO.util.Event.preventDefault(e);
> }

Gratuitous whitespace change.

>+function showEditableFieldFocusLast (e, ContainerInputArray) {
>+    showEditableField(e, ContainerInputArray, true);
>+}

This function won't be needed if you add the "focusLast" argument to the
ContainerInputArray.

>     hideEditableField( 'summary_alias_container','summary_alias_input',
>-			 'editme_action','short_desc', short_desc_value);  
>+			 'editme_action', 'alias_nonedit_display',
'short_desc_nonedit_display', 'short_desc', short_desc_value);	

Again, I think we only need to add one argument to hideEditableField().

r- for the above issues.  (Patches to Bugzilla should try to match the existing
design to make them easier to merge upstream.)

FWIW, this patch did work using a local copy of the original page (saved as
HTML) with a <base> tag added and the URL to the field.js adjusted to use a
local copy.

I have an updated patch that I'll post next.