Bug 33368 – Show patch status in bugs.webkit.org

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

Add StatusBubbleExtension

[Adam Barth]

Created attachment 46118 [details]
Patch

[Mark Rowe (bdash)]

I don’t think it makes sense to add a Chrome-specific extension to the WebKit SVN repository in this fashion.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46118 [details] without any errors.

[Mark Rowe (bdash)]

Comment on attachment 46118 [details]
Patch

bugs.webkit.org is a website that we control.  We should whatever functionality we need to it.  Marking as r- based on this, and the fact that WebKit SVN is not a repository of Chrome extensions.

[Adam Barth]

Ok.  I'll work on a patch for BugsSite.

[Eric Seidel (no email)]

I disagree that webkit.org will never host plugins/tools specific to WebKit which are used by other applications.  We certainly host scripts to integrate with other build systems and shells, which could be seen in a similar vein.

But I definitely agree that a better patch would be one for BugsSite instead of the extension.  The extension is awesome for testing, but BugsSite would help everyone.

[Mark Rowe (bdash)]

(In reply to comment #6)
> I disagree that webkit.org will never host plugins/tools specific to WebKit
> which are used by other applications.

I’m not sure who you’re disagreeing with, since no-one said that.

[Adam Barth]

Created attachment 46121 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46121 [details] without any errors.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Created an attachment (id=46121) [details]
> Patch

Is the iframe displayed on hover, or is it always present?  Where is the statusBubble class used in CSS?

[Adam Barth]

> Is the iframe displayed on hover, or is it always present?

It's always present.  I sent some screenshots to webkit-dev, or you can try installing the extension, which does the same thing:

https://chrome.google.com/extensions/detail/ahinhccldaefdhieflgmecppompikcnn

> Where is the statusBubble class used in CSS?

Nothing currently, but having class names is good semantics and can be quite useful down the road.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> > Is the iframe displayed on hover, or is it always present?
> 
> It's always present.  I sent some screenshots to webkit-dev, or you can try
> installing the extension, which does the same thing:
> 
> https://chrome.google.com/extensions/detail/ahinhccldaefdhieflgmecppompikcnn

When did you send screenshots to webkit-dev?  I don't recall seeing them.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > > Is the iframe displayed on hover, or is it always present?
> > 
> > It's always present.  I sent some screenshots to webkit-dev, or you can try
> > installing the extension, which does the same thing:
> > 
> > https://chrome.google.com/extensions/detail/ahinhccldaefdhieflgmecppompikcnn
> 
> When did you send screenshots to webkit-dev?  I don't recall seeing them.

Nevermind, I found them:

<https://lists.webkit.org/pipermail/webkit-dev/2009-December/011097.html>

[David Kilzer (:ddkilzer)]

Comment on attachment 46121 [details]
Patch

> diff --git a/BugsSite/template/en/custom/attachment/edit.html.tmpl b/BugsSite/template/en/custom/attachment/edit.html.tmpl
> index c6b5ab4..353c403 100644
> --- a/BugsSite/template/en/custom/attachment/edit.html.tmpl
> +++ b/BugsSite/template/en/custom/attachment/edit.html.tmpl
> @@ -256,6 +256,17 @@
>                                             attach_id = attachment.id %]<br>
>          [% END %]
>  
> +[%# if WEBKIT_CHANGES %]
> +        [% IF attachment.ispatch %]
> +        <div class="statusBubble">

I think this should have a label with it to separate it from the "Flags:" items:

             <b><small>Bot Status:</small></b>

> +          <iframe src="https://webkit-commit-queue.appspot.com/status-bubble/[% attachment.id %]"
> +                  style="width: 300px; height: 18px; border: none;" scrolling="no">
> +          </iframe>
> +        </div>
> +        <br>
> +        [% END %]
> +[%# endif // WEBKIT_CHANGES %]
> +
>          <div id="smallCommentFrame">
>            <b><small><label for="comment">Comment</label> (on the
>            [%+ terms.bug %]):</small></b><br>
> diff --git a/BugsSite/template/en/custom/attachment/list.html.tmpl b/BugsSite/template/en/custom/attachment/list.html.tmpl
> index be98425..fd2202a 100644
> --- a/BugsSite/template/en/custom/attachment/list.html.tmpl
> +++ b/BugsSite/template/en/custom/attachment/list.html.tmpl
> @@ -138,6 +138,15 @@
>              | <a href="attachment.cgi?id=[% attachment.id %]&amp;action=diff">Diff</a>
>            [% END %]
>            [% Hook.process("action") %]
> +[%# if WEBKIT_CHANGES %]
> +          [% IF attachment.ispatch %]
> +          <div class="statusBubble">
> +            <iframe src="https://webkit-commit-queue.appspot.com/status-bubble/[% attachment.id %]"
> +                    style="width: 300px; height: 18px; border: none;" scrolling="no">
> +            </iframe>
> +          </div>
> +          [% END %]
> +[%# endif // WEBKIT_CHANGES %]
>          </td>
>        </tr>
>      [% END %]

We may eventually want a separate table column for bot status, but adding it to this cell is fine for now.

The BugsSite/template/en/custom/attachment/reviewform.html.tmpl template also needs to be updated with this status.

r- to update reviewform.html.tmpl and add a label to edit.html.tmpl.

[Adam Barth]

Created attachment 46154 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46154 [details] without any errors.

[Darin Adler]

Comment on attachment 46154 [details]
Patch

Looks good. Lets give it a try!

[WebKit Commit Bot]

Comment on attachment 46154 [details]
Patch

Clearing flags on attachment: 46154

Committed r53007: <http://trac.webkit.org/changeset/53007>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Follow-up fix for the review form (to make the status actually appear):

$ svn commit BugsSite
Sending        BugsSite/ChangeLog
Sending        BugsSite/template/en/custom/attachment/reviewform.html.tmpl
Transmitting file data ..
Committed revision 53016.

<http://trac.webkit.org/changeset/53016>

[Adam Barth]

I copied the logic from the last table cell, which also doesn't seem to be showing up.  Is it supposed to show up?

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> I copied the logic from the last table cell, which also doesn't seem to be
> showing up.  Is it supposed to show up?

It is, but when I enabled it on the server, the functionality was broken.  I'm taking a look at it now.  (The button lets you switch between pretty and non-pretty patches in the upper frame.)

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> (In reply to comment #21)
> > I copied the logic from the last table cell, which also doesn't seem to be
> > showing up.  Is it supposed to show up?
> 
> It is, but when I enabled it on the server, the functionality was broken.  I'm
> taking a look at it now.  (The button lets you switch between pretty and
> non-pretty patches in the upper frame.)

I filed Bug 33410 to address this.