Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1216483002:
golden/pdfxform a pdf rasterization server (Closed)
|
Created:
5 years, 6 months ago by hal.canary Modified:
5 years, 5 months ago CC:
jcgregorio, reviews_skia.org Base URL:
https://skia.googlesource.com/buildbot@master Target Ref:
refs/heads/master Project:
skiabuildbot Visibility:
Public. |
Descriptiongolden/pdfxform a pdf rasterization server
also: added PDF Rasterizer
BUG=skia:3721
Committed: https://skia.googlesource.com/buildbot/+/18a17c0a39d79df770d40bdcc528dce7bfee4fca
Patch Set 1 #
Total comments: 3
Patch Set 2 : 2015-06-26 (Friday) 11:26:48 EDT #
Total comments: 6
Patch Set 3 : 2015-06-26 (Friday) 13:16:51 EDT #Patch Set 4 : 2015-06-26 (Friday) 14:33:47 EDT #
Total comments: 21
Patch Set 5 : 2015-06-26 (Friday) 18:10:07 EDT #Patch Set 6 : 2015-06-26 (Friday) 18:37:58 EDT #
Total comments: 10
Patch Set 7 : 2015-07-07 (Tuesday) 14:55:58 EDT #
Total comments: 8
Patch Set 8 : 2015-07-09 (Thursday) 13:32:58 EDT #
Total comments: 4
Patch Set 9 : 2015-07-09 (Thursday) 13:58:11 EDT #Patch Set 10 : 2015-07-09 (Thursday) 14:48:41 EDT #Patch Set 11 : 2015-07-09 (Thursday) 15:12:53 EDT #Messages
Total messages: 40 (11 generated)
halcanary@google.com changed reviewers: + stephana@google.com
PTAL
On 2015/06/25 at 21:30:51, halcanary wrote: > PTAL Opposite to what I said in conversation yesterday, it would be very helpful to break this into two CLs. The go/pdf package L G T M and it would be good see if we can land that and pass the tests on the bot. And then review the server part. Please rename "pdfxform.go" to "main.go", the executable will be the name of the directory it is in.
jcgregorio@google.com changed reviewers: + jcgregorio@google.com
https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform.go File golden/go/pdfxform/pdfxform.go (right): https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform... golden/go/pdfxform/pdfxform.go:226: } // data/45aa8af265d16839402583df5756a7c6.png How does this image get deployed to the server, and uploaded to the digests directory? My preference would be that pdfxform take a command-line flag that points to the directory where this image is located. The normal name for such a flag is resourcesDir. See how it is defined and used in Gold: https://github.com/google/skia-buildbot/blob/master/golden/go/skiacorrectness...
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216483002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)
https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:39: type md5digest [md5.Size]byte I think treating MD5 hashes as byte slices (instead of hex strings) is overkill. Using strings throughout for MD5s is fine IMO and it avoids having function like decodeMd5(...). This is a largely IO bound process, if CPU matters at all it's how fast the rasterizers can convert a PDF to PNG. https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:122: // parsed and prooduced. Instead of the defining it's own structures to receive the results. It should re-use the ones from the gold ingester (see golden/go/goldingester/goldingester.go). That way any changes in the ingester will be reflected here and help detect modifications that need to be made. https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:221: var errorImageMd5 = md5digest{ Another argument to use strings instead of md5digest.
https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform.go File golden/go/pdfxform/pdfxform.go (right): https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform... golden/go/pdfxform/pdfxform.go:226: } // data/45aa8af265d16839402583df5756a7c6.png On 2015/06/26 15:09:23, jcgregorio wrote: > How does this image get deployed to the server, and uploaded to the digests > directory? > > My preference would be that pdfxform take a command-line flag that points to the > directory where this image is located. The normal name for such a flag is > resourcesDir. See how it is defined and used in Gold: > > > https://github.com/google/skia-buildbot/blob/master/golden/go/skiacorrectness... That would make sense if I had a lot of binary resources. Instead, I suggest that we simply embed the 500 bytes into the source code. This way, we can never fail to upload the file.
https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform.go File golden/go/pdfxform/pdfxform.go (right): https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform... golden/go/pdfxform/pdfxform.go:226: } // data/45aa8af265d16839402583df5756a7c6.png If this is the only external resource pdfxform needs then embedding is SGTM. On 2015/06/26 at 17:27:09, Hal Canary wrote: > On 2015/06/26 15:09:23, jcgregorio wrote: > > How does this image get deployed to the server, and uploaded to the digests > > directory? > > > > My preference would be that pdfxform take a command-line flag that points to the > > directory where this image is located. The normal name for such a flag is > > resourcesDir. See how it is defined and used in Gold: > > > > > > https://github.com/google/skia-buildbot/blob/master/golden/go/skiacorrectness... > > That would make sense if I had a lot of binary resources. Instead, I suggest that we simply embed the 500 bytes into the source code. This way, we can never fail to upload the file.
On 2015/06/26 at 17:27:10, halcanary wrote: > https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform.go > File golden/go/pdfxform/pdfxform.go (right): > > https://codereview.chromium.org/1216483002/diff/1/golden/go/pdfxform/pdfxform... > golden/go/pdfxform/pdfxform.go:226: } // data/45aa8af265d16839402583df5756a7c6.png > On 2015/06/26 15:09:23, jcgregorio wrote: > > How does this image get deployed to the server, and uploaded to the digests > > directory? > > > > My preference would be that pdfxform take a command-line flag that points to the > > directory where this image is located. The normal name for such a flag is > > resourcesDir. See how it is defined and used in Gold: > > > > > > https://github.com/google/skia-buildbot/blob/master/golden/go/skiacorrectness... > > That would make sense if I had a lot of binary resources. Instead, I suggest that we simply embed the 500 bytes into the source code. This way, we can never fail to upload the file. Then just make it a flag for a single file and have the server fail if cannot decode the PNG. We'll make that part of the deploy package and can even check an image into git, but it's much easier to be switched if necessary and also easier to understand IMO than bytes embedded in source.
https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:39: type md5digest [md5.Size]byte On 2015/06/26 17:25:06, stephana wrote: > I think treating MD5 hashes as byte slices (instead of hex strings) is overkill. > Using strings throughout for MD5s is fine IMO and it avoids having function like > decodeMd5(...). > This is a largely IO bound process, if CPU matters at all it's how fast the > rasterizers can convert a PDF to PNG. The pdfXformer.results map can grow without constraint as the process lives longer. But I'll do it your way until we notice problems. Done. https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:122: // parsed and prooduced. On 2015/06/26 17:25:06, stephana wrote: > Instead of the defining it's own structures to receive the results. > It should re-use the ones from the gold ingester (see > golden/go/goldingester/goldingester.go). > > That way any changes in the ingester will be reflected here and help detect > modifications that need > to be made. Done. Thanks. https://codereview.chromium.org/1216483002/diff/20001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:221: var errorImageMd5 = md5digest{ On 2015/06/26 17:25:06, stephana wrote: > Another argument to use strings instead of md5digest. Done.
https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:34: pngExt = "png" PNG_EXT https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:35: pdfExt = "pdf" PDF_EXT https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:113: config := auth.OAuthConfig(cacheFilePath, auth.SCOPE_FULL_CONTROL) Create two command line flags: --local --oauth_cache_file ala local = flag.Bool("local", false, "True if not running in prod.") oauthCacheFile = flag.String("oauth_cache_file", "oauth_cache.dat", "..description..") Then call: if client, err = auth.NewClient(!*local, *oauthCacheFile, auth.SCOPE_FULL_CONTROL, auth.SCOPE_GCE); err != nil { glog.Fatalf("Failed to create authenticated HTTP client: %s", err) } https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:225: xformer.identifier = fmt.Sprintf("%s.%s.%s.tmp.%d.", filepath.Base(os.Args[0]), host, userName, os.Getpid()) When this is run in prod the exe will sit in /usr/local/bin, so you can't use the exe directory to store data. Add a command line flag: --data_dir ala dataDir = flag.String("data_dir", "", "Directory to store data in.") Use *dataDir as the place to store the data, if *dataDir is not set then you can use ioutil.TempDir(). https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:468: flag.Parse() Use common.InitWithMetrics. https://github.com/google/skia-buildbot/blob/master/go/common/common.go#L38 Also needs a command-line flag added for the graphite server.
https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:184: defer removeIf(pngPath) We have a function for this already. defer util.Remove(pngPath) should do the trick and then you can get rid the removeIf function. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:406: continue wouldn't this repeat a failed request indefinitely ? You could have a counter here and retry for a fixed number of times and then leave the loop. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:447: // uploadErrorImage should be run once to verify that the image is there This is really not a good idea. The image should not be in here, but a command line flag. MUCH easier to maintain down the road. And then to a test decode upon startup. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:472: assertNil(os.MkdirAll(configDir, 0700)) Making sure that directory exists is already implemented in fileutil: See: https://github.com/google/skia-buildbot/blob/master/golden/go/filediffstore/f... https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:474: xformer := pdfXformer{ These should not be hard coded, but instead be flags (see skiacorrectness). The hard coded values can be the defaults for the flag. There should also be a flag whether it's run local or not, in the latter case the oauth info will come from the CGE instance's meta data. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:496: end := time.Now() This is not wrong, but the pattern that we use throughout is more like this: oneRun := func(start, end int64) { ... xformer.processTimeRange(start, end) ... } oneRun() for _ = range time.Tick(time.Minute) { oneRun() }
https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:447: // uploadErrorImage should be run once to verify that the image is there Upon further thought I agree, should be cmd line flag. On 2015/06/26 at 20:03:00, stephana wrote: > This is really not a good idea. The image should not be in here, but a command line flag. MUCH easier to maintain down the road. And then to a test decode upon startup.
https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:34: pngExt = "png" On 2015/06/26 19:21:05, jcgregorio wrote: > PNG_EXT Done. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:35: pdfExt = "pdf" On 2015/06/26 19:21:06, jcgregorio wrote: > PDF_EXT Done. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:184: defer removeIf(pngPath) On 2015/06/26 20:02:59, stephana wrote: > We have a function for this already. > > defer util.Remove(pngPath) > > should do the trick and then you can get rid the removeIf function. I modified the name of the function to make it clear how it differs. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:225: xformer.identifier = fmt.Sprintf("%s.%s.%s.tmp.%d.", filepath.Base(os.Args[0]), host, userName, os.Getpid()) On 2015/06/26 19:21:06, jcgregorio wrote: > When this is run in prod the exe will sit in /usr/local/bin, so you can't use > the exe directory to store data. > > Add a command line flag: > > --data_dir > > ala > > dataDir = flag.String("data_dir", "", "Directory to store data in.") > > Use *dataDir as the place to store the data, if *dataDir is not set then you can > use ioutil.TempDir(). Done. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:406: continue On 2015/06/26 20:02:59, stephana wrote: > wouldn't this repeat a failed request indefinitely ? > > You could have a counter here and retry for a fixed number of times and then > leave the loop. Done. Good catch. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:447: // uploadErrorImage should be run once to verify that the image is there On 2015/06/26 20:08:07, jcgregorio wrote: > Upon further thought I agree, should be cmd line flag. > > On 2015/06/26 at 20:03:00, stephana wrote: > > This is really not a good idea. The image should not be in here, but a command > line flag. MUCH easier to maintain down the road. And then to a test decode upon > startup. > Done. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:496: end := time.Now() On 2015/06/26 20:02:59, stephana wrote: > This is not wrong, but the pattern that we use throughout is more like this: > > > oneRun := func(start, end int64) { > ... > xformer.processTimeRange(start, end) > ... > } > > oneRun() > for _ = range time.Tick(time.Minute) { > oneRun() > } Done.
https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:113: config := auth.OAuthConfig(cacheFilePath, auth.SCOPE_FULL_CONTROL) On 2015/06/26 19:21:05, jcgregorio wrote: > Create two command line flags: > > --local > --oauth_cache_file > > ala > > local = flag.Bool("local", false, "True if not running in prod.") > oauthCacheFile = flag.String("oauth_cache_file", "oauth_cache.dat", > "..description..") > > > Then call: > > if client, err = auth.NewClient(!*local, *oauthCacheFile, > auth.SCOPE_FULL_CONTROL, auth.SCOPE_GCE); err != nil { > glog.Fatalf("Failed to create authenticated HTTP client: %s", err) > } Mostly done. https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... golden/go/pdfxform/main.go:472: assertNil(os.MkdirAll(configDir, 0700)) On 2015/06/26 20:03:00, stephana wrote: > Making sure that directory exists is already implemented in fileutil: > > See: > > https://github.com/google/skia-buildbot/blob/master/golden/go/filediffstore/f... Done.
message: On 2015/06/26 22:38:24, Hal Canary wrote: > https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main.go > File golden/go/pdfxform/main.go (right): > > https://codereview.chromium.org/1216483002/diff/60001/golden/go/pdfxform/main... > golden/go/pdfxform/main.go:113: config := auth.OAuthConfig(cacheFilePath, > auth.SCOPE_FULL_CONTROL) > > On 2015/06/26 19:21:05, jcgregorio wrote: > > Create two command line flags: > > > > --local > > --oauth_cache_file > > > > ala > > > > local = flag.Bool("local", false, "True if not running in prod.") > > oauthCacheFile = flag.String("oauth_cache_file", "oauth_cache.dat", > > "..description..") > > > > > > Then call: > > > > if client, err = auth.NewClient(!*local, *oauthCacheFile, > > auth.SCOPE_FULL_CONTROL, auth.SCOPE_GCE); err != nil { > > glog.Fatalf("Failed to create authenticated HTTP client: %s", err) > > } > > Mostly done. By mostly done, I mean that jcgregorio@'s suggestion did not work for me as is. I suggest we should land my modified code as is and let you fix auth.NewClient at a later date.
In addition to the error handling this should also add metrics that record the failures to InfluxDB (see ingestion code for example). That way we can define alerts for errors. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:89: // assertNil logs the err and exits if it is not nil. This is kind of a go antipattern. See https://golang.org/doc/faq#assertions Please deal with the errors where they occur, either log it or return it to the calling function. Our general approach has been to abort the program in main via glog.Fatal if an error occurred while setting up basic app infrastructure. If an error occurs in our main work loop, we usually log an error and take on the next task. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:125: assertNil(err) Should return error instead. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:179: graphiteServer = flag.String("graphite_server", "", "") Nit: Please add descriptions to the flags. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:243: assertNil(err) Should return error instead. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:460: // *storageBucket = "skia-infra-testdata" Should this be removed ?
https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:89: // assertNil logs the err and exits if it is not nil. On 2015/07/07 17:28:27, stephana wrote: > This is kind of a go antipattern. > See https://golang.org/doc/faq#assertions > > Please deal with the errors where they occur, either log it or return it to the > calling function. > > Our general approach has been to abort the program in main via glog.Fatal if an > error occurred while setting up basic app infrastructure. > If an error occurs in our main work loop, we usually log an error and take on > the next task. Done. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:125: assertNil(err) On 2015/07/07 17:28:27, stephana wrote: > Should return error instead. Done. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:179: graphiteServer = flag.String("graphite_server", "", "") On 2015/07/07 17:28:27, stephana wrote: > Nit: Please add descriptions to the flags. Done. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:243: assertNil(err) On 2015/07/07 17:28:27, stephana wrote: > Should return error instead. Done. https://codereview.chromium.org/1216483002/diff/100001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:460: // *storageBucket = "skia-infra-testdata" On 2015/07/07 17:28:27, stephana wrote: > Should this be removed ? Done.
https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:217: func (xformer *pdfXformer) makeTmpDir() string { This should return the error IMO. https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:251: func (xformer *pdfXformer) processResult(res goldingester.Result) []goldingester.Result { This should return []*goldingester.Result https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:269: if tempdir == "" { Test for an error here instead of an empty string and either return the error or log it here (rather than in makeTmpDir). https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:350: for _, rasterizedResult := range xformer.processResult(*res) { If you make the changes to processResult suggested above, you can then write this without a loop: rasterizedResults = append(rasterizedResults, rasterizedResult...)
https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:217: func (xformer *pdfXformer) makeTmpDir() string { On 2015/07/09 17:18:54, stephana wrote: > This should return the error IMO. Done. https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:251: func (xformer *pdfXformer) processResult(res goldingester.Result) []goldingester.Result { On 2015/07/09 17:18:54, stephana wrote: > This should return []*goldingester.Result Done. https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:269: if tempdir == "" { On 2015/07/09 17:18:54, stephana wrote: > Test for an error here instead of an empty string and either return the error or > log it here (rather than in makeTmpDir). Done. https://codereview.chromium.org/1216483002/diff/120001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:350: for _, rasterizedResult := range xformer.processResult(*res) { On 2015/07/09 17:18:54, stephana wrote: > If you make the changes to processResult suggested above, you > can then write this without a loop: > > rasterizedResults = append(rasterizedResults, rasterizedResult...) Done.
https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:70: var buffer [4]byte buffer := make([]byte, 4) ... f.Read(buffer).... https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:75: return bytes.Equal(magic[:], buffer[:]) return string(buffer) == "%PDF"
https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... File golden/go/pdfxform/main.go (right): https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:70: var buffer [4]byte On 2015/07/09 17:55:45, stephana wrote: > buffer := make([]byte, 4) > ... f.Read(buffer).... Done. https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... golden/go/pdfxform/main.go:75: return bytes.Equal(magic[:], buffer[:]) On 2015/07/09 17:55:45, stephana wrote: > return string(buffer) == "%PDF" Done.
On 2015/07/09 17:58:38, Hal Canary wrote: > https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... > File golden/go/pdfxform/main.go (right): > > https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... > golden/go/pdfxform/main.go:70: var buffer [4]byte > On 2015/07/09 17:55:45, stephana wrote: > > buffer := make([]byte, 4) > > ... f.Read(buffer).... > > Done. > > https://codereview.chromium.org/1216483002/diff/140001/golden/go/pdfxform/mai... > golden/go/pdfxform/main.go:75: return bytes.Equal(magic[:], buffer[:]) > On 2015/07/09 17:55:45, stephana wrote: > > return string(buffer) == "%PDF" > > Done. LGTM Before we can deploy this we should do tesruns and/or add some basic tests.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216483002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stephana@google.com Link to the patchset: https://codereview.chromium.org/1216483002/#ps180001 (title: "2015-07-09 (Thursday) 14:48:41 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216483002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stephana@google.com Link to the patchset: https://codereview.chromium.org/1216483002/#ps200001 (title: "2015-07-09 (Thursday) 15:12:53 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216483002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/buildbot/+/18a17c0a39d79df770d40bdcc528dce7bfee... |
