Summary: | Show patch status in bugs.webkit.org | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, commit-queue, ddkilzer, eric, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 50496 | ||||||||||
Attachments: |
|
Description
Adam Barth
2010-01-08 01:19:19 PST
Created attachment 46118 [details]
Patch
I don’t think it makes sense to add a Chrome-specific extension to the WebKit SVN repository in this fashion. style-queue ran check-webkit-style on attachment 46118 [details] without any errors.
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.
Ok. I'll work on a patch for BugsSite. 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. (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. Created attachment 46121 [details]
Patch
style-queue ran check-webkit-style on attachment 46121 [details] without any errors.
(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? > 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. (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. (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 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 %]&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. Created attachment 46154 [details]
Patch
style-queue ran check-webkit-style on attachment 46154 [details] without any errors.
Comment on attachment 46154 [details]
Patch
Looks good. Lets give it a try!
Comment on attachment 46154 [details] Patch Clearing flags on attachment: 46154 Committed r53007: <http://trac.webkit.org/changeset/53007> All reviewed patches have been landed. Closing bug. 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> I copied the logic from the last table cell, which also doesn't seem to be showing up. Is it supposed to show up? (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.) (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. |