Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613683004:
Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the way, clean it up a l… (Closed)
DescriptionRewrite the PpdProvider/PpdCache to use the SCS backend.
Along the way, clean it up a lot based on lessons learned from my first chromium code.
Note this changes PpdReference to use just a single "effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's usually available from IPP and other sources, so seems to be the most sensible key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2613683004
Cr-Commit-Position: refs/heads/master@{#448100}
Committed: https://chromium.googlesource.com/chromium/src/+/2fc4d82bfb5950942d8ac1548fa8cb8c4222dad6
Patch Set 1 #
Total comments: 20
Patch Set 2 : Major API change to better fit usage from the UI, intergration with the rest of the codebase #Patch Set 3 : Plumb through rest of changes, address skau comments. #
Total comments: 28
Patch Set 4 : Address latest skau comments, rebase and make it work with recent sync changes #
Total comments: 2
Patch Set 5 : Add file:/// handling, address other comments. #Patch Set 6 : Remove ability to fetch network ppds for user supplied ppds. #Patch Set 7 : Fixup unit specifics_translation unittest #
Total comments: 3
Patch Set 8 : Convert proto to old and busted field deprecation syntax since new hotness is not available on andr… #Patch Set 9 : Fixup one more unit test #
Total comments: 14
Patch Set 10 : Address michealpg@ comments #Messages
Total messages: 61 (33 generated)
Generally looks good. Since the PPDProvider returns contents now, did you figure out how to get the PPD contents to CUPS? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:94: if (crypto::SHA256HashString(contents) == checksum) { Should we log a warning if this is false? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:140: LOG(ERROR) << "Failed to create ppd cache file"; Do we need to clean up the file if one of the writes failed? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:8: #include <functional> Do you need this header? Most of the included features are banned by the style guide. https://chromium-cpp.appspot.com/ https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:106: MaybeStartFetch(); Can you make this an early return? Unless your intention it's possible to do something after the fetch has been dispatched. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:121: } nit: add a new line https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:122: bool redo_from_start; Can you break instead of using this flag? It'll reduce the nesting and you expect to exit the loop after any fetch operation has started. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:352: auto code = ValidateAndParseJSONResponse(&contents); Is this the result code? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:457: // the necessary index data from the server. to the ppd server. Note we 'to the ppd server'? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.h:19: // chrome/browser/browser_process.h for GetApplicationLocale what is this for? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider_unittest.cc:175: // Should have an exact match for en-gb. this comment is incorrect
Description was changed from ========== Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the way, clean it up a lot based on lessons learned from my first chromium code. Note this changes PpdReference to use just a single "effective_model" field to resolve ppds from the ppd server. The added complexity of doing (manufacturer, model) tuples didn't seem worth it. If we ever do have a model name collision, we can disambiguate the model names on the server. Note this also doesn't (yet) fixup the callsites to PpdProvider -- I'm still working on that, but wanted to get the core out for review without waiting for that. BUG=617253 R=skau ========== to ========== Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the way, clean it up a lot based on lessons learned from my first chromium code. Note this changes PpdReference to use just a single "effective_model" field to resolve ppds from the ppd server. The added complexity of doing (manufacturer, model) tuples didn't seem worth it. If we ever do have a model name collision, we can disambiguate the model names on the server. Note this also doesn't (yet) fixup the callsites to PpdProvider -- I'm still working on that, but wanted to get the core out for review without waiting for that. BUG=617253 R=skau CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
I haven't re-tested this since some other changes went in, but will do that shortly. This should be good for another look. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:94: if (crypto::SHA256HashString(contents) == checksum) { On 2017/01/05 20:38:46, skau wrote: > Should we log a warning if this is false? Done. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:140: LOG(ERROR) << "Failed to create ppd cache file"; On 2017/01/05 20:38:46, skau wrote: > Do we need to clean up the file if one of the writes failed? Done. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:8: #include <functional> On 2017/01/05 20:38:46, skau wrote: > Do you need this header? Most of the included features are banned by the style > guide. > > https://chromium-cpp.appspot.com/ Not sure what this got put in for. Removed. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:106: MaybeStartFetch(); On 2017/01/05 20:38:46, skau wrote: > Can you make this an early return? Unless your intention it's possible to do > something after the fetch has been dispatched. Done. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:121: } On 2017/01/05 20:38:46, skau wrote: > nit: add a new line Done. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:122: bool redo_from_start; On 2017/01/05 20:38:46, skau wrote: > Can you break instead of using this flag? It'll reduce the nesting and you > expect to exit the loop after any fetch operation has started. Restructured a bit to reduce the loop complexity. I think this is better? https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:352: auto code = ValidateAndParseJSONResponse(&contents); On 2017/01/05 20:38:46, skau wrote: > Is this the result code? Removed some autos to make this clearer. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:457: // the necessary index data from the server. to the ppd server. Note we On 2017/01/05 20:38:46, skau wrote: > 'to the ppd server'? Sentence fragment. Good device. More later. (removed). https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.h:19: // chrome/browser/browser_process.h for GetApplicationLocale On 2017/01/05 20:38:46, skau wrote: > what is this for? It was a note to myself that I forgot to cleanup. Removed. https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider_unittest.cc (right): https://codereview.chromium.org/2613683004/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider_unittest.cc:175: // Should have an exact match for en-gb. On 2017/01/05 20:38:46, skau wrote: > this comment is incorrect Done.
Looks like you have some stale documentation and a few places that need updating. https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:216: } else if (!printer_manufacturer.empty() && !printer_model.empty()) { Didn't we remove the effective_manufacturer field? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:31: constexpr int kMaxCacheItemSize = 100 * 1024; This is 100KB right? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:45: if (!GURL(reference.user_supplied_ppd_url).is_valid()) { I'm not sure we can depend on this being a well formed URL. GURL accepts a limited set of schemes as valid. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:450: // If we kicked of a resolution, the entry better have already been s/of/off/ https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:487: // server.. involuntary ellipsis? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:61: size_t max_ppd_contents_size_ = 100 * 1024; Is this parameter still used by the cache? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:103: // |cb| will be called on the invoking thread, and will be sequenced. If I don't see any cancellation behavior in the impl. Is this comment still accurate? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... chromeos/printing/printer_configuration.h:31: std::string user_supplied_ppd_url; Note: Technically, this is a uri rather than a url since it's usual the path to a file. e.g. file:///home/user/...../PrinterPrinter.ppd.gz https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... chromeos/printing/printer_translator.cc:24: const char kEffectiveMakeAndModel[] = "effective_make_and_model"; Leave the constants alone for now. The Policy proto will take some time to change and the local keys are going to get removed. https://codereview.chromium.org/2613683004/diff/40001/components/sync/protoco... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/40001/components/sync/protoco... components/sync/protocol/printer_specifics.proto:21: // FIELD 2 RETIRED If it compiles (it was supposed to be fixed) can we use reserved 2,3 here?
https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:61: <paper-icon-button suffix id="searchIcon" icon="cr:search" on-tap="onTap_" hidden> Can you put this file in a separate CL? These changes aren't really related and it'll be easier to get it approved.
https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:61: <paper-icon-button suffix id="searchIcon" icon="cr:search" on-tap="onTap_" hidden> On 2017/01/27 01:38:09, skau wrote: > Can you put this file in a separate CL? These changes aren't really related and > it'll be easier to get it approved. Had already done this, just hadn't synced, sorry. https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:216: } else if (!printer_manufacturer.empty() && !printer_model.empty()) { On 2017/01/27 01:37:00, skau wrote: > Didn't we remove the effective_manufacturer field? This is in the dialog, not the ppd reference; we still present to the user as select a make, then select a model, and we use those pieces of information together to derive the effective_make_and_model string. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:31: constexpr int kMaxCacheItemSize = 100 * 1024; On 2017/01/27 01:37:00, skau wrote: > This is 100KB right? Yeah, upped it to 250k to match other definitions. It would be nice to have a single definition in the system of "how big a ppd file is reasonable". Any suggestions for a good place for that to live? https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:45: if (!GURL(reference.user_supplied_ppd_url).is_valid()) { On 2017/01/27 01:37:01, skau wrote: > I'm not sure we can depend on this being a well formed URL. GURL accepts a > limited set of schemes as valid. I don't think I understand your concern here. Mostly I'm trying to prevent URLFetcher from barfing when doing things behind the scenes. I *think* everything we'll ever want to use to lookup a ppd will be well-formed from GURL's POV; if not, then you're right, this needs rethinking. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:450: // If we kicked of a resolution, the entry better have already been On 2017/01/27 01:37:01, skau wrote: > s/of/off/ Done. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:487: // server.. On 2017/01/27 01:37:00, skau wrote: > involuntary ellipsis? Involuntary sideways colon. (Fixed) https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:61: size_t max_ppd_contents_size_ = 100 * 1024; On 2017/01/27 01:37:01, skau wrote: > Is this parameter still used by the cache? This is actually used in the provider to reject things before we ever get to the cache. See earlier comment about "what's a good single place to define this". We *could* just define it here and remove the limit in the cache with the understanding that we would never try to cache something too large there. Really not sure what the right answer is here. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:103: // |cb| will be called on the invoking thread, and will be sequenced. If On 2017/01/27 01:37:01, skau wrote: > I don't see any cancellation behavior in the impl. Is this comment still > accurate? You're right, it was stale. We new queue ALL THE THINGS. Fixed up several related comments. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... chromeos/printing/printer_configuration.h:31: std::string user_supplied_ppd_url; On 2017/01/27 01:37:01, skau wrote: > Note: Technically, this is a uri rather than a url since it's usual the path to > a file. e.g. file:///home/user/...../PrinterPrinter.ppd.gz I've always been a little hazy on the distinction, so I went and did some digging. https://www.ietf.org/rfc/rfc1738.txt section 3.10 seems pretty definitive that file:// does indeed fall into the scope of URL's, not just URIs. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/print... chromeos/printing/printer_translator.cc:24: const char kEffectiveMakeAndModel[] = "effective_make_and_model"; On 2017/01/27 01:37:01, skau wrote: > Leave the constants alone for now. The Policy proto will take some time to > change and the local keys are going to get removed. Not sure how that migration will work, but ok. https://codereview.chromium.org/2613683004/diff/40001/components/sync/protoco... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/40001/components/sync/protoco... components/sync/protocol/printer_specifics.proto:21: // FIELD 2 RETIRED On 2017/01/27 01:37:02, skau wrote: > If it compiles (it was supposed to be fixed) can we use reserved 2,3 here? Done.
Looks good. I'll take another look after we fix up the call sites if we hit any issues. Is it possible to somehow get this CL to compile and pass CQ without including the call site changes? It's a bit large now and it'll be quite unwieldly after. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:31: constexpr int kMaxCacheItemSize = 100 * 1024; On 2017/01/27 18:48:43, Carlson wrote: > On 2017/01/27 01:37:00, skau wrote: > > This is 100KB right? > > Yeah, upped it to 250k to match other definitions. > > It would be nice to have a single definition in the system of "how big a ppd > file is reasonable". Any suggestions for a good place for that to live? chromeos/printing/printing_constants.h? chromeos::printing::kMaxCacheItemSize? I think everything that cares should already depend on the chromeos/printing package. Also, add a note in the comment that this is Bytes and not some other unit in case we forget. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:45: if (!GURL(reference.user_supplied_ppd_url).is_valid()) { On 2017/01/27 18:48:43, Carlson wrote: > On 2017/01/27 01:37:01, skau wrote: > > I'm not sure we can depend on this being a well formed URL. GURL accepts a > > limited set of schemes as valid. > > I don't think I understand your concern here. Mostly I'm trying to prevent > URLFetcher from barfing when doing things behind the scenes. I *think* > everything we'll ever want to use to lookup a ppd will be well-formed from > GURL's POV; if not, then you're right, this needs rethinking. I stand corrected. I didn't realize file:// was in the RFC. I don't believe that the urls we save here will be fetchable by URLFetcher since they're expected to be a local file. If it works then it's fine. We'll find out when the call sites are updated. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:61: size_t max_ppd_contents_size_ = 100 * 1024; On 2017/01/27 18:48:43, Carlson wrote: > On 2017/01/27 01:37:01, skau wrote: > > Is this parameter still used by the cache? > > This is actually used in the provider to reject things before we ever get to the > cache. See earlier comment about "what's a good single place to define this". > > We *could* just define it here and remove the limit in the cache with the > understanding that we would never try to cache something too large there. > > Really not sure what the right answer is here. Doesn't the PpdProviderImpl construct the cache? In that case, we should just set it during construction of the cache. For the location, I would pull it out to a common location since we'll probably need the value in the UI as well when we report the reason the PPD was rejected. https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... components/sync/protocol/printer_specifics.proto:22: reserved 2 to 4; You can use 4. We never committed a CL that reserves 4. https://cs.chromium.org/chromium/src/components/sync/protocol/printer_specifi...
Description was changed from
==========
Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the
way, clean it up a lot based on lessons learned from my first chromium code.
Note this changes PpdReference to use just a single "effective_model"
field to resolve ppds from the ppd server. The added complexity of
doing (manufacturer, model) tuples didn't seem worth it. If we ever
do have a model name collision, we can disambiguate the model names on
the server.
Note this also doesn't (yet) fixup the callsites to PpdProvider -- I'm still
working on that, but wanted to get the core out for review without waiting for
that.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the
way, clean it up a lot based on lessons learned from my first chromium code.
Note this changes PpdReference to use just a single
"effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's
usually available from IPP and other sources, so seems to be the most sensible
key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider
changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Sorry, I think I wasn't clear earlier -- I edited my git commit log, but didn't realize it doesn't seem to propagate here, so I've edited the description now. This revision has all the callsites updated. It complies and passes all the unit tests (at least, all the unit tests I know will be affected...). On 2017/01/27 19:16:10, skau wrote: > Looks good. I'll take another look after we fix up the call sites if we hit any > issues. > > Is it possible to somehow get this CL to compile and pass CQ without including > the call site changes? It's a bit large now and it'll be quite unwieldly after. > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... > File chromeos/printing/ppd_cache.cc (right): > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... > chromeos/printing/ppd_cache.cc:31: constexpr int kMaxCacheItemSize = 100 * 1024; > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:00, skau wrote: > > > This is 100KB right? > > > > Yeah, upped it to 250k to match other definitions. > > > > It would be nice to have a single definition in the system of "how big a ppd > > file is reasonable". Any suggestions for a good place for that to live? > > chromeos/printing/printing_constants.h? chromeos::printing::kMaxCacheItemSize? > I think everything that cares should already depend on the chromeos/printing > package. > > Also, add a note in the comment that this is Bytes and not some other unit in > case we forget. > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... > File chromeos/printing/ppd_provider.cc (right): > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... > chromeos/printing/ppd_provider.cc:45: if > (!GURL(reference.user_supplied_ppd_url).is_valid()) { > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:01, skau wrote: > > > I'm not sure we can depend on this being a well formed URL. GURL accepts a > > > limited set of schemes as valid. > > > > I don't think I understand your concern here. Mostly I'm trying to prevent > > URLFetcher from barfing when doing things behind the scenes. I *think* > > everything we'll ever want to use to lookup a ppd will be well-formed from > > GURL's POV; if not, then you're right, this needs rethinking. > > I stand corrected. I didn't realize file:// was in the RFC. I don't believe > that the urls we save here will be fetchable by URLFetcher since they're > expected to be a local file. If it works then it's fine. We'll find out when > the call sites are updated. > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... > File chromeos/printing/ppd_provider.h (right): > > https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... > chromeos/printing/ppd_provider.h:61: size_t max_ppd_contents_size_ = 100 * 1024; > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:01, skau wrote: > > > Is this parameter still used by the cache? > > > > This is actually used in the provider to reject things before we ever get to > the > > cache. See earlier comment about "what's a good single place to define this". > > > > > We *could* just define it here and remove the limit in the cache with the > > understanding that we would never try to cache something too large there. > > > > Really not sure what the right answer is here. > > Doesn't the PpdProviderImpl construct the cache? In that case, we should just > set it during construction of the cache. For the location, I would pull it out > to a common location since we'll probably need the value in the UI as well when > we report the reason the PPD was rejected. > > https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... > File components/sync/protocol/printer_specifics.proto (right): > > https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... > components/sync/protocol/printer_specifics.proto:22: reserved 2 to 4; > You can use 4. We never committed a CL that reserves 4. > > https://cs.chromium.org/chromium/src/components/sync/protocol/printer_specifi...
https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_c... chromeos/printing/ppd_cache.cc:31: constexpr int kMaxCacheItemSize = 100 * 1024; On 2017/01/27 19:16:10, skau wrote: > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:00, skau wrote: > > > This is 100KB right? > > > > Yeah, upped it to 250k to match other definitions. > > > > It would be nice to have a single definition in the system of "how big a ppd > > file is reasonable". Any suggestions for a good place for that to live? > > chromeos/printing/printing_constants.h? chromeos::printing::kMaxCacheItemSize? > I think everything that cares should already depend on the chromeos/printing > package. > > Also, add a note in the comment that this is Bytes and not some other unit in > case we forget. Moved to a constant, added Bytes to the name. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:45: if (!GURL(reference.user_supplied_ppd_url).is_valid()) { On 2017/01/27 19:16:10, skau wrote: > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:01, skau wrote: > > > I'm not sure we can depend on this being a well formed URL. GURL accepts a > > > limited set of schemes as valid. > > > > I don't think I understand your concern here. Mostly I'm trying to prevent > > URLFetcher from barfing when doing things behind the scenes. I *think* > > everything we'll ever want to use to lookup a ppd will be well-formed from > > GURL's POV; if not, then you're right, this needs rethinking. > > I stand corrected. I didn't realize file:// was in the RFC. I don't believe > that the urls we save here will be fetchable by URLFetcher since they're > expected to be a local file. If it works then it's fine. We'll find out when > the call sites are updated. Yeah, this was a big problem, thanks for pointing it out. I've now added some stuff to provider to be able to handle file:/// urls and added a unit test for that functionality. https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2613683004/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:61: size_t max_ppd_contents_size_ = 100 * 1024; On 2017/01/27 19:16:10, skau wrote: > On 2017/01/27 18:48:43, Carlson wrote: > > On 2017/01/27 01:37:01, skau wrote: > > > Is this parameter still used by the cache? > > > > This is actually used in the provider to reject things before we ever get to > the > > cache. See earlier comment about "what's a good single place to define this". > > > > > We *could* just define it here and remove the limit in the cache with the > > understanding that we would never try to cache something too large there. > > > > Really not sure what the right answer is here. > > Doesn't the PpdProviderImpl construct the cache? In that case, we should just > set it during construction of the cache. For the location, I would pull it out > to a common location since we'll probably need the value in the UI as well when > we report the reason the PPD was rejected. No, it's passed in at construction time because dependency injection. Anyways, this is obsolete, I think, with the addition of the common constant. By location you mean the server root? Ideally we should have a way to report the *full* url that failed, not just the base of it. I suspect the right way to do that is with some sort of accessor, but until we have more structure around error reporting, I think it's not clear. https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/60001/components/sync/protoco... components/sync/protocol/printer_specifics.proto:22: reserved 2 to 4; On 2017/01/27 19:16:10, skau wrote: > You can use 4. We never committed a CL that reserves 4. > > https://cs.chromium.org/chromium/src/components/sync/protocol/printer_specifi... Indeed, that was used in a previous revision of *this* cl. :) done.
lgtm
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/30 19:13:50, skau wrote: > lgtm I had to fix up a unittest I missed (was only running the chromeos ones, not the chrome ones before) so I think I need you to re-sign-off on this? I'm not entire clear on what the rules are around that in chromium.
On 2017/02/01 23:25:10, Carlson wrote: > On 2017/01/30 19:13:50, skau wrote: > > lgtm > > I had to fix up a unittest I missed (was only running the chromeos ones, not the > chrome ones before) so I think I need you to re-sign-off on this? I'm not > entire clear on what the rules are around that in chromium. lgtm sticks unless it's explicitly removed (like Google internal). This behavior is different from Gerrit which needs to be carried forward. You do need approval from a components/sync owner. I'll add skym@ who I've worked with before.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
skau@chromium.org changed reviewers: + skym@chromium.org
skym@: Can you take a look to approve the components/sync changes?
https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... components/sync/protocol/printer_specifics.proto:22: reserved 2 to 3; Sorry, it looks like they didn't update the proto compiler >:( You'll want to change this back to what you had before. This is what is breaking the android tests.
https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... components/sync/protocol/printer_specifics.proto:22: reserved 2 to 3; On 2017/02/01 23:45:14, skau wrote: > Sorry, it looks like they didn't update the proto compiler >:( You'll want to > change this back to what you had before. This is what is breaking the android > tests. Can use the option [deprecated = true] to make intention more clear as well. This is currently our general best practice for sync protos.
https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... File components/sync/protocol/printer_specifics.proto (right): https://codereview.chromium.org/2613683004/diff/120001/components/sync/protoc... components/sync/protocol/printer_specifics.proto:22: reserved 2 to 3; On 2017/02/02 00:02:28, skym wrote: > On 2017/02/01 23:45:14, skau wrote: > > Sorry, it looks like they didn't update the proto compiler >:( You'll want to > > change this back to what you had before. This is what is breaking the android > > tests. > > Can use the option [deprecated = true] to make intention more clear as well. > This is currently our general best practice for sync protos. Done.
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2613683004/#ps160001 (title: "Fixup one more unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
justincarlson@chromium.org changed reviewers: + dpapad@chromium.org
dpapad@ can I ask you to look over the changes under chrome/browser/ui/webui? Thanks, -J
On 2017/02/02 at 19:59:43, justincarlson wrote: > dpapad@ can I ask you to look over the changes under chrome/browser/ui/webui? > > Thanks, > -J I am not familiar enough with the code being changed. Can you forward to the following reviewers instead? chrome/browser/ui/webui/print_preview: vitalybuka chrome/browser/ui/webui/settings/chromeos: michaelpg Thanks.
justincarlson@chromium.org changed reviewers: + michaelpg@chromium.org, vitalybuka@chromium.org - dpapad@chromium.org
Per dpapad's suggestion: vitalybuka@ can you look over chrome/browser/ui/webui/print_preview/... michaelpg@ can you look over chrome/browser/ui/webui/settings/chromeos/... Thanks, -J
chrome/browser/ui/webui/print_preview/ LGTM
Please update the description first line and subject to be shorter (so it fits on a single line in git commit logs). e.g., "Rewrite PpdProvider/PpdCache to use SCS backend" https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:205: printer_protocol + "://" + printer_address + "/" + printer_queue; optionally, while you're here, building this as a GURL instead of constructing it by hand would be cool https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:215: "file:///" + printer_ppd_path; Can you use a util function for this, e.g. net::FilePathToFileURL? https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:220: LOG(ERROR) << "Failed to get ppd reference!"; nit: the exclamation point is a bit dramatic ;-) https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:242: const Printer::PpdReference& ppd_reference = printer->ppd_reference(); is the Printer at |printer| guaranteed to outlive this reference? (even if so, that's not obvious. I don't see the need to avoid a copy here.) https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:329: if (result_code == chromeos::printing::PpdProvider::SUCCESS) { nit: remove {} for single-line ifs https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:344: if (result_code == chromeos::printing::PpdProvider::SUCCESS) { ditto
Description was changed from
==========
Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the
way, clean it up a lot based on lessons learned from my first chromium code.
Note this changes PpdReference to use just a single
"effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's
usually available from IPP and other sources, so seems to be the most sensible
key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider
changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
Rewrite the PpdProvider/PpdCache to use the SCS backend.
Along the way, clean it up a lot based on lessons learned from my first chromium
code.
Note this changes PpdReference to use just a single
"effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's
usually available from IPP and other sources, so seems to be the most sensible
key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider
changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
michaelpg@ Thanks for the review, PTAL? https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:205: printer_protocol + "://" + printer_address + "/" + printer_queue; On 2017/02/03 01:57:00, michaelpg wrote: > optionally, while you're here, building this as a GURL instead of constructing > it by hand would be cool I like this idea, but I'm not sure I understand what you mean here, since GURL doesn't appear to have builder functionality, only parsing. Or do you mean just passing this through GURL parsing and checking is_valid() before continuing? (Or am I missing some URLBuilder functionality somewhere?) https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:215: "file:///" + printer_ppd_path; On 2017/02/03 01:57:00, michaelpg wrote: > Can you use a util function for this, e.g. net::FilePathToFileURL? Done. https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:220: LOG(ERROR) << "Failed to get ppd reference!"; On 2017/02/03 01:57:00, michaelpg wrote: > nit: the exclamation point is a bit dramatic ;-) Done. https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:242: const Printer::PpdReference& ppd_reference = printer->ppd_reference(); On 2017/02/03 01:57:00, michaelpg wrote: > is the Printer at |printer| guaranteed to outlive this reference? (even if so, > that's not obvious. I don't see the need to avoid a copy here.) It should be safe, but sure, done. The conventions around Passed and Bind in the Chromium code are all a mess IMHO, and you're right, making a copy makes this much more obviously safe. https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:329: if (result_code == chromeos::printing::PpdProvider::SUCCESS) { On 2017/02/03 01:57:00, michaelpg wrote: > nit: remove {} for single-line ifs As I read style guide, this is left to the author, and I'm in the "always do braces because someday you will add something to the if and forget to add braces and be confused" camp. https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:344: if (result_code == chromeos::printing::PpdProvider::SUCCESS) { On 2017/02/03 01:57:00, michaelpg wrote: > ditto Acknowledged.
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:205: printer_protocol + "://" + printer_address + "/" + printer_queue; On 2017/02/03 19:32:19, Carlson wrote: > On 2017/02/03 01:57:00, michaelpg wrote: > > optionally, while you're here, building this as a GURL instead of constructing > > it by hand would be cool > > I like this idea, but I'm not sure I understand what you mean here, since GURL > doesn't appear to have builder functionality, only parsing. Or do you mean just > passing this through GURL parsing and checking is_valid() before continuing? > > (Or am I missing some URLBuilder functionality somewhere?) Huh, I thought we had that. Nevermind. https://codereview.chromium.org/2613683004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:329: if (result_code == chromeos::printing::PpdProvider::SUCCESS) { On 2017/02/03 19:32:19, Carlson wrote: > On 2017/02/03 01:57:00, michaelpg wrote: > > nit: remove {} for single-line ifs > > As I read style guide, this is left to the author, and I'm in the "always do > braces because someday you will add something to the if and forget to add braces > and be confused" camp. whoops, I was thinking of the Chromium JS style guide.
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org, skau@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/2613683004/#ps180001 (title: "Address michealpg@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486159579916550,
"parent_rev": "827d43cc4695df6291f3f30dade899aaf93665fd", "commit_rev":
"2fc4d82bfb5950942d8ac1548fa8cb8c4222dad6"}
Message was sent while issue was closed.
Description was changed from
==========
Rewrite the PpdProvider/PpdCache to use the SCS backend.
Along the way, clean it up a lot based on lessons learned from my first chromium
code.
Note this changes PpdReference to use just a single
"effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's
usually available from IPP and other sources, so seems to be the most sensible
key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider
changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
Rewrite the PpdProvider/PpdCache to use the SCS backend.
Along the way, clean it up a lot based on lessons learned from my first chromium
code.
Note this changes PpdReference to use just a single
"effective_make_and_model"
field to resolve ppds from the ppd server. This closely mirrors what's
usually available from IPP and other sources, so seems to be the most sensible
key for doing ppd lookups.
This also updates all callsites and cascading changes to make the provider
changes work.
BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2613683004
Cr-Commit-Position: refs/heads/master@{#448100}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fc4d82bfb5950942d8ac1548fa8...
==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2fc4d82bfb5950942d8ac1548fa8... |
