Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(219)

Issue 2343983004: Add PPDProvider barebones implementation and associated cache skeleton. (Closed)

Created:
4 years, 3 months ago by Carlson
Modified:
4 years, 1 month ago
Reviewers:
Lei Zhang, skau
CC:
chromium-reviews, oshima+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add initial PpdProvider implementation and associated PpdCache. Also add unit tests for both. BUG=617254 Committed: https://crrev.com/c7d4aa6cfd11244553fb6308434587149a8518ba Cr-Commit-Position: refs/heads/master@{#427353}

Patch Set 1 #

Patch Set 2 : Remove extra includes #

Total comments: 16

Patch Set 3 : Address skau comments #

Patch Set 4 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 5 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 25

Patch Set 6 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 7 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 21

Patch Set 8 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 38

Patch Set 9 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 10 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 11 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 12 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 61

Patch Set 13 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 14 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 15 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 16 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 18

Patch Set 17 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 18 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 26

Patch Set 19 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Total comments: 2

Patch Set 20 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Patch Set 21 : Initial PPDProvider/PPDCache implementation. Also, add associated unittests. This doesn't plumb th… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+896 lines, -2 lines) Patch
M chromeos/chromeos.gyp View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +57 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +144 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +150 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_provider.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +104 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +237 lines, -0 lines 0 comments Download
A chromeos/printing/ppd_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +186 lines, -0 lines 0 comments Download
M chromeos/printing/printer_configuration.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +12 lines, -0 lines 0 comments Download
M chromeos/printing/printer_translator_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 63 (15 generated)
skau
Looks generally reasonable. We should chat about PPDFile. I made a comment in ppd_cache.h. I'm ...
4 years, 3 months ago (2016-09-17 01:00:00 UTC) #3
Carlson
Took a while to circle back to this, but I think this is in better ...
4 years, 2 months ago (2016-10-06 15:56:05 UTC) #4
skau
Generally looks good. There's a few issues with convention and a couple things that are ...
4 years, 2 months ago (2016-10-07 16:29:07 UTC) #5
Carlson
Reworked this given the PpdReference changes. Many of the comments are now obsolete, but I ...
4 years, 2 months ago (2016-10-14 19:28:56 UTC) #6
skau
Please make it clear that you should create multiple PpdResolvers if you want to run ...
4 years, 2 months ago (2016-10-14 22:10:17 UTC) #7
Carlson
Addressed comments. If we do think this can be called concurrently, I'm going to also ...
4 years, 2 months ago (2016-10-14 23:05:56 UTC) #8
skau
PpdProvider doesn't need to be thread safe. I'd just like some warning of accidental usage. ...
4 years, 2 months ago (2016-10-14 23:51:49 UTC) #9
Carlson
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc#newcode30 chromeos/printing/ppd_cache.cc:30: char HexifyNibble(int nibble) { On 2016/10/14 23:51:49, skau wrote: ...
4 years, 2 months ago (2016-10-17 16:43:20 UTC) #10
skau
Adding thestig@ for committer's review https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc#newcode139 chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) ...
4 years, 2 months ago (2016-10-17 17:16:26 UTC) #12
Lei Zhang
I only glanced at the CL. https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc#newcode163 chromeos/printing/ppd_cache.cc:163: }; Add DISALLOW_COPY_AND_ASSIGN(...); ...
4 years, 2 months ago (2016-10-17 17:54:07 UTC) #13
Carlson
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.cc#newcode139 chromeos/printing/ppd_cache.cc:139: FilePath GetCachePathBase(const Printer::PpdReference& ref) const { On 2016/10/17 17:16:26, ...
4 years, 2 months ago (2016-10-18 19:05:02 UTC) #14
Lei Zhang
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h#newcode43 chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 19:05:01, Carlson wrote: > ...
4 years, 2 months ago (2016-10-18 19:37:01 UTC) #15
Carlson
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h#newcode43 chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 19:37:00, Lei Zhang wrote: ...
4 years, 2 months ago (2016-10-18 20:06:28 UTC) #16
Lei Zhang
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h#newcode43 chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 20:06:28, Carlson wrote: > ...
4 years, 2 months ago (2016-10-18 22:25:34 UTC) #17
Carlson
https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h File chromeos/printing/ppd_cache.h (right): https://codereview.chromium.org/2343983004/diff/140001/chromeos/printing/ppd_cache.h#newcode43 chromeos/printing/ppd_cache.h:43: virtual base::Optional<base::FilePath> Find( On 2016/10/18 22:25:34, Lei Zhang wrote: ...
4 years, 2 months ago (2016-10-18 23:54:31 UTC) #18
Lei Zhang
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_cache.cc#newcode9 chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" Not used? https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_cache.cc#newcode12 chromeos/printing/ppd_cache.cc:12: #include "chromeos/printing/ppd_cache.h" This ...
4 years, 2 months ago (2016-10-19 01:25:10 UTC) #20
Carlson
Mostly resolved, but need more info on a couple of things (particularly ppd_provider.cc:79 and ppd_provider.cc:151) ...
4 years, 2 months ago (2016-10-19 16:38:27 UTC) #21
Lei Zhang
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_cache.cc#newcode9 chromeos/printing/ppd_cache.cc:9: #include "base/path_service.h" On 2016/10/19 16:38:26, Carlson wrote: > On ...
4 years, 2 months ago (2016-10-19 23:15:46 UTC) #22
Carlson
I think I've addressed everything so far. If I missed something, apologies; it's a little ...
4 years, 2 months ago (2016-10-19 23:38:56 UTC) #23
Lei Zhang
https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_provider.cc File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_provider.cc#newcode149 chromeos/printing/ppd_provider.cc:149: auto parsed_json = ::base::JSONReader::Read(contents); On 2016/10/19 23:38:56, Carlson wrote: ...
4 years, 2 months ago (2016-10-19 23:51:55 UTC) #24
Carlson
On 2016/10/19 23:51:55, Lei Zhang wrote: > https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_provider.cc > File chromeos/printing/ppd_provider.cc (right): > > https://codereview.chromium.org/2343983004/diff/220001/chromeos/printing/ppd_provider.cc#newcode149 ...
4 years, 2 months ago (2016-10-20 17:39:29 UTC) #25
Lei Zhang
On 2016/10/20 17:39:29, Carlson wrote: > I would argue that the most relevant audience for ...
4 years, 2 months ago (2016-10-20 21:28:53 UTC) #26
skau
I'll take a look. I was waiting for the style issues to be arbitrated. :) ...
4 years, 2 months ago (2016-10-21 00:26:21 UTC) #27
Lei Zhang
On 2016/10/21 00:26:21, skau wrote: > I'll take a look. I was waiting for the ...
4 years, 2 months ago (2016-10-21 00:37:51 UTC) #28
skau
Just once concern regarding the whether we should post the callback from the PpdProvider back ...
4 years, 2 months ago (2016-10-21 16:12:57 UTC) #29
skau
Just once concern regarding the whether we should post the callback from the PpdProvider back ...
4 years, 2 months ago (2016-10-21 16:12:58 UTC) #30
skau
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll ...
4 years, 2 months ago (2016-10-21 16:15:36 UTC) #31
skau
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll ...
4 years, 2 months ago (2016-10-21 16:15:37 UTC) #32
skau
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll ...
4 years, 2 months ago (2016-10-21 16:15:41 UTC) #33
skau
On 2016/10/21 00:37:51, Lei Zhang wrote: > On 2016/10/21 00:26:21, skau wrote: > > I'll ...
4 years, 2 months ago (2016-10-21 16:15:42 UTC) #34
skau
not lgtm
4 years, 2 months ago (2016-10-21 17:08:10 UTC) #35
Carlson
https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_cache.cc#newcode70 chromeos/printing/ppd_cache.cc:70: // Try to clean up the file, as it ...
4 years, 1 month ago (2016-10-24 17:32:33 UTC) #36
skau
lgtm Thanks for putting up with all the comments. https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_provider.cc File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_provider.cc#newcode94 chromeos/printing/ppd_provider.cc:94: ...
4 years, 1 month ago (2016-10-24 17:55:33 UTC) #37
Carlson
https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_provider.cc File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2343983004/diff/300001/chromeos/printing/ppd_provider.cc#newcode94 chromeos/printing/ppd_provider.cc:94: url_context_getter_->GetNetworkTaskRunner()->PostTask( On 2016/10/24 17:55:33, skau wrote: > On 2016/10/24 ...
4 years, 1 month ago (2016-10-24 18:39:50 UTC) #38
Carlson
4 years, 1 month ago (2016-10-24 18:39:51 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2343983004/340001
4 years, 1 month ago (2016-10-24 20:18:33 UTC) #42
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
4 years, 1 month ago (2016-10-24 20:18:36 UTC) #44
Carlson
On 2016/10/24 20:18:36, commit-bot: I haz the power wrote: > No L-G-T-M from a valid ...
4 years, 1 month ago (2016-10-24 20:19:58 UTC) #45
Lei Zhang
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_cache_unittest.cc File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_cache_unittest.cc#newcode48 chromeos/printing/ppd_cache_unittest.cc:48: PpdCacheTest() { CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); } Many unit tests override SetUp() ...
4 years, 1 month ago (2016-10-24 22:26:25 UTC) #46
Carlson
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_cache_unittest.cc File chromeos/printing/ppd_cache_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_cache_unittest.cc#newcode48 chromeos/printing/ppd_cache_unittest.cc:48: PpdCacheTest() { CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir()); } On 2016/10/24 22:26:24, Lei Zhang ...
4 years, 1 month ago (2016-10-24 22:48:44 UTC) #47
Lei Zhang
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc#newcode112 chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 22:48:44, Carlson ...
4 years, 1 month ago (2016-10-24 23:33:44 UTC) #48
Carlson
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc#newcode112 chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/24 23:33:43, Lei ...
4 years, 1 month ago (2016-10-25 00:04:02 UTC) #49
Lei Zhang
- I really thinks we have spent way too much time arguing over some not ...
4 years, 1 month ago (2016-10-25 00:33:44 UTC) #50
Carlson
https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2343983004/diff/340001/chromeos/printing/ppd_provider_unittest.cc#newcode112 chromeos/printing/ppd_provider_unittest.cc:112: GURL expected_url(string("https://") + kTestQuirksServer // On 2016/10/25 00:33:44, Lei ...
4 years, 1 month ago (2016-10-25 01:59:32 UTC) #51
Lei Zhang
lgtm
4 years, 1 month ago (2016-10-25 02:04:25 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2343983004/400001
4 years, 1 month ago (2016-10-25 15:06:00 UTC) #59
commit-bot: I haz the power
Committed patchset #21 (id:400001)
4 years, 1 month ago (2016-10-25 15:12:37 UTC) #61
commit-bot: I haz the power
4 years, 1 month ago (2016-10-25 15:43:44 UTC) #63
Message was sent while issue was closed.
Patchset 21 (id:??) landed as
https://crrev.com/c7d4aa6cfd11244553fb6308434587149a8518ba
Cr-Commit-Position: refs/heads/master@{#427353}

Powered by Google App Engine
This is Rietveld 408576698