RFR: JMC-5473: Quick search for Automated Analysis Result Table

[Henrik Dafgård]

This looks good to me!


Cheers,
Henrik Dafgård


On Tue, 24 Sep 2019 at 19:27, Jie Kang <jkang at redhat.com> wrote:

> On Wed, Sep 18, 2019 at 10:59 AM Alex Macdonald <almacdon at redhat.com>
> wrote:
> >
> > On Tue, Sep 17, 2019 at 5:20 PM Jie Kang <jkang at redhat.com> wrote:
> >>
> >> On Tue, Sep 3, 2019 at 2:57 PM Alex Macdonald <almacdon at redhat.com>
> wrote:
> >> >
> >> > On Wed, Aug 28, 2019 at 11:36 AM Jie Kang <jkang at redhat.com> wrote:
> >> >>
> >> >> On Wed, Aug 28, 2019 at 10:46 AM Jie Kang <jkang at redhat.com> wrote:
> >> >> >
> >> >> > Hi all,
> >> >> >
> >> >> > Carmine has requested I continue this work in his stead. Please
> find
> >> >> > an updated webrev for review below.
> >> >> >
> >> >> > Webrev: http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.00/
> >> >> > Bug: https://bugs.openjdk.java.net/browse/JMC-5473
> >> >>
> >> >> Ah; I had a mistake in the previous webrev (the TableViewer was no
> >> >> longer set), please find an updated one below. Apologies for the
> >> >> mistake.
> >> >>
> >> >> http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.01/
> >> >>
> >> >>
> >> >
> >> > Looks good - the functionality works as it should and the uitests are
> passing on both my Linux & Windows machines.
> >> >
> >> > There is an unused import that can be cleaned up at:
> >> >
> >> > ResultTableUi.java
> >> > - unused import GridData @ line 50
> >>
> >> Hi Alex,
> >>
> >> Nice catch thanks. I've removed the unused import.
> >>
> >> >
> >> > And my only other nit is that the test is placed in
> "org.openjdk.jmc.flightrecorder.uitest.overview" which would be a new
> package that would only have this test inside of it. It doesn't test (yet
> at least) the entire Automated Analysis page so maybe it doesn't fit under
> "[..].flightrecorder.uitest.pages", but maybe it belongs under
> [..].flightrecorder.uitest alongside the other tests? Just a thought.
> >>
> >> Sure; I've moved it to be under o.o.jmc.flightrecorder.uitest. Here's
> >> the updated webrev:
> >
> >
> > Great, thanks. LGTM.
> >
> >>
> >>
> >> http://cr.openjdk.java.net/~jkang/jmc-5473/webrev.02/
> >>
> >> How does it look?
>
> Thanks for the review Alex. Could I get a second reviewer for this?
>
>
> Regards,
>
> >>
> >>
> >> Regards,
> >>
>