Bug 38143 – move chrome extension code for fancy-review into bugzilla

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ojan Vafai]

Hopefully in the process of making it part of bugzilla proper, we can remove some of our hacks we put in to make it work from pure JS.

Julie, you're going to do this right?

Eric, Adam, do you all know the process for making bugzilla changes? Do we just check in the code and then ping someone to restart bugzilla? Does it need a restart?

[Eric Seidel (no email)]

I'm not sure it needs any restart.  However ddkilzer or wms are your peeps if it does.

[William Siegrist]

If, after checking in the changes to trunk, it does not work still, ping me (_wms) on irc, or comment here, so I can reload things as needed.

[Julie Parent]

Created attachment 55439 [details]
Moves Rietveld integration from Chrome extension into Bugzilla proper.

[WebKit Review Bot]

Attachment 55439 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
BugsSite/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
BugsSite/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
BugsSite/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
BugsSite/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julie Parent]

Created attachment 55440 [details]
Fixes tabs in changelog.

[Adam Barth]

Comment on attachment 55440 [details]
Fixes tabs in changelog.

This looks reasonable to me, but I'm far from a bugzilla expert and I don't read Perl.  Who should we get to review this change?

BugsSite/ChangeLog:7
 +  
Extra blank line here.

BugsSite/ChangeLog:5
 +          Adds support for Rietveld reviews.  When their is an associated
their => there

BugsSite/template/en/custom/attachment/fancyreview.html.tmpl:57
 +          postMessage('publish', 'http://wkrietveld.appspot.com');
Should we detect the lack of postMessage and disable this feature?

BugsSite/template/en/custom/attachment/list.html.tmpl:144
 +              IF attachment.description.search('.*-([0-9]+)$') %]
I wish there was a better place to put the ID than the attachment description.

[Julie Parent]

Created attachment 55585 [details]
Addresses issues from abarth's review.

[Julie Parent]

(In reply to comment #6)
> (From update of attachment 55440 [details])
> This looks reasonable to me, but I'm far from a bugzilla expert and I don't read Perl.  Who should we get to review this change?

Eric added ddkilzer and wms to this cc list.  Hopefully one of them?  (pretty please!)
 
> BugsSite/ChangeLog:7
>  +  
> Extra blank line here.
> 
> BugsSite/ChangeLog:5
>  +          Adds support for Rietveld reviews.  When their is an associated
> their => there
> 
> BugsSite/template/en/custom/attachment/fancyreview.html.tmpl:57
>  +          postMessage('publish', 'http://wkrietveld.appspot.com');
> Should we detect the lack of postMessage and disable this feature?
> 
> BugsSite/template/en/custom/attachment/list.html.tmpl:144
>  +              IF attachment.description.search('.*-([0-9]+)$') %]
> I wish there was a better place to put the ID than the attachment description.

Agreed.  Ojan said he talked to you about some ideas for this, that he'll investigate this week.  I think we should move forward with this patch in the meantime, so we can test out all of the integration more.  It should be easy to change the Rietveld id part after the fact.

All other comments addressed.

[Julie Parent]

Comment on attachment 55585 [details]
Addresses issues from abarth's review.

Taking away review? while I modify this patch to not get the Rietveld id from the patch description, and to use the soon-to-be-added flag instead.

[Julie Parent]

Created attachment 55717 [details]
Changes fetching Rietveld ID from patch description to just using bug number and in_rietveld flag.

[Julie Parent]

Created attachment 55722 [details]
Updates ChangeLog and fixes type-o.

[William Siegrist]

You are submitting code licensed under the MPL. Has this been approved by anyone?

[Julie Parent]

(In reply to comment #12)
> You are submitting code licensed under the MPL. Has this been approved by anyone?

I actually just copied this from the standard review page: http://trac.webkit.org/browser/trunk/BugsSite/template/en/custom/attachment/review.html.tmpl

I wondered about the license when I saw it there, as that looked like a brand new file for webkit, but it had the MPL license on it.  For reference, https://bugs.webkit.org/show_bug.cgi?id=21315 is where it was added for that template.

Perhaps I should just remove it from fancyreview.html.tmpl (and review.html.tmpl)?

[David Kilzer (:ddkilzer)]

Comment on attachment 55722 [details]
Updates ChangeLog and fixes type-o.

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

[Julie Parent]

Created attachment 55876 [details]
Updates to address all comments from review.

[David Kilzer (:ddkilzer)]

Comment on attachment 55876 [details]
Updates to address all comments from review.

> Index: BugsSite/ChangeLog
> +        * template/en/custom/attachment/rietveldreview.html.tmpl: Added.
> +            New template for fancyreview mode.  Hosts the rietveld frame in the
> +            top and the regular comments form in the bottom.
> +            Uses postMessage to communicate with Rietveld frame.

Nit: "fancyreview" should be "reitveldreview" here.

r=me

[Julie Parent]

Committed in http://trac.webkit.org/changeset/59265.