Real world impact script: scalable manual rendering QA

Real world impact script: scalable manual rendering QA

This patch is almost entirely by John Mellor <johnme@google.com>; [1]

Layout Tests can tell you whether rendering has changed, but it's often
hard to determine whether a subtle/controversial change is beneficial
without additional context.

This script aims to provide that context. It takes screenshots of 1000s
of sites, both before and after applying the patch being evaluated,
then diffs each pair of screenshots and sorts them by greatest
difference in rendering, in an HTML report. A human reviewer can then
manually skim through this to review the most impacted sites, rather
than having to browse random sites to see what their patch changed.

This is a script I wrote at the start of the year (but hadn't got round
to uploading a patch, as it used to depend on proprietary binaries;
I fixed that dependency by writing a replacement: crrev.com/67973005).

The script isn't yet smart enough to build content_shell itself, so you
run it in several steps:

1. Build content_shell in out/Release, without the controversial patch.
2. Run: real_world_impact.py before [num sites to test (default 1000)]
3. Apply the controversial patch, and rebuild content_shell in out/Release.
4. Run: real_world_impact.py after [num sites to test (default 1000)]
5. Run: real_world_impact.py compare [num sites to test (default 1000)]

[1] Original issue: https://codereview.chromium.org/112423006
I've made a few changes to the original patch:
1. Updated to support OSX and Windows content shells
2. Fixed the multiprocess fetch code so you can use ctrl+c to stop it
3. Added a timeout, retry limit, and no-check-certificate to wget so
sites download more reliably.
4. Updated the nsfw urls.
5. Added proper argument parsing.
6. Added optional "additional_flags" argument for doing before/after
comparisons with a flag.
7. Updated help text and example usage.

BUG=135823
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259024

[skobes, 2014-03-22 06:32:54]

https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
File tools/real_world_impact/real_world_impact.py (right):

https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
tools/real_world_impact/real_world_impact.py:195: "--timeout 20", # 20s timeout
I'd probably use a shorter timeout, like 5s.

https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
tools/real_world_impact/real_world_impact.py:506: elif arg in ['download',
'before', 'after', 'compare']:
Usage text doesn't mention 'download'...

[pdr., 2014-03-23 22:08:23]

I think many users will want to do before/after comparisons using content shell
flags. I've added a new optional argument in this patch ("--additional_flags")
so users can do before/after comparisons without rebuilding.

PTAL

On 2014/03/22 06:32:54, skobes wrote:
>
https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
> File tools/real_world_impact/real_world_impact.py (right):
> 
>
https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
> tools/real_world_impact/real_world_impact.py:195: "--timeout 20", # 20s
timeout
> I'd probably use a shorter timeout, like 5s.

Done.

> 
>
https://codereview.chromium.org/209393002/diff/20001/tools/real_world_impact/...
> tools/real_world_impact/real_world_impact.py:506: elif arg in ['download',
> 'before', 'after', 'compare']:
> Usage text doesn't mention 'download'...

I made a big change here by adding proper argument parsing and help text
printing. Sample output of --help is now:
[15:05:11]:~/Desktop/chromium/src $
./tools/real_world_impact/real_world_impact.py --help
usage: real_world_impact.py [-h] [--allow_js]
                            [--additional_flags ADDITIONAL_FLAGS]
                            [--num_sites NUM_SITES]
                            {download,before,after,compare}

Compare the real world impact of a content shell change.

positional arguments:
  {download,before,after,compare}
                        Action to perform.
                          download - Just download the sites.
                          before - Run content shell and record 'before' result.
                          after - Run content shell and record 'after' result.
                          compare - Compare before and after results.

optional arguments:
  -h, --help            show this help message and exit
  --allow_js            Don't disable Javascript
  --additional_flags ADDITIONAL_FLAGS
                        Additional flags to pass to content shell
  --num_sites NUM_SITES
                        Number of sites (default 100)

Example usage:
  1. Build content_shell in out/Release without any changes.
  2. Run: ./tools/real_world_impact/real_world_impact.py before [num sites to
test (default 100)].
  3. Either:
       a. Apply your controversial patch and rebuild content_shell.
       b. Pass --additional_flags="--enable_your_flag" in step 4.
  4. Run: ./tools/real_world_impact/real_world_impact.py after [num sites to
test (default 100)].
  5. Run: ./tools/real_world_impact/real_world_impact.py compare [num sites to
test (default 100)].
     This will open the results in your web browser.

[skobes, 2014-03-24 19:57:48]

lgtm

[pdr., 2014-03-24 20:00:40]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-24 20:00:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/209393002/80001

[commit-bot: I haz the power, 2014-03-24 21:58:45]

Change committed as 259024

[mdempsky, 2014-03-24 23:00:05]

https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
File tools/real_world_impact/nsfw_urls.py (right):

https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
tools/real_world_impact/nsfw_urls.py:41: "http://costco.com/",
I think snapdeal.com and costco.com are SFW?

[pdr., 2014-03-24 23:01:57]

On 2014/03/24 23:00:05, mdempsky wrote:
>
https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
> File tools/real_world_impact/nsfw_urls.py (right):
> 
>
https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
> tools/real_world_impact/nsfw_urls.py:41: "http://costco.com/",
> I think http://snapdeal.com and http://costco.com are SFW?

Good catch :) I have a small followup patch and will include a fix for these.

[timvolodine, 2014-03-26 13:36:59]

On 2014/03/24 23:01:57, pdr wrote:
> On 2014/03/24 23:00:05, mdempsky wrote:
> >
>
https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
> > File tools/real_world_impact/nsfw_urls.py (right):
> > 
> >
>
https://codereview.chromium.org/209393002/diff/80001/tools/real_world_impact/...
> > tools/real_world_impact/nsfw_urls.py:41: "http://costco.com/",
> > I think http://snapdeal.com and http://costco.com are SFW?
> 
> Good catch :) I have a small followup patch and will include a fix for these.

also, we had a wrapper for the old script where it was possible to simply do:
$ impact <git-branch-name>
which would run the comparison between the given branch and master branch
running all 1-5 steps automatically. I found that very convenient to use.