From faf208b1a9e95b9f10c74246df9449dcc56fef75 Mon Sep 17 00:00:00 2001 From: thakis Date: Fri, 29 May 2015 19:35:11 -0700 Subject: [PATCH] Revert of Include USB printers in printer list as "provisional" devices. (patchset #5 id:80001 of https://codereview.chromium.org/1153173002/) Reason for revert: Looks like this broke at least one test on the official builders (http://crbug.com/493584) Original issue's description: > Include USB printers in printer list as "provisional" devices. > > This patch starts including a list of connected printers supported by > printerProvider apps in the normal list of extension printers returned > to the print preview WebUI. These printers are given "provisional" IDs > because the devices are not yet known to the apps themselves. > > BUG=468955 > > Committed: https://crrev.com/6c3c36136b4567f2225550f15e27d2720e1930d8 > Cr-Commit-Position: refs/heads/master@{#331886} TBR=vitalybuka@chromium.org,tbarzic@chromium.org,rockot@chromium.org,reillyg@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=468955 Review URL: https://codereview.chromium.org/1163743002 Cr-Commit-Position: refs/heads/master@{#332104} --- .../print_preview/extension_printer_handler.cc | 105 +------------- .../print_preview/extension_printer_handler.h | 11 +- .../extension_printer_handler_unittest.cc | 156 ++------------------- .../webui/print_preview/print_preview_handler.cc | 2 +- .../printer_provider/usb_printer_manifest_data.cc | 10 +- .../printer_provider/usb_printer_manifest_data.h | 3 - 6 files changed, 16 insertions(+), 271 deletions(-) diff --git a/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc b/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc index 3021dcf223cb..3236e3ecda46 100644 --- a/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc +++ b/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc @@ -18,29 +18,12 @@ #include "chrome/browser/local_discovery/pwg_raster_converter.h" #include "components/cloud_devices/common/cloud_device_description.h" #include "components/cloud_devices/common/printer_description.h" -#include "device/core/device_client.h" -#include "device/usb/usb_device.h" -#include "device/usb/usb_service.h" -#include "extensions/browser/api/device_permissions_manager.h" #include "extensions/browser/api/printer_provider/printer_provider_api.h" #include "extensions/browser/api/printer_provider/printer_provider_api_factory.h" #include "extensions/browser/api/printer_provider/printer_provider_print_job.h" -#include "extensions/browser/extension_registry.h" -#include "extensions/common/api/printer_provider/usb_printer_manifest_data.h" -#include "extensions/common/permissions/permissions_data.h" -#include "extensions/common/permissions/usb_device_permission.h" -#include "extensions/common/permissions/usb_device_permission_data.h" -#include "extensions/common/value_builder.h" #include "printing/pdf_render_settings.h" #include "printing/pwg_raster_settings.h" -using device::UsbDevice; -using extensions::DevicePermissionsManager; -using extensions::DictionaryBuilder; -using extensions::Extension; -using extensions::ExtensionRegistry; -using extensions::ListBuilder; -using extensions::UsbPrinterManifestData; using local_discovery::PWGRasterConverter; namespace { @@ -83,14 +66,6 @@ void UpdateJobFileInfo( callback); } -bool HasUsbPrinterProviderPermissions(const Extension* extension) { - return extension->permissions_data() && - extension->permissions_data()->HasAPIPermission( - extensions::APIPermission::kPrinterProvider) && - extension->permissions_data()->HasAPIPermission( - extensions::APIPermission::kUsb); -} - } // namespace ExtensionPrinterHandler::ExtensionPrinterHandler( @@ -107,34 +82,11 @@ ExtensionPrinterHandler::~ExtensionPrinterHandler() { void ExtensionPrinterHandler::Reset() { // TODO(tbarzic): Keep track of pending request ids issued by |this| and // cancel them from here. - pending_enumeration_count_ = 0; weak_ptr_factory_.InvalidateWeakPtrs(); } void ExtensionPrinterHandler::StartGetPrinters( const PrinterHandler::GetPrintersCallback& callback) { - // Assume that there can only be one printer enumeration occuring at once. - DCHECK_EQ(pending_enumeration_count_, 0); - pending_enumeration_count_ = 1; - - bool extension_supports_usb_printers = false; - ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_); - for (const auto& extension : registry->enabled_extensions()) { - if (UsbPrinterManifestData::Get(extension.get()) && - HasUsbPrinterProviderPermissions(extension.get())) { - extension_supports_usb_printers = true; - break; - } - } - - if (extension_supports_usb_printers) { - device::UsbService* service = device::DeviceClient::Get()->GetUsbService(); - pending_enumeration_count_++; - service->GetDevices( - base::Bind(&ExtensionPrinterHandler::OnUsbDevicesEnumerated, - weak_ptr_factory_.GetWeakPtr(), callback)); - } - extensions::PrinterProviderAPIFactory::GetInstance() ->GetForBrowserContext(browser_context_) ->DispatchGetPrintersRequested( @@ -240,11 +192,7 @@ void ExtensionPrinterHandler::WrapGetPrintersCallback( const PrinterHandler::GetPrintersCallback& callback, const base::ListValue& printers, bool done) { - DCHECK_GT(pending_enumeration_count_, 0); - if (done) - pending_enumeration_count_--; - - callback.Run(printers, pending_enumeration_count_ == 0); + callback.Run(printers, done); } void ExtensionPrinterHandler::WrapGetCapabilityCallback( @@ -260,54 +208,3 @@ void ExtensionPrinterHandler::WrapPrintCallback( const std::string& status) { callback.Run(success, status); } - -void ExtensionPrinterHandler::OnUsbDevicesEnumerated( - const PrinterHandler::GetPrintersCallback& callback, - const std::vector>& devices) { - ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_); - DevicePermissionsManager* permissions_manager = - DevicePermissionsManager::Get(browser_context_); - - ListBuilder printer_list; - - for (const auto& extension : registry->enabled_extensions()) { - const UsbPrinterManifestData* manifest_data = - UsbPrinterManifestData::Get(extension.get()); - if (!manifest_data || !HasUsbPrinterProviderPermissions(extension.get())) - continue; - - const extensions::DevicePermissions* device_permissions = - permissions_manager->GetForExtension(extension->id()); - for (const auto& device : devices) { - if (manifest_data->SupportsDevice(device)) { - extensions::UsbDevicePermission::CheckParam param( - device->vendor_id(), device->product_id(), - extensions::UsbDevicePermissionData::UNSPECIFIED_INTERFACE); - if (device_permissions->FindUsbDeviceEntry(device) || - extension->permissions_data()->CheckAPIPermissionWithParam( - extensions::APIPermission::kUsbDevice, ¶m)) { - // Skip devices the extension already has permission to access. - continue; - } - - printer_list.Append( - DictionaryBuilder() - .Set("id", base::StringPrintf("provisional-usb:%s:%u", - extension->id().c_str(), - device->unique_id())) - .Set("name", - DevicePermissionsManager::GetPermissionMessage( - device->vendor_id(), device->product_id(), - device->manufacturer_string(), - device->product_string(), base::string16(), false)) - .Set("extensionId", extension->id()) - .Set("extensionName", extension->name()) - .Set("provisional", true)); - } - } - } - - DCHECK_GT(pending_enumeration_count_, 0); - pending_enumeration_count_--; - callback.Run(*printer_list.Build().get(), pending_enumeration_count_ == 0); -} diff --git a/chrome/browser/ui/webui/print_preview/extension_printer_handler.h b/chrome/browser/ui/webui/print_preview/extension_printer_handler.h index 5f3d1bb2efa8..9995184afd6a 100644 --- a/chrome/browser/ui/webui/print_preview/extension_printer_handler.h +++ b/chrome/browser/ui/webui/print_preview/extension_printer_handler.h @@ -6,7 +6,6 @@ #define CHROME_BROWSER_UI_WEBUI_PRINT_PREVIEW_EXTENSION_PRINTER_HANDLER_H_ #include -#include #include "base/macros.h" #include "base/memory/scoped_ptr.h" @@ -30,10 +29,6 @@ namespace cloud_devices { class CloudDeviceDescription; } -namespace device { -class UsbDevice; -} - namespace gfx { class Size; } @@ -98,7 +93,7 @@ class ExtensionPrinterHandler : public PrinterHandler { // They just propagate results to callbacks passed to them. void WrapGetPrintersCallback( const PrinterHandler::GetPrintersCallback& callback, - const base::ListValue& printers, + const base::ListValue& pritners, bool done); void WrapGetCapabilityCallback( const PrinterHandler::GetCapabilityCallback& callback, @@ -107,14 +102,10 @@ class ExtensionPrinterHandler : public PrinterHandler { void WrapPrintCallback(const PrinterHandler::PrintCallback& callback, bool success, const std::string& status); - void OnUsbDevicesEnumerated( - const PrinterHandler::GetPrintersCallback& callback, - const std::vector>& devices); content::BrowserContext* browser_context_; scoped_ptr pwg_raster_converter_; - int pending_enumeration_count_ = 0; scoped_refptr slow_task_runner_; diff --git a/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc b/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc index 94905a51b920..1f701a6067c5 100644 --- a/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc +++ b/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc @@ -13,36 +13,23 @@ #include "base/memory/scoped_ptr.h" #include "base/run_loop.h" #include "base/strings/string16.h" -#include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" -#include "base/test/values_test_util.h" #include "base/values.h" -#include "chrome/browser/extensions/test_extension_environment.h" #include "chrome/browser/local_discovery/pwg_raster_converter.h" #include "chrome/browser/ui/webui/print_preview/extension_printer_handler.h" #include "chrome/test/base/testing_profile.h" -#include "device/core/device_client.h" -#include "device/usb/mock_usb_device.h" -#include "device/usb/mock_usb_service.h" -#include "extensions/browser/api/device_permissions_manager.h" +#include "content/public/test/test_browser_thread_bundle.h" #include "extensions/browser/api/printer_provider/printer_provider_api.h" #include "extensions/browser/api/printer_provider/printer_provider_api_factory.h" #include "extensions/browser/api/printer_provider/printer_provider_print_job.h" -#include "extensions/common/extension.h" -#include "extensions/common/value_builder.h" #include "printing/pdf_render_settings.h" #include "printing/pwg_raster_settings.h" #include "printing/units.h" #include "testing/gtest/include/gtest/gtest.h" #include "ui/gfx/geometry/size.h" -using device::MockUsbDevice; -using device::MockUsbService; -using extensions::DictionaryBuilder; -using extensions::Extension; using extensions::PrinterProviderAPI; using extensions::PrinterProviderPrintJob; -using extensions::TestExtensionEnvironment; using local_discovery::PWGRasterConverter; namespace { @@ -131,50 +118,6 @@ const char kPrintTicketWithDuplex[] = " }" "}"; -// An extension with permission for 1 printer it supports. -const char kExtension1[] = - "{" - " \"name\": \"Provider 1\"," - " \"app\": {" - " \"background\": {" - " \"scripts\": [\"background.js\"]" - " }" - " }," - " \"permissions\": [" - " \"printerProvider\"," - " \"usb\"," - " {" - " \"usbDevices\": [" - " { \"vendorId\": 0, \"productId\": 1 }" - " ]" - " }," - " ]," - " \"usb_printers\": {" - " \"filters\": [" - " { \"vendorId\": 0, \"productId\": 0 }," - " { \"vendorId\": 0, \"productId\": 1 }" - " ]" - " }" - "}"; - -// An extension with permission for none of the printers it supports. -const char kExtension2[] = - "{" - " \"name\": \"Provider 2\"," - " \"app\": {" - " \"background\": {" - " \"scripts\": [\"background.js\"]" - " }" - " }," - " \"permissions\": [ \"printerProvider\", \"usb\" ]," - " \"usb_printers\": {" - " \"filters\": [" - " { \"vendorId\": 0, \"productId\": 0 }," - " { \"vendorId\": 0, \"productId\": 1 }" - " ]" - " }" - "}"; - const char kContentTypePDF[] = "application/pdf"; const char kContentTypePWG[] = "image/pwg-raster"; @@ -413,22 +356,6 @@ KeyedService* BuildTestingPrinterProviderAPI(content::BrowserContext* context) { return new FakePrinterProviderAPI(); } -class FakeDeviceClient : public device::DeviceClient { - public: - FakeDeviceClient() {} - - // device::DeviceClient implementation: - device::UsbService* GetUsbService() override { - DCHECK(usb_service_); - return usb_service_; - } - - void set_usb_service(device::UsbService* service) { usb_service_ = service; } - - private: - device::UsbService* usb_service_ = nullptr; -}; - } // namespace class ExtensionPrinterHandlerTest : public testing::Test { @@ -437,32 +364,35 @@ class ExtensionPrinterHandlerTest : public testing::Test { ~ExtensionPrinterHandlerTest() override = default; void SetUp() override { - extensions::PrinterProviderAPIFactory::GetInstance()->SetTestingFactory( - env_.profile(), &BuildTestingPrinterProviderAPI); + TestingProfile::Builder profile_builder; + profile_builder.AddTestingFactory( + extensions::PrinterProviderAPIFactory::GetInstance(), + &BuildTestingPrinterProviderAPI); + profile_ = profile_builder.Build(); + extension_printer_handler_.reset(new ExtensionPrinterHandler( - env_.profile(), base::MessageLoop::current()->task_runner())); + profile_.get(), base::MessageLoop::current()->task_runner())); pwg_raster_converter_ = new FakePWGRasterConverter(); extension_printer_handler_->SetPwgRasterConverterForTesting( scoped_ptr(pwg_raster_converter_)); - device_client_.set_usb_service(&usb_service_); } protected: FakePrinterProviderAPI* GetPrinterProviderAPI() { return static_cast( extensions::PrinterProviderAPIFactory::GetInstance() - ->GetForBrowserContext(env_.profile())); + ->GetForBrowserContext(profile_.get())); } - MockUsbService usb_service_; - TestExtensionEnvironment env_; scoped_ptr extension_printer_handler_; FakePWGRasterConverter* pwg_raster_converter_; private: - FakeDeviceClient device_client_; + content::TestBrowserThreadBundle thread_bundle_; + + scoped_ptr profile_; DISALLOW_COPY_AND_ASSIGN(ExtensionPrinterHandlerTest); }; @@ -519,68 +449,6 @@ TEST_F(ExtensionPrinterHandlerTest, GetPrinters_Reset) { EXPECT_EQ(0u, call_count); } -TEST_F(ExtensionPrinterHandlerTest, GetUsbPrinters) { - scoped_refptr device0 = - new MockUsbDevice(0, 0, "Google", "USB Printer", ""); - usb_service_.AddDevice(device0); - scoped_refptr device1 = - new MockUsbDevice(0, 1, "Google", "USB Printer", ""); - usb_service_.AddDevice(device1); - - const Extension* extension_1 = env_.MakeExtension( - *base::test::ParseJson(kExtension1), "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); - const Extension* extension_2 = env_.MakeExtension( - *base::test::ParseJson(kExtension2), "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"); - - extensions::DevicePermissionsManager* permissions_manager = - extensions::DevicePermissionsManager::Get(env_.profile()); - permissions_manager->AllowUsbDevice(extension_2->id(), device0); - - size_t call_count = 0; - scoped_ptr printers; - bool is_done = false; - extension_printer_handler_->StartGetPrinters( - base::Bind(&RecordPrinterList, &call_count, &printers, &is_done)); - - FakePrinterProviderAPI* fake_api = GetPrinterProviderAPI(); - ASSERT_TRUE(fake_api); - ASSERT_EQ(1u, fake_api->pending_get_printers_count()); - - EXPECT_EQ(1u, call_count); - EXPECT_FALSE(is_done); - EXPECT_TRUE(printers.get()); - EXPECT_EQ(2u, printers->GetSize()); - scoped_ptr extension_1_entry( - DictionaryBuilder() - .Set("id", base::StringPrintf("provisional-usb:%s:%u", - extension_1->id().c_str(), - device0->unique_id())) - .Set("name", "USB Printer") - .Set("extensionName", "Provider 1") - .Set("extensionId", extension_1->id()) - .Set("provisional", true) - .Build()); - scoped_ptr extension_2_entry( - DictionaryBuilder() - .Set("id", base::StringPrintf("provisional-usb:%s:%u", - extension_2->id().c_str(), - device1->unique_id())) - .Set("name", "USB Printer") - .Set("extensionName", "Provider 2") - .Set("extensionId", extension_2->id()) - .Set("provisional", true) - .Build()); - EXPECT_TRUE(printers->Find(*extension_1_entry) != printers->end()); - EXPECT_TRUE(printers->Find(*extension_2_entry) != printers->end()); - - fake_api->TriggerNextGetPrintersCallback(base::ListValue(), true); - - EXPECT_EQ(2u, call_count); - EXPECT_TRUE(is_done); - EXPECT_TRUE(printers.get()); - EXPECT_EQ(0u, printers->GetSize()); // RecordPrinterList resets |printers|. -} - TEST_F(ExtensionPrinterHandlerTest, GetCapability) { size_t call_count = 0; std::string destination_id; diff --git a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc index d80bb7a4f98f..0567ac7303ad 100644 --- a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc +++ b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc @@ -1683,7 +1683,7 @@ void PrintPreviewHandler::FillPrinterDescription( #endif // defined(ENABLE_SERVICE_DISCOVERY) void PrintPreviewHandler::EnsureExtensionPrinterHandlerSet() { - if (extension_printer_handler_) + if (extension_printer_handler_.get()) return; extension_printer_handler_ = diff --git a/extensions/common/api/printer_provider/usb_printer_manifest_data.cc b/extensions/common/api/printer_provider/usb_printer_manifest_data.cc index 4cfca0082fb1..61dc9e83bbea 100644 --- a/extensions/common/api/printer_provider/usb_printer_manifest_data.cc +++ b/extensions/common/api/printer_provider/usb_printer_manifest_data.cc @@ -5,13 +5,10 @@ #include "extensions/common/api/printer_provider/usb_printer_manifest_data.h" #include "base/strings/utf_string_conversions.h" -#include "device/usb/usb_device.h" #include "device/usb/usb_device_filter.h" #include "extensions/common/api/extensions_manifest_types.h" #include "extensions/common/manifest_constants.h" -using device::UsbDeviceFilter; - namespace extensions { UsbPrinterManifestData::UsbPrinterManifestData() { @@ -40,7 +37,7 @@ scoped_ptr UsbPrinterManifestData::FromValue( scoped_ptr result(new UsbPrinterManifestData()); for (const auto& input : usb_printers->filters) { DCHECK(input.get()); - UsbDeviceFilter output; + device::UsbDeviceFilter output; output.SetVendorId(input->vendor_id); if (input->product_id && input->interface_class) { *error = base::ASCIIToUTF16( @@ -64,9 +61,4 @@ scoped_ptr UsbPrinterManifestData::FromValue( return result.Pass(); } -bool UsbPrinterManifestData::SupportsDevice( - const scoped_refptr& device) const { - return UsbDeviceFilter::MatchesAny(device, filters_); -} - } // namespace extensions diff --git a/extensions/common/api/printer_provider/usb_printer_manifest_data.h b/extensions/common/api/printer_provider/usb_printer_manifest_data.h index ed397f0ede05..fae6053fc3c4 100644 --- a/extensions/common/api/printer_provider/usb_printer_manifest_data.h +++ b/extensions/common/api/printer_provider/usb_printer_manifest_data.h @@ -10,7 +10,6 @@ #include "extensions/common/extension.h" namespace device { -class UsbDevice; class UsbDeviceFilter; } @@ -31,8 +30,6 @@ class UsbPrinterManifestData : public Extension::ManifestData { static scoped_ptr FromValue(const base::Value& value, base::string16* error); - bool SupportsDevice(const scoped_refptr& device) const; - const std::vector& filters() const { return filters_; } -- 2.11.4.GIT