Add magnifier to rebaseline server

Add magnifier to rebaseline server.


NOTRY=true
BUG=skia:1917
Committed: http://code.google.com/p/skia/source/detail?r=13315

[epoger, 2014-02-04 16:45:51]

Looks pretty good, thanks for doing this!

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/diff_viewer.js (right):

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/diff_viewer.js:1: var MAX_SWAP_IMG_SIZE = 200;
I assume the previous version of this file was copied in from
compute_engine_scripts/telemetry/static-files/diff_viewer.js , as created in
https://codereview.chromium.org/148093012/ ('Add magnifying ability, perceptual
diff and improve layout of Skia Tryserver HTML output').  Right?

Do we envision maintaining this copy separately, or should rebaseline_server and
compute_engine_scripts/ share identical diff_viewer.js code?

As noted in the codereview linked above, I already had some comments on this
code...

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/view.html (right):

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:279: <tr ng-repeat="result in
limitedTestData" ng-controller="ImageController">
One thing I'm not sure about (being an AngularJS neophyte) is what happens when
different elements use different controllers.  Previously, all elements used
Loader.Controller.

I don't know if that might have anything to do with the pixel-diff images not
displaying until you mouseover their spaces, or not.

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:314: class="gm-image left-image" />
What's the significance of this class name?  Should it be different than the
class name of the expected image?

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:348: class="left-image"
Ditto: What's the significance of this class name?  Should it be different than
the class name of the whitediffs?

[rmistry, 2014-02-04 18:35:33]

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/diff_viewer.js (right):

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/diff_viewer.js:1: var MAX_SWAP_IMG_SIZE = 200;
On 2014/02/04 16:45:52, epoger wrote:
> I assume the previous version of this file was copied in from
> compute_engine_scripts/telemetry/static-files/diff_viewer.js , as created in
> https://codereview.chromium.org/148093012/ ('Add magnifying ability,
perceptual
> diff and improve layout of Skia Tryserver HTML output').  Right?

The previous version of this file is actually from tools/skpdiff/diff_viewer.js
> 
> Do we envision maintaining this copy separately, or should rebaseline_server
and
> compute_engine_scripts/ share identical diff_viewer.js code?

Having separate copies is probably ok.

This is my thinking:
CT will use the code in RBS once it is done. I am not going to add any more
features to CT's output HTML, I might add bugfixes if necessary but I really
want to use RBS for this because it has more features than I have, plus it has
the ability to handle multiple images on a page (by limiting them to 20 or 50)
instead of risking the page crashing like it does in CT.

> 
> As noted in the codereview linked above, I already had some comments on this
> code...

This code is a direct copy of the one in skpdiff, I do not completely understand
how it does what it does but it works in RBS. These are the changes I made:
* Changed it to handle 4 columns of images instead of the 3.
* Getting and setting the image source from view.html
* Reduced the max image size.
* Removed logic for sorting since you already have that in RBS.

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/view.html (right):

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:279: <tr ng-repeat="result in
limitedTestData" ng-controller="ImageController">
On 2014/02/04 16:45:52, epoger wrote:
> One thing I'm not sure about (being an AngularJS neophyte) is what happens
when
> different elements use different controllers.  Previously, all elements used
> Loader.Controller.
> 
> I don't know if that might have anything to do with the pixel-diff images not
> displaying until you mouseover their spaces, or not.


Yes maybe it is because of different controllers, I am still investigating.

The behavior is: When you first go to the page all images are there but when you
move the mouse on an image the rest disappear till you mouse over them.

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:314: class="gm-image left-image" />
On 2014/02/04 16:45:52, epoger wrote:
> What's the significance of this class name?  Should it be different than the
> class name of the expected image?

Does not look like the class name is important here, I had it like this
initially because that is how it was in skpdiff, but not needed for RBS or CT.

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/view.html:348: class="left-image"
On 2014/02/04 16:45:52, epoger wrote:
> Ditto: What's the significance of this class name?  Should it be different
than
> the class name of the whitediffs?

Removed as per comment above.

[epoger, 2014-02-04 18:50:14]

LGTM at patchset 4.

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/diff_viewer.js (right):

https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/diff_viewer.js:1: var MAX_SWAP_IMG_SIZE = 200;
On 2014/02/04 18:35:33, rmistry wrote:
> On 2014/02/04 16:45:52, epoger wrote:
> > I assume the previous version of this file was copied in from
> > compute_engine_scripts/telemetry/static-files/diff_viewer.js , as created in
> > https://codereview.chromium.org/148093012/ ('Add magnifying ability,
> perceptual
> > diff and improve layout of Skia Tryserver HTML output').  Right?
> 
> The previous version of this file is actually from
tools/skpdiff/diff_viewer.js

Ah.  So we currently have *3* diff_viewers:
1. here
2. tools/skpdiff
3. compute_engine_scripts/telemetry

We will allow them to diverge for now; eventually, we hope to unify on the RBS
UI anyway, and the others can be deleted.

[rmistry, 2014-02-04 19:38:05]

On 2014/02/04 18:50:14, epoger wrote:
> LGTM at patchset 4.
> 
>
https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
> File gm/rebaseline_server/static/diff_viewer.js (right):
> 
>
https://codereview.chromium.org/149473005/diff/50001/gm/rebaseline_server/sta...
> gm/rebaseline_server/static/diff_viewer.js:1: var MAX_SWAP_IMG_SIZE = 200;
> On 2014/02/04 18:35:33, rmistry wrote:
> > On 2014/02/04 16:45:52, epoger wrote:
> > > I assume the previous version of this file was copied in from
> > > compute_engine_scripts/telemetry/static-files/diff_viewer.js , as created
in
> > > https://codereview.chromium.org/148093012/ ('Add magnifying ability,
> > perceptual
> > > diff and improve layout of Skia Tryserver HTML output').  Right?
> > 
> > The previous version of this file is actually from
> tools/skpdiff/diff_viewer.js
> 
> Ah.  So we currently have *3* diff_viewers:
> 1. here
> 2. tools/skpdiff
> 3. compute_engine_scripts/telemetry
> 
> We will allow them to diverge for now; eventually, we hope to unify on the RBS
> UI anyway, and the others can be deleted.

Yes that SGTM.

I figured out the problem with the images disappearing it was because of some
MarkSize code in diff_viewer.js that we did not need but it was interfering with
the canvas painting.
I did not see this in CT because I had commented out two lines that were causing
JS errors (I did not see those JS errors in RBS so I left them in).
I went ahead and cleaned up a bunch of code relating to MaskSizes from
diff_viewer.js

PTAL @patchset5

[epoger, 2014-02-04 19:55:52]

LGTM at patchset 5

[rmistry, 2014-02-04 20:05:20]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-02-04 20:05:45]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/rmistry@google.com/149473005/160002

[commit-bot: I haz the power, 2014-02-04 20:59:51]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/rmistry@google.com/149473005/160002

[rmistry, 2014-02-04 22:08:59]

The CQ bit was unchecked by rmistry@google.com

[rmistry, 2014-02-04 22:09:01]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-02-04 22:22:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:22:44]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[rmistry, 2014-02-04 22:45:24]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2014-02-04 22:45:28]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/rmistry@google.com/149473005/160002

[commit-bot: I haz the power, 2014-02-04 22:45:43]

Change committed as 13315