Putting Splinter on bugzilla.gnome.org

[Owen Taylor]

After a couple of more weekend hacking sessions on Splinter, I'm pretty
happy with where it is.

As compared to where it was when I wrote:

http://blog.fishsoup.net/2009/09/23/splinter-patch-review/

What's new is:

 * A Bugzilla extension for tight integration:

   - Links from review comments to the review
   - Links from the attachment table to review pages
   - Posting reviews with attachment status changes doesn't
     send two emails.
   - Integrated appearance (Bugzilla header/footer)

* A cleaned up UI - it's not necessarily much more functional,
  but should be more easily understood. By breaking up the
  review by files it also has less of a performance problem
  on really gigantic patches.

* And even a bit of user documentation:

    http://splinter.fishsoup.net/help.html

I have more stuff I want to do with it eventually (allow specifying what
lines are being commented, keynav, getting extra context from Git,
tracking of what patches obsolete what, etc), but I think it's useful as
is and what I need most is user feedback.

(Next weekend, I'll be at the GNOME Summit rather than hacking on
Splinter... so it's not immediately going to get better by waiting.)

My thought at this point is to just to go ahead and install it onto
bugzilla.gnome.org later this week once the GNOME Shell 2.28.0 preview
release is out the door. There's always a chance to break something, but
I think it's pretty small here and I'll be around to fix anything I
break.

I certainly wouldn't mind people poking around with it and looking at
the code (of the Bugzilla extension in particular), but I don't think
putting it on bugzilla.gnome.org needs to block on that.

Yell now if you object.

- Owen


_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Olav Vitters]

On Sun, Oct 04, 2009 at 11:02:36PM -0400, Owen Taylor wrote:
>  * A Bugzilla extension for tight integration:
>
>    - Links from review comments to the review
>    - Links from the attachment table to review pages
>    - Posting reviews with attachment status changes doesn't
>      send two emails.
>    - Integrated appearance (Bugzilla header/footer)

It is an extension that relies on something being installed on the
server? How does Splinter connect with Bugzilla? Why have splinter be
seperate? Shouldn't everything just stay within the Bugzilla extension?

What happens if you use an old browser btw? I don't want it to appear if
you do not have the right browser. Further, if someone still posts the
link, it should fall back properly (e.g. display an warning). At the
moment it is very confusing if you see it in IE7.

>     http://splinter.fishsoup.net/help.html
>
> I have more stuff I want to do with it eventually (allow specifying what
> lines are being commented, keynav, getting extra context from Git,
> tracking of what patches obsolete what, etc), but I think it's useful as
> is and what I need most is user feedback.

> My thought at this point is to just to go ahead and install it onto
> bugzilla.gnome.org later this week once the GNOME Shell 2.28.0 preview
> release is out the door. There's always a chance to break something, but
> I think it's pretty small here and I'll be around to fix anything I
> break.

We'll need to figure out how to properly handle that extension. This as
I want everything in one module, no mix of different things. Perhaps you
should commit the extension to the Bzr repos of b.g.o.
Further, splinter itself should be on git.gnome.org; I saw a typo in a
comment in the webservice part which I couldn't fix.

btw: Bugzilla HEAD attachment.cgi is a bit nicer.

I did wonder why you repeated the login method in the webservice part.

> Yell now if you object.

Seems ok, just wonder about the Splinter <-> Bugzilla interaction. How
Splinter retrieves information. I see Splinter->Bugzilla is just
XML-RPC, wonder about Bugzilla->Splinter.

--
Regards,
Olav
_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Owen Taylor]

On Wed, 2009-10-07 at 11:27 +0200, Olav Vitters wrote:

> On Sun, Oct 04, 2009 at 11:02:36PM -0400, Owen Taylor wrote:
> >  * A Bugzilla extension for tight integration:
> >
> >    - Links from review comments to the review
> >    - Links from the attachment table to review pages
> >    - Posting reviews with attachment status changes doesn't
> >      send two emails.
> >    - Integrated appearance (Bugzilla header/footer)
>
> It is an extension that relies on something being installed on the
> server? How does Splinter connect with Bugzilla? Why have splinter be
> seperate? Shouldn't everything just stay within the Bugzilla extension?

Well, two things here:

 - I don't want Splinter to be bugzilla.gnome.org specific. So it
   shouldn't be just checked into our bzr repository under extensions/.

 - To keep the (large amounts) of Javascript manageable, it's maintained
   in separate files then "compiled" into a single file. This means that
   more than a simple checkout into extensions/ is needed.

So the way I set it up is that you do a
'make install BUGZILLA_ROOT=/path/to/bugzilla'.

But it really is structurally just a bugzilla extension (with a ton of
Javascript code in it.) Or is that way when you run it as an extension -
there's also a standalone "proxy" mode for development.

> What happens if you use an old browser btw? I don't want it to appear if
> you do not have the right browser. Further, if someone still posts the
> link, it should fall back properly (e.g. display an warning). At the
> moment it is very confusing if you see it in IE7.

Well, depends on the old browser - if your browser just doesn't support
DOM Storage you just don't get saved drafts - if you leave the page you
lose your in-progress review.

Or that's what's supposed to happen. Like any web page, it probably
doesn't work perfectly cross-browser without some testing and work.
I'm not completely sure how hard IE7 is going to be - I forget the
capabilities and limitations there, but it's certainly possible to get
to work Epiphany/Chrome/IE8/Safari

If you have something old and non-conformant like IE6 the page probably
just falls apart. The intersection of IE6 users and people who are
viewing and reviewing patches in GNOME bugzilla is empty.

I can put some work in on testing and fixing on other browsers than
Firefox and adding warnings if you are using IE6 ... maybe not
immediately but soon.

> >     http://splinter.fishsoup.net/help.html
> >
> > I have more stuff I want to do with it eventually (allow specifying what
> > lines are being commented, keynav, getting extra context from Git,
> > tracking of what patches obsolete what, etc), but I think it's useful as
> > is and what I need most is user feedback.
>
> > My thought at this point is to just to go ahead and install it onto
> > bugzilla.gnome.org later this week once the GNOME Shell 2.28.0 preview
> > release is out the door. There's always a chance to break something, but
> > I think it's pretty small here and I'll be around to fix anything I
> > break.
>
> We'll need to figure out how to properly handle that extension. This as
> I want everything in one module, no mix of different things. Perhaps you
> should commit the extension to the Bzr repos of b.g.o.

When used as an extension, all the Splinter Javascript/HTML is "part of
the extension" - there's not really a separate extension that could be
checked into the b.g.o code.

> Further, splinter itself should be on git.gnome.org; I saw a typo in a
> comment in the webservice part which I couldn't fix.

Yeah, I mentioned moving it in the earlier private mail I sent. I'm
happy to do that, I just don't want it merged into our Bugzilla bzr
branch.

> btw: Bugzilla HEAD attachment.cgi is a bit nicer.

If attachment.cgi&action=edit has a text area bigger than 10 rows by 4
columns, everybody will cheer.

> I did wonder why you repeated the login method in the webservice part.

Are you think thinking of the Splinter.info() method? That returns:

  { 'version': 1,
    'logged_in': true,
    'login': otaylor@...,
    'name': "Owen Taylor" }

The version is the Splinter extension API version. The rest of it is
stuff that there *should* be a way to retrieve in the standard Bugzilla
RPC methods, but isn't. (Having it here does save a round-trip, but I
wouldn't have duplicated it if it was available.)

So far I'm trying to make things work with stock Bugzilla 3.4, so I
don't want to patching the standard webservices and depending on that.

The call to Bugzilla::login in:

 my $user = Bugzilla->login;

Doesn't set new cookies or check the password or anything, it's just the
standard way to get the login information from the existing cookies sent
with the request.

> > Yell now if you object.
>
> Seems ok, just wonder about the Splinter <-> Bugzilla interaction. How
> Splinter retrieves information. I see Splinter->Bugzilla is just
> XML-RPC, wonder about Bugzilla->Splinter.

Splinter just uses xmlHttpRequest to fetch:

 /show_bug.cgi?ctype=xml
 /attachment.cgi?action=view

To get bug and attachment data.

- Owen


_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Olav Vitters]

On Wed, Oct 07, 2009 at 11:09:35AM -0400, Owen Taylor wrote:

> On Wed, 2009-10-07 at 11:27 +0200, Olav Vitters wrote:
> > On Sun, Oct 04, 2009 at 11:02:36PM -0400, Owen Taylor wrote:
> > >  * A Bugzilla extension for tight integration:
> > >
> > >    - Links from review comments to the review
> > >    - Links from the attachment table to review pages
> > >    - Posting reviews with attachment status changes doesn't
> > >      send two emails.
> > >    - Integrated appearance (Bugzilla header/footer)
> >
> > It is an extension that relies on something being installed on the
> > server? How does Splinter connect with Bugzilla? Why have splinter be
> > seperate? Shouldn't everything just stay within the Bugzilla extension?
>
> Well, two things here:
>
>  - I don't want Splinter to be bugzilla.gnome.org specific. So it
>    shouldn't be just checked into our bzr repository under extensions/.

I didn't mean that for the Bzr repos. The b.g.o repos should just have a
copy of splinter. Basically, the make install bits. However, I thought
there were other parts that aren't just in the extension.

>  - To keep the (large amounts) of Javascript manageable, it's maintained
>    in separate files then "compiled" into a single file. This means that
>    more than a simple checkout into extensions/ is needed.
>
> So the way I set it up is that you do a
> 'make install BUGZILLA_ROOT=/path/to/bugzilla'.

I don't like that. Just make install it to the b.g.o repos, then commit.
If I update the server I already have to:
bzr up
./checksetup.pl -t
sudo service httpd restart

That the splinter extension is compiled and so on I don't care about if
I 'bzr up'. I don't want e.g. 'bzr up' && 'git pull --rebase' && 'make
install FOO' for b.g.o.

> But it really is structurally just a bugzilla extension (with a ton of
> Javascript code in it.) Or is that way when you run it as an extension -
> there's also a standalone "proxy" mode for development.

Ah ok, I thought it relied on some daemon on the server.

> > What happens if you use an old browser btw? I don't want it to appear if
> > you do not have the right browser. Further, if someone still posts the
> > link, it should fall back properly (e.g. display an warning). At the
> > moment it is very confusing if you see it in IE7.
>
> Well, depends on the old browser - if your browser just doesn't support
> DOM Storage you just don't get saved drafts - if you leave the page you
> lose your in-progress review.
>
> Or that's what's supposed to happen. Like any web page, it probably
> doesn't work perfectly cross-browser without some testing and work.
> I'm not completely sure how hard IE7 is going to be - I forget the
> capabilities and limitations there, but it's certainly possible to get
> to work Epiphany/Chrome/IE8/Safari

I don't care if it is cross-browser btw, just that it fails properly
when some unsupported browser is used.

> If you have something old and non-conformant like IE6 the page probably
> just falls apart. The intersection of IE6 users and people who are
> viewing and reviewing patches in GNOME bugzilla is empty.
>
> I can put some work in on testing and fixing on other browsers than
> Firefox and adding warnings if you are using IE6 ... maybe not
> immediately but soon.

Only just trap the place it fails, then show a warning.

> > >     http://splinter.fishsoup.net/help.html
> > >
> > > I have more stuff I want to do with it eventually (allow specifying what
> > > lines are being commented, keynav, getting extra context from Git,
> > > tracking of what patches obsolete what, etc), but I think it's useful as
> > > is and what I need most is user feedback.
> >
> > > My thought at this point is to just to go ahead and install it onto
> > > bugzilla.gnome.org later this week once the GNOME Shell 2.28.0 preview
> > > release is out the door. There's always a chance to break something, but
> > > I think it's pretty small here and I'll be around to fix anything I
> > > break.
> >
> > We'll need to figure out how to properly handle that extension. This as
> > I want everything in one module, no mix of different things. Perhaps you
> > should commit the extension to the Bzr repos of b.g.o.
>
> When used as an extension, all the Splinter Javascript/HTML is "part of
> the extension" - there's not really a separate extension that could be
> checked into the b.g.o code.

I was still in the assumption it needed files elsewhere on the server
(something like a daemon).

> > Further, splinter itself should be on git.gnome.org; I saw a typo in a
> > comment in the webservice part which I couldn't fix.
>
> Yeah, I mentioned moving it in the earlier private mail I sent. I'm
> happy to do that, I just don't want it merged into our Bugzilla bzr
> branch.

Only the 'make install' result, nothing more. I am not proposing you use
Bzr for splinter, just only for b.g.o (I could check it in).

> > btw: Bugzilla HEAD attachment.cgi is a bit nicer.
>
> If attachment.cgi&action=edit has a text area bigger than 10 rows by 4
> columns, everybody will cheer.

I meant the code, not the UI :-) or :-/

> > I did wonder why you repeated the login method in the webservice part.
>
> Are you think thinking of the Splinter.info() method? That returns:
>
>   { 'version': 1,
>     'logged_in': true,
>     'login': otaylor@...,
>     'name': "Owen Taylor" }
>
> The version is the Splinter extension API version. The rest of it is
> stuff that there *should* be a way to retrieve in the standard Bugzilla
> RPC methods, but isn't. (Having it here does save a round-trip, but I
> wouldn't have duplicated it if it was available.)

Ah ok. Still, we should enhance the standard RPC stuff though.

> So far I'm trying to make things work with stock Bugzilla 3.4, so I
> don't want to patching the standard webservices and depending on that.
>
> The call to Bugzilla::login in:
>
>  my $user = Bugzilla->login;
>
> Doesn't set new cookies or check the password or anything, it's just the
> standard way to get the login information from the existing cookies sent
> with the request.

Yeah, I quickly read the code, together with things like
check_can_access I wondered if you didn't copy too much. In Bugzilla
HEAD it would be saner.

> > > Yell now if you object.
> >
> > Seems ok, just wonder about the Splinter <-> Bugzilla interaction. How
> > Splinter retrieves information. I see Splinter->Bugzilla is just
> > XML-RPC, wonder about Bugzilla->Splinter.
>
> Splinter just uses xmlHttpRequest to fetch:
>
>  /show_bug.cgi?ctype=xml
>  /attachment.cgi?action=view
>
> To get bug and attachment data.

Anyway, main thing for me is if you agree with putting the result of
'make install' in Bzr. So not the splinter repository.

--
Regards,
Olav
_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Owen Taylor]

On Wed, 2009-10-07 at 18:05 +0200, Olav Vitters wrote:

> On Wed, Oct 07, 2009 at 11:09:35AM -0400, Owen Taylor wrote:
> > On Wed, 2009-10-07 at 11:27 +0200, Olav Vitters wrote:
> > > On Sun, Oct 04, 2009 at 11:02:36PM -0400, Owen Taylor wrote:
> > > >  * A Bugzilla extension for tight integration:
> > > >
> > > >    - Links from review comments to the review
> > > >    - Links from the attachment table to review pages
> > > >    - Posting reviews with attachment status changes doesn't
> > > >      send two emails.
> > > >    - Integrated appearance (Bugzilla header/footer)
> > >
> > > It is an extension that relies on something being installed on the
> > > server? How does Splinter connect with Bugzilla? Why have splinter be
> > > seperate? Shouldn't everything just stay within the Bugzilla extension?
> >
> > Well, two things here:
> >
> >  - I don't want Splinter to be bugzilla.gnome.org specific. So it
> >    shouldn't be just checked into our bzr repository under extensions/.
>
> I didn't mean that for the Bzr repos. The b.g.o repos should just have a
> copy of splinter. Basically, the make install bits. However, I thought
> there were other parts that aren't just in the extension.
>
> >  - To keep the (large amounts) of Javascript manageable, it's maintained
> >    in separate files then "compiled" into a single file. This means that
> >    more than a simple checkout into extensions/ is needed.
> >
> > So the way I set it up is that you do a
> > 'make install BUGZILLA_ROOT=/path/to/bugzilla'.
>
> I don't like that. Just make install it to the b.g.o repos, then commit.
> If I update the server I already have to:
> bzr up
> ./checksetup.pl -t
> sudo service httpd restart
>
> That the splinter extension is compiled and so on I don't care about if
> I 'bzr up'. I don't want e.g. 'bzr up' && 'git pull --rebase' && 'make
> install FOO' for b.g.o.

Hmm, that's possible. And I guess it would help make test installs of
bugzilla.gnome.org more like the real server.

But I'm not completely happy with it. First, it strikes me as a little
awkward and confusing. To have an extension/ in the bzr repo that looks
a lot like the other extensions, but that you must not edit.

It hasn't really made updating Splinter any more like updating the rest
of Bugzilla - it's just moved the 'make install' procedure that has to
be documented from the server to the developer's machine.

But more substantively, if I have some Splinter fix I want to put into
place, I don't want to have to risk redeploying all the
bugzilla.gnome.org code. What if there are some other changes that are
sitting in the repository that haven't been deployed yet?

I'd be happier, especially while Splinter is still under active
development, if the install procedures for the bugzilla code and for
Splinter were orthogonal and separate:

 - To update bugzilla, follow the instructions you listed above

 - To update Splinter:

    cd /usr/local/www/splinter
    git pull
    make install
    sudo service httpd restart
 
- Owen


_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Olav Vitters]

On Wed, Oct 07, 2009 at 12:26:52PM -0400, Owen Taylor wrote:
> It hasn't really made updating Splinter any more like updating the rest
> of Bugzilla - it's just moved the 'make install' procedure that has to
> be documented from the server to the developer's machine.

I prefer the complexity to be on the developers machine.

> I'd be happier, especially while Splinter is still under active
> development, if the install procedures for the bugzilla code and for
> Splinter were orthogonal and separate:

Yeah, while you're developing it, then it makes sense to have it
separate. I just wont touch it :-)

btw: you said somewhere the look is similar. You just immediately
include the header and footer templates I hope? Frederic had an
extension to change these into something more GNOMEy like.

>  - To update bugzilla, follow the instructions you listed above
>
>  - To update Splinter:
>
>     cd /usr/local/www/splinter
>     git pull
>     make install
>     sudo service httpd restart

I still always use git pull --rebase, even if there shouldn't be any
changes on the server.

--
Regards,
Olav
_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure

[Owen Taylor]

On Wed, 2009-10-07 at 11:09 -0400, Owen Taylor wrote:
> I can put some work in on testing and fixing on other browsers than
> Firefox and adding warnings if you are using IE6 ... maybe not
> immediately but soon.

- Did some testing
- Spent some time making it work pretty well with IE7. (Without
  any real justification, got sucked in by the challenge, but some of
  the problems with IE were actual bugs in the Splinter code so so it
  wasn't completely wasted time.)
- Added a warning for IE6

Based on the stats below, time spent on Opera might have been more worth
it than time spent on IE... though they are both in the 1% range.

- Owen

User-Agent Stats on POST to /process_bug.cgi last week
======================================================
(People changing bugs probably are most representative of people
who would want to use Splinter, then general access to Bugzilla, which
tends to get distorted by random google hits)

Total - 3104

Gecko - 2319
  Firefox/3.5.3 - 976
  Firefox/3.0.14 - 384
  Firefox/3.5.2 - 224
  Epiphany/2.22 Firefox/3.0 - 126
  Epiphany/2.2 Firefox/3.5 - 112
  Iceweasel/3.0.14 - 72

WebKit - 684
  Unidentified Linux (Epiphany?) - 454
  Chrome - 188
  Midori - 19
  Safari - 10
  Iron - 7
  Epiphany - 4

Opera - 43
  10.00 - 43

git-bz - 35

MSIE - 20
  7.0 - 11
  8.0 - 7
  6.0 - 7

Konqueror - 2

Testing results
===============

Firefox-3.5.3, Fedora 11

Development platform, no known problems. DOM Storage and draft
saving work out of the box.

Epiphany 2.28.0 (webkitgtk-1.1.15), Fedora 12

textareas a few pixels too short at the bottom. DOM Storage and
draft saving work out of the box.

Chromium 4.0.220.0-0.1.20090930svn27599, Fedora 11

textareas a few pixels misaligned at the right, and a few pixels
too short at the bottom.  Needs --enable-local-storage on the command
line to enable DOM Storage and draft saving.

Konqueror 4.3.0

Comes up, looks good, but adding inline comments didn't work;
Apparently doesn't support dblclick event according to quirksmode.org.
No DOM Storage.

Firefox-3.5.1, OS X 10.5

textareas for inline comments a few pixels too short. DOM Storage and
draft saving work out of the box.

Safari 3.2.1, OS X 10.5

textareas a few pixels too short. No DOM Storage or draft saving. Draft
indicator on a separate line.

Firefox-3.0.8, Windows

textareas for inline comments a few pixels too short. No DOM Storage
or draft saving. Draft indicator on a separate line.

IE 7, Windows

Did not work out of the box, required fixes for a few bugs in splinter
and a small pile of minor workarounds. Remaining problems:

  - Overall Comment box doesn't change color on focus
  - textarea for inline comments have buggy partial borders
  - Draft indicator on a separate line

No DOM Storage or draft saving. Should be there for IE8, not tested.

IE6, Windows

Did not test, added prominent warning at the top of the page.

Operator 10, Windows

Mostly looks good, but seemed to be some problem parsing or
formatting reviews. Not quite functional. Didn't investigate further.


_______________________________________________
gnome-infrastructure mailing list
gnome-infrastructure@...
http://mail.gnome.org/mailman/listinfo/gnome-infrastructure