[webkit-reviews] review denied: [Bug 38143] move chrome extension code for fancy-review into bugzilla : [Attachment 55722] Updates ChangeLog and fixes type-o.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Julie Parent
<jparent at google.com>'s request for review:
Bug 38143: move chrome extension code for fancy-review into bugzilla
https://bugs.webkit.org/show_bug.cgi?id=38143

Attachment 55722: Updates ChangeLog and fixes type-o.
https://bugs.webkit.org/attachment.cgi?id=55722&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Overall this looks good.  A few nits below:

> Index: BugsSite/attachment.cgi
> +elsif ($action eq "fancyreview")
> +{
> +    edit("fancyreview");
> +}

Can we find a better name than fancyreview?  Maybe reitveldreview?

> Index: BugsSite/template/en/custom/attachment/fancyreview.html.tmpl
> Property changes on:
BugsSite/template/en/custom/attachment/fancyreview.html.tmpl
> ___________________________________________________________________
> Added: svn:executable
>    + *

I don't think this file should be executable.

> Index: BugsSite/template/en/custom/attachment/list.html.tmpl
> +		   | <a href="attachment.cgi?id=[% attachment.id
%]&amp;action=fancyreview&amp;GoAheadAndLogIn=1">Fancy Review</a>

Again, can we use something more descriptive than fancy review like Reitveld
Review?

> Index: BugsSite/template/en/custom/attachment/reviewform.html.tmpl
> -<form method="post" action="attachment.cgi" target="_top"
onsubmit="omitUntouchedComment(); return true;">
> +<form method="post" action="attachment.cgi" target="_top" onsubmit=
> +[% IF fancyReview %]
> +"onFancyReviewSubmit(); return false;"
> +[% ELSE %]
> +"omitUntouchedComment(); return true;"
> +[% END %]
> +>

It would be nicer if we set a variable for the onsubmit contents instead of
breaking out the <form> tag this way.  This is just a style nit, though, and
doesn't have to be fixed.

> @@ -72,11 +100,10 @@
>    [% IF (Param("insidergroup") && user.in_group(Param("insidergroup"))) %]
>      <input type="hidden" name="isprivate" [% IF attachment.isprivate %]
value="1" [% ELSE %] value="0" [% END %] >
>    [% END %]
> -
>  <table style="width:100%; height:90%" cellpadding=0 cellspacing=0>

If this blank line isn't part of previous WebKit modifications, please don't
remove it.

> -    <button type="submit">Submit</button>
> +    <button id='submitBtn' type="submit">Submit</button>

Please use double quotes for the id name here.

r- to clean up a few nits above and to consider using a more descriptive name
than "fancy review".