Refactor PrinterDiscoverer and PrinterDetector to use a common interface.

Refactor PrinterDiscoverer and PrinterDetector to use a common interface.

Take 2.  The previous version of this patch broke the debug build.  This
has been fixed in this version.

This change factors out a PrinterDetector interface and associated
Observer class.  The Cups USB printer detector now implements this
external interface, and the PrinterDiscoverer class is re-expressed
as a generic PrinterDetectorInterface implementation that combines the
results of subsystem-specific printer detectors.

This change also adds unit tests for the new CombiningPrinterDetector class
used to provide discovery functionality.

BUG=588234
Review-Url: https://codereview.chromium.org/2945303005
Cr-Commit-Position: refs/heads/master@{#481602}
Committed: https://chromium.googlesource.com/chromium/src/+/066f992488dd298b4af581f56283a2f7d2ddbe6f

[Carlson, 2017-06-22 00:17:51]

The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 00:18:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Carlson, 2017-06-22 00:25:33]

justincarlson@chromium.org changed reviewers:
+ skau@chromium.org, stevenjb@chromium.org, xdai@chromium.org

[Carlson, 2017-06-22 00:25:34]

Take 2.  Sorry for the previous break.  Verified that this version also builds
in Debug mode.

[stevenjb, 2017-06-22 00:28:53]

On 2017/06/22 00:25:34, Carlson wrote:
> Take 2.  Sorry for the previous break.  Verified that this version also builds
> in Debug mode.

Feel free to TBR this if the change is trivial, but if you want me to review it
could you upload the original patchset and then the fixed one so I can just look
at the delta? Or at least note exactly what changed? Thanks!

[Carlson, 2017-06-22 00:40:16]

On 2017/06/22 00:28:53, stevenjb wrote:
> On 2017/06/22 00:25:34, Carlson wrote:
> > Take 2.  Sorry for the previous break.  Verified that this version also
builds
> > in Debug mode.
> 
> Feel free to TBR this if the change is trivial, but if you want me to review
it
> could you upload the original patchset and then the fixed one so I can just
look
> at the delta? Or at least note exactly what changed? Thanks!

I'll do a patch delta if you want, but the entirety of the change is

s/{}/std::vector<std::string>()/

on line 163 of combining_printer_detector_unittest.cc

I'm still staring at the pre-fix compiler error in debug mode and I can't quite
figure out if the compiler is wrong or the code is.  I know it's, like, *never*
the compiler.  But I think it might be the compiler.  :)

-J

[skau, 2017-06-22 01:39:57]

On 2017/06/22 00:40:16, Carlson wrote:
> On 2017/06/22 00:28:53, stevenjb wrote:
> > On 2017/06/22 00:25:34, Carlson wrote:
> > > Take 2.  Sorry for the previous break.  Verified that this version also
> builds
> > > in Debug mode.
> > 
> > Feel free to TBR this if the change is trivial, but if you want me to review
> it
> > could you upload the original patchset and then the fixed one so I can just
> look
> > at the delta? Or at least note exactly what changed? Thanks!
> 
> I'll do a patch delta if you want, but the entirety of the change is
> 
> s/{}/std::vector<std::string>()/
> 
> on line 163 of combining_printer_detector_unittest.cc
> 
> I'm still staring at the pre-fix compiler error in debug mode and I can't
quite
> figure out if the compiler is wrong or the code is.  I know it's, like,
*never*
> the compiler.  But I think it might be the compiler.  :)
> 
> -J

lgtm

I'm a bit confused as to how it got though CQ since
linux_chromium_chromeos_rel_ng builds that test.

[commit-bot: I haz the power, 2017-06-22 02:45:52]

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

[commit-bot: I haz the power, 2017-06-22 02:45:53]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-06-22 16:47:24]

It only failed in the Debug build, which is much slower so we don't run have it
in the CQ.

Thanks for providing the diff, LKGM

[xdai1, 2017-06-22 16:49:06]

cups_printers_handler.h .cc lgtm

[Carlson, 2017-06-22 17:44:52]

On 2017/06/22 16:49:06, xdai1 wrote:
> cups_printers_handler.h .cc lgtm

[chromium-reviews, 2017-06-22 17:45:55]

Sorry, stevenjb@ did you mean lgtm?  I don't know what LKGM would mean in
this context.

On Thu, Jun 22, 2017 at 10:44 AM <justincarlson@chromium.org> wrote:

> On 2017/06/22 16:49:06, xdai1 wrote:
> > cups_printers_handler.h .cc lgtm
>
>
>
> https://codereview.chromium.org/2945303005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2017-06-22 17:55:42]

On 2017/06/22 17:45:55, chromium-reviews wrote:
> Sorry, stevenjb@ did you mean lgtm?  I don't know what LKGM would mean in
> this context.
> 
> On Thu, Jun 22, 2017 at 10:44 AM <mailto:justincarlson@chromium.org> wrote:
> 
> > On 2017/06/22 16:49:06, xdai1 wrote:
> > > cups_printers_handler.h .cc lgtm
> >
> >
> >
> > https://codereview.chromium.org/2945303005/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Lol, sorry, I've been working on replacing the Chrome LKGM ('Last Known good
manifest') builder, apparently I've retrained my fingers to type that :)

LGTM

[Carlson, 2017-06-22 17:58:27]

The CQ bit was checked by justincarlson@chromium.org

[commit-bot: I haz the power, 2017-06-22 17:58:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 18:04:10]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498154307034940, "parent_rev":
"f51fbcd2367c3d4d5f91ca8bf37b76eda60f4da0", "commit_rev":
"7305b63c233546b767c44babd25f057f87540c5a"}

[commit-bot: I haz the power, 2017-06-22 18:04:25]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1498154307034940, "parent_rev":
"09b71bd5e4c6411e6d7b6750313d55a670dce488", "commit_rev":
"066f992488dd298b4af581f56283a2f7d2ddbe6f"}

[commit-bot: I haz the power, 2017-06-22 18:04:39]

Description was changed from

==========
Refactor PrinterDiscoverer and PrinterDetector to use a common interface.

Take 2.  The previous version of this patch broke the debug build.  This
has been fixed in this version.

This change factors out a PrinterDetector interface and associated
Observer class.  The Cups USB printer detector now implements this
external interface, and the PrinterDiscoverer class is re-expressed
as a generic PrinterDetectorInterface implementation that combines the
results of subsystem-specific printer detectors.

This change also adds unit tests for the new CombiningPrinterDetector class
used to provide discovery functionality.

BUG=588234
==========

to

==========
Refactor PrinterDiscoverer and PrinterDetector to use a common interface.

Take 2.  The previous version of this patch broke the debug build.  This
has been fixed in this version.

This change factors out a PrinterDetector interface and associated
Observer class.  The Cups USB printer detector now implements this
external interface, and the PrinterDiscoverer class is re-expressed
as a generic PrinterDetectorInterface implementation that combines the
results of subsystem-specific printer detectors.

This change also adds unit tests for the new CombiningPrinterDetector class
used to provide discovery functionality.

BUG=588234

Review-Url: https://codereview.chromium.org/2945303005
Cr-Commit-Position: refs/heads/master@{#481602}
Committed:
https://chromium.googlesource.com/chromium/src/+/066f992488dd298b4af581f56283...
==========

[commit-bot: I haz the power, 2017-06-22 18:04:40]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/066f992488dd298b4af581f56283...