Bug 33368 - Show patch status in bugs.webkit.org
Summary: Show patch status in bugs.webkit.org
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 50496
  Show dependency treegraph
 
Reported: 2010-01-08 01:19 PST by Adam Barth
Modified: 2010-12-03 15:43 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.39 KB, patch)
2010-01-08 01:20 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2010-01-08 02:04 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2010-01-08 13:31 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-01-08 01:19:19 PST
Add StatusBubbleExtension
Comment 1 Adam Barth 2010-01-08 01:20:44 PST
Created attachment 46118 [details]
Patch
Comment 2 Mark Rowe (bdash) 2010-01-08 01:25:55 PST
I don’t think it makes sense to add a Chrome-specific extension to the WebKit SVN repository in this fashion.
Comment 3 WebKit Review Bot 2010-01-08 01:28:09 PST
style-queue ran check-webkit-style on attachment 46118 [details] without any errors.
Comment 4 Mark Rowe (bdash) 2010-01-08 01:33:53 PST
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.
Comment 5 Adam Barth 2010-01-08 01:34:59 PST
Ok.  I'll work on a patch for BugsSite.
Comment 6 Eric Seidel (no email) 2010-01-08 01:44:27 PST
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.
Comment 7 Mark Rowe (bdash) 2010-01-08 01:52:52 PST
(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.
Comment 8 Adam Barth 2010-01-08 02:04:43 PST
Created attachment 46121 [details]
Patch
Comment 9 WebKit Review Bot 2010-01-08 02:12:15 PST
style-queue ran check-webkit-style on attachment 46121 [details] without any errors.
Comment 10 David Kilzer (:ddkilzer) 2010-01-08 07:56:25 PST
(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?
Comment 11 Adam Barth 2010-01-08 09:39:11 PST
> 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.
Comment 12 David Kilzer (:ddkilzer) 2010-01-08 10:12:29 PST
(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.
Comment 13 David Kilzer (:ddkilzer) 2010-01-08 10:15:31 PST
(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>
Comment 14 David Kilzer (:ddkilzer) 2010-01-08 12:04:47 PST
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.
Comment 15 Adam Barth 2010-01-08 13:31:04 PST
Created attachment 46154 [details]
Patch
Comment 16 WebKit Review Bot 2010-01-08 13:33:29 PST
style-queue ran check-webkit-style on attachment 46154 [details] without any errors.
Comment 17 Darin Adler 2010-01-08 13:42:01 PST
Comment on attachment 46154 [details]
Patch

Looks good. Lets give it a try!
Comment 18 WebKit Commit Bot 2010-01-08 14:05:14 PST
Comment on attachment 46154 [details]
Patch

Clearing flags on attachment: 46154

Committed r53007: <http://trac.webkit.org/changeset/53007>
Comment 19 WebKit Commit Bot 2010-01-08 14:05:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 David Kilzer (:ddkilzer) 2010-01-08 15:28:05 PST
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>
Comment 21 Adam Barth 2010-01-08 15:33:23 PST
I copied the logic from the last table cell, which also doesn't seem to be showing up.  Is it supposed to show up?
Comment 22 David Kilzer (:ddkilzer) 2010-01-08 15:37:56 PST
(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.)
Comment 23 David Kilzer (:ddkilzer) 2010-01-08 18:15:18 PST
(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.